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

Relax requirement to not update state for statuses other than "success" / "error" #1

Closed
clearjs opened this issue Jul 2, 2015 · 19 comments

Comments

@clearjs
Copy link

clearjs commented Jul 2, 2015

I'd like to be able to handle the fact that some async action has started, and have state reflex that.

@acdlite
Copy link
Contributor

acdlite commented Jul 2, 2015

Could you give me more details? Quoting from README:

Other values of status are valid, but only success and error are treated with any special meaning.

@acdlite
Copy link
Contributor

acdlite commented Jul 2, 2015

Ah, do you mean that this requirement should be lifted?

If status is defined but not one of success or error, the consumer MUST NOT respond to the action.

Thinking about it more that's probably a good idea. The consumer should be allowed to choose whether or not to respond to it.

@clearjs
Copy link
Author

clearjs commented Jul 2, 2015

Yep. Lifted or somehow amended.

@clearjs
Copy link
Author

clearjs commented Jul 2, 2015

One complication is that such status might need to be treated like success in some cases. No specific examples yet, but I can come up with one if needed. Possible solution is to introduce another special status for pending.

@trabianmatt
Copy link

Would it be worth considering pending as a status with special meaning?

@acdlite
Copy link
Contributor

acdlite commented Jul 2, 2015

@trabianmatt What would the special meaning be? How should the consumer interpret it?

@clearjs
Copy link
Author

clearjs commented Jul 2, 2015

Consumer (reducer) might apply an optimistic update. Also, UI could display some visual indication (a loader or a notification). The former has been discussed elsewhere, and the latter might be a separate action.

The more I think about it, the less I like an idea of the "pending" status. Still, would be interesting to compare how this may work with and without a special status.

@acdlite
Copy link
Contributor

acdlite commented Jul 2, 2015

Both of those feel like separate action types to me.

Think of an action as being a part of a stream of actions with the same type. If you think about other kinds of streams — let's use RxJS as an example — a subscriber to a stream (in this analogy, that's our store or reducer function) has onNext() and onError() functions.

  • onNext() corresponds to a status of success
  • onError() corresponds to a status of error

There's also onComplete() that is called when the stream terminates, but a stream of Flux actions never truly ends. It continues throughout the lifecycle of the application. So it's not relevant in this case.

So a status of pending doesn't fit into this metaphor. I can see how a pending flag would be useful for optimistic updates, but I don't think it belongs in the status field.

Maybe we should rename status so that it's more clear? Can't think of a better alternative.

@clearjs
Copy link
Author

clearjs commented Jul 2, 2015

Rx has great abstractions, I always enjoy using them. Agree with your reasoning here.

Maybe we should rename status so that it's more clear? Can't think of a better alternative.

Did you consider having a falsy (or missing) error property for success and truthy for error actions?

@acdlite
Copy link
Contributor

acdlite commented Jul 2, 2015

Yes, that's actually what Flummox does, but having a single status field felt more explicit and easier to work with.

@clearjs
Copy link
Author

clearjs commented Jul 2, 2015

Let's see what others think, but now I like the error property much more as it does what is says and only that.

@acdlite
Copy link
Contributor

acdlite commented Jul 2, 2015

Now that I see how status could be confusing, I'm inclined to agree with you, but let's take some time to think about it.

@tappleby
Copy link

tappleby commented Jul 3, 2015

I think part of the issue is we need a way to identify when an action has "started".

@acdlite you mentioned how a flux stream never completes, which is true, but within that stream there are sequences of actions that do begin and complete.

Perhaps there is a way we could identify a sequence, which would consist of a start, complete and N intermediate actions. The complete action must also indicate if it is a success|error.

In the case of the Promise it would just be a sequence of 2 actions. For a more complex example we could take something that needs to report progress such as loading a file or processing data:

[
    { type: 'LOAD_MEDIA', mediaId: 42, sequence: 0 },
    { type: 'LOAD_MEDIA', mediaId: 42, sequence: 1, payload: { progress: 10 } },
    { type: 'LOAD_MEDIA', mediaId: 42, sequence: 2, payload: { progress: 67 } },
    { type: 'LOAD_MEDIA', mediaId: 42, sequence: 3, payload: { progress: 84 } },
    { type: 'LOAD_MEDIA', mediaId: 42, sequence: 4, status: 'success', payload: { id: 42, path: '...' } },
]

The above example is a bit contrived since a progress action could be handled with a different type.

@acdlite
Copy link
Contributor

acdlite commented Jul 3, 2015

I agree this is a common pattern, just not sure if belongs in the spec... Maybe as an extension, but not part of the core. Let's keep thinking about it.

@clearjs
Copy link
Author

clearjs commented Jul 3, 2015

@tappleby each of those actions may be considered to have status "success" if we define the latter as "no error".

Depending on needs and priorities of a specific project, at least a few alternative solutions are possible:

  • Use a (single) dedicated action type (e.g., 'UPDATE_PROGRESS') for tracking progress for all actions that need this. Add fields for the "main" action type (e.g., 'LOAD_MEDIA'), progress, and an ID to correlate a specific series of actions. These may either be meta fields (added to the action object), or be part of payload.

    Possibly use helpers to fire these progress updates from any async action creator that needs them. A single reducer may handle all progress updates. Using multiple reducers (e.g., one per main action type) is also easy, just need to check the main action type field in them.

  • Same as above, but each main action type has a separate corresponding action type for tracking progress (e.g., 'LOAD_MEDIA_PROGRESS'). In this case there is no need to keep another field for the main action type.

  • Treat progress as part of state. No need for introducing additional action types. This may make reducer logic more complicated, but still simple enough for some use cases.

One of these, or some more specific/generic approach might become part of some spec extension, after at least 1-2 implementations are available and battle-tested in production projects. Alternatively, they may be described as recipes and simply go to the documentation.

NOTE: there are many more cross-cutting concerns like this. E.g., ability to cancel a series of actions, retry failed operations, etc.

@trabianmatt
Copy link

A better name might be begin (for my purposes, at least). What I'd like to be able to do is:

// Action
createAction('FETCH_THING', async id => {
  const res = await api.fetchThing(id);
  return res.body;
});

// Reducer
handleAction('FETCH_THING', {
  begin(state, action) {
    return { ...state, pending: true };
  },
  success(state, action) {...},
  error(state, action) {...}
});

@acdlite
Copy link
Contributor

acdlite commented Jul 3, 2015

@trabianmatt In that example, what does the action payload look like in begin()? Usually you want to send along some kind of ID. Flummox addresses this by just passing along the arguments passed to the action creator, but this has always felt like a hacky solution to me.

Another problem with that API is mixes up separate concerns: begin is about time, but success and error are about whether or not an error occurred.

In redux-actions v0.6, the handlers were renamed to next() and throw() to make this a bit clearer, and to emulate the ES6 generator interface.

What about something like this?

handleAsyncAction('FETCH_THING', {
  begin(state, action) {...}, 
  end: {
    success(state, action) {...},
    error(state, action) {...}
  }
});

@acdlite
Copy link
Contributor

acdlite commented Jul 3, 2015

Closing this since the original issue was about status, which is superseded by error in v0.6. #6

@acdlite acdlite closed this as completed Jul 3, 2015
@acdlite
Copy link
Contributor

acdlite commented Jul 3, 2015

Please continue any further discussion over here #7

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

4 participants