Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flux Standard Action #205

Closed
acdlite opened this issue Jul 2, 2015 · 23 comments
Closed

Flux Standard Action #205

acdlite opened this issue Jul 2, 2015 · 23 comments

Comments

@acdlite
Copy link
Collaborator

acdlite commented Jul 2, 2015

I whipped together this minimal standard for Flux actions. Quoting from the README:

It's much easier to work with Flux actions if we can make certain assumptions about their shape. For example, essentially all Flux actions have an identifier field, such as type, actionType, or actionId. Many Flux implementations also include a way for actions to indicate success or failure, especially as the result of a data-fetching operation. Defining a minimal, common standard for these patterns enables the creation of useful tools and abstractions.

I came up with these initial specs based on my experience with Flummox, which internally uses a similar pattern for action handling. The key difference is that with Flummox, the implementation was baked into the core, whereas Redux allows us to take a much more modular, flexible approach.

I absolutely don't think this should be part of the Redux core, but it'd be nice to encourage middleware/extension users support this standard (or something like it) so that they're all interoperable.

To see an example of how FAS is useful, see this section of redux-promise.

Thoughts? Would love to receive feedback.

EDIT: Forgot to mention that I created some tools for creating and handling FSA actions in Redux. Look at handleActions() in particular for an example of how to create a reducer that handle multiple actions https://github.com/acdlite/redux-fsa#handleactionsreducermap-defaultstate.

@acdlite acdlite changed the title Flux Action Standard Flux Standard Action Jul 2, 2015
@emmenko
Copy link
Contributor

emmenko commented Jul 2, 2015

I like the idea. So basically like a "json-schema", only for actions, right?

@clearjs
Copy link
Contributor

clearjs commented Jul 2, 2015

Interface instead of assumptions?

The overall idea is great, but there are lots of options how to achieve it. One of possibilities is to define an "interface" (a set of functions that a user can provide) which would give users full control. This might work best if redux had some sane defaults.

I have no specific suggestions yet, because for making them it would be better to examine several popular action "schemas" first.

For example, current FSA version would require a change to a popular pattern of "SOME_ACTION_REQUESTED", "SOME_ACTION_SUCCESS", "SOME_ACTION_ERROR", where status is part of action's name, and the first of these actions may be handled in a reducer and impact state.

Drawback of using isSuccess / isError, is that many reducers would need to check for both, leading to lots of code duplication.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 2, 2015

@emmenko Pretty much, except much more limited in scope.

@clearjs

The overall idea is great, but there are lots of options how to achieve it.

This is the point of defining a standard, right? To agree on some basic conventions so we can build tools around it.

For example, current FSA version would require a change to popular pattern of "SOME_ACTION_REQUESTED", "SOME_ACTION_SUCCESS", "SOME_ACTION_ERROR", where status is part of action's name, and the first of these actions may be handled in a reducer and impact state.

Yes, this is a popular pattern today, but it can easily be expressed using FSA's status field, which has the advantage of 1) not requiring multiple action types, and 2) explicitly indicating the success or failure of an action, without needing to parse the action type using a regex or something.

Drawback of using isSuccess / isError, is that many reducers would need to check for both, leading to lots of code duplication.

You wouldn't have to use isSuccess / isError. The code for both is very simple. It's mostly just there as a reference implementation.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 2, 2015

Take a look at the source of redux-promise as an example: https://github.com/acdlite/redux-promise/blob/master/src/index.js#L15-L20

@clearjs
Copy link
Contributor

clearjs commented Jul 2, 2015

This is the point of defining a standard, right?

Exactly. My point is that it might (but might not) be better to give users control and give middleware some "interface".

You wouldn't have to use isSuccess / isError.

What I meant was that for "SOME_ACTION_SUCCESS" there might be a separate reducer from the other two. No need for a check at all.

Another point was that not all actions are either success or error. "Something has been requested" may have impact on state as well.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 2, 2015

You can still have separate reducers. The reducer just needs to check the status. See redux-fsa's handleAction() function, which does exactly this.

@clearjs
Copy link
Contributor

clearjs commented Jul 2, 2015

What I suggest is to discuss a few popular alternative action schemas somewhere, then think about best way to standardise.

@clearjs
Copy link
Contributor

clearjs commented Jul 2, 2015

You can still have separate reducers. The reducer just needs to check the status.

Adding some boilerplate.

See redux-fsa's handleAction() function, which does exactly this.

This looks very nice to me.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 2, 2015

@clearjs Sure, of course, this standard isn't set in stone by any means. This initial version is based on my specific experiences with Flummox, but I'm totally open to changing and evolving it. The most important thing is that it's something the community can agree upon.

@clearjs
Copy link
Contributor

clearjs commented Jul 2, 2015

How to organise this discussion best? Maybe create one issue in the FSA repo per a schema that somebody uses in real life?

UPDATE: thinking more about this, the handleAction() example solves my current objections, and I wouldn't have raised them if I saw that first.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 2, 2015

Another thing to note is that a middleware like redux-promise simply doesn't work with the ACTION_SUCCESS, ACTION_ERROR pattern without appending strings to the type. And then on the other side, the reducer has to do a action.type.endsWith('_SUCCESS') check, which is pretty gross IMO. It's better to have separate fields for type and status rather than overload them in a single field.

@clearjs Yeah, for in depth discussion of this it's probably best to open issues in the FSA repo.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 2, 2015

@gaearon I know you're busy this week but I'd love to hear your thoughts, as well!

@clearjs
Copy link
Contributor

clearjs commented Jul 2, 2015

And then on the other side, the reducer has to do a action.type.endsWith('_SUCCESS') check, which is pretty gross IMO.

Not if the reducer only handles SOME_ACTION_SUCCESS. (Just a remark, you've already convinced me.)

EDIT: @acdlite I was still thinking in general, but now see that your remark is specific to redux-promise.

@bdowning
Copy link

bdowning commented Jul 2, 2015

@acdlite, when you said:

Unlike Flummox, it will not perform a dispatch at the beginning of the operation, only at the end. Use Redux's built-in thunkMiddleware in combination with redux-promise to perform optimistic updates.

Could you briefly state how that would be structured? I'm having a little bit of a hard time understanding how the order of middlewares would affect a thunk-dispatching-a-promise verses a promise-returning-a-thunk. Also if I'm programming with promises having to fall back to a hand-written thunk rather than just using the ES7 async functionality wouldn't be my first choice.

But like I said I'm not completely clear on what's going on here, so it may make perfect sense.

@bdowning
Copy link

bdowning commented Jul 2, 2015

(Also, is it legal to dispatch multiple times from a thunk-handled action? Glancing at the code I'd say yes, but...)

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 2, 2015

@bdowning Yes, it is legal :)

The reason for not dispatching at the beginning is because usually when doing optimistic updates, you need to dispatch certain information like an id or a timestamp. Since there's no obvious pattern for how to do this, for now I'm suggesting that redux-promise users just use the thunk middleware.

Basic example:

// Relies on both thunk and promise middleware
createAction('FETCH_SOMETHING', id => {
  // Thunk will be handled by thunk middleware
  return dispatch => {
    // Trigger optimistic update
    dispatch({ type: 'FETCH_SOMETHING_BEGIN', body: { id } });

    // Promise will be handled by promise middleware
    return api.fetch(id);
  }
});

We're considering adding another status type besides error and success (perhaps pending?) though I'm not sure about this yet. redux-utilities/flux-standard-action#1 (comment)

@bdowning
Copy link

bdowning commented Jul 2, 2015

@acdlite And just to check, for your example to work is there a specific ordering of thunk and promise middleware that has to be installed, or would any order work?

-bcd

@gaearon
Copy link
Contributor

gaearon commented Jul 2, 2015

I'll take a look soon. Too tired :-)

@lexfrl
Copy link

lexfrl commented Jul 3, 2015

The main problem with this standardization process is I think that this kind of "actions" should be generic and this question shouldn't be exists at all. Also I found that other aspects Flux architecture itself is unnecessary overcomplicated in general.
I've started an alternative to Flux arch that's follows the same unidirectional approach but it differs in focus on the consistency of the state at any point of time. You could check it out here https://github.com/AlexeyFrolov/slt . It's on the early point of development, and it lack of examples (I'm quite busy with my projects and I have a lack of free time today to make a good presentation) and community, but it's well tested and I'm already using it in production in my projects.

@acstll
Copy link

acstll commented Jul 3, 2015

@acdlite I really like this FSA idea 👍 in fact I use this { type: String, payload: Object } signature myself.

But I'm not sure about the status string (even if it allows these helpers here). It seems like added complexity, you're now handling 2 values, one constant and a plain string which could be error prone.

I mean, this

switch (action.type) {
  case FETCH_DATA:
  // ..
    break

  case FETCH_DATA_SUCCESS:
  // ..
    break

  case FETCH_DATA_ERROR:
  // ..
    break
}

is much more simpler than this

switch (action.type) {
  case FETCH_DATA:
  if (action.status === 'success') {
    // ..
  } else if (action.status === 'error') {
    // ..
  } else {
    // ..
  }
     break
}

Don't you think? That's my 2c

@clearjs
Copy link
Contributor

clearjs commented Jul 3, 2015

@acstll There is an interesting discussion on this topic here: redux-utilities/flux-standard-action#1, and apparently in the #redux Slack channel as well.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 3, 2015

@acstll The reason for a status field is so we can do this:

handleAction('FETCH_DATA', {
  success(state, action) {...}
  error(state, action) {...}
});

https://github.com/acdlite/redux-fsa#handleactiontype-reducer--reducermap

@gaearon
Copy link
Contributor

gaearon commented Jul 30, 2015

I'm closing, as it's not a Redux issue, and it has been dormant for a while.
FSA is mentioned in the new docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants