Gracefully handle nested dispatches #110
Conversation
|
Thanks for the failing test case. Best way to start a conversation :-)
Yes. Let's solve this somehow. I'm currently busy with time travel but I'm sure @acdlite, @johanneslumpe, @goatslacker could provide valuable insight from their experience building Flux libraries (even on top of FB's dispatcher).. |
|
I do not have a solution for this now but I remember that nested dispatches are something that's difficult to handle on the server-side. Let's say we have the classic react-router example where we want to fetch some data prior to rendering. We added some static handlers to our container components, and inject redux etc etc. to fetch some data. Let's use a promise middleware too. It returns the promise from |
|
We could do this:
Am I making sense? |
|
I think we should probably just throw for now, so that a user at least knows what's going on and isn't stumped by the fact that the state isn't what they expected. I do like the the |
|
I think the problem with the last action overriding the entire atom of the first dispatch gets fixed with #119, since the problem is that the dispatcher is using its internal state to update, which is set after setting the state in the redux instance's state and calling the listeners. I'm saying this because after that PR gets merged, the nested dispatches would set the whole state as expected, so it could be an option for desired behavior. Having said that, I'm not sure if this would cause any edge cases, or if it will increase the mental model both on developing redux (taking into account supporting nested dispatches), and in userland. |
|
@gaearon @johanneslumpe just to let you know that it seems |
@leoasis Would you like to implement this as part of this PR? |
|
This is actually caused by a subtle bug in the current version where This is kind of bug that #119 is intended to avoid by moving all state into the Redux instance. I've added the test to that PR to show that it passed. (Modified slightly — the original version causes an infinite loop because this line is always true: leoasis@e23b56b#diff-2dce452f5f745e87ca49080c71d81a21R87.) See the passing test here: b71a7fc |
|
So long story short, nested dispatches in Redux aren't a problem like they are in other libraries, because our "dispatcher" is just a reduce operation. |
|
@acdlite awesome, yeah that's what I thought in #110 (comment), so let's keep nested dispatches supported and fix that bug in your PR!
Edit: disregard the last paragraph, I see why it was causing an infinite loop now |
|
@leoasis The infinite loop never kicked off in the original case for the same reason the test failed: the call to |
|
@acdlite yeah, realised that right after sending the comment :P |
|
@leoasis :) |
|
Perfect then, since this will be fixed in #119, I'm closing this one |
Naming: * “Stateless Stores” are now called reducers. (#137 (comment)) * The “Redux instance” is now called “The Store”. (#137 (comment)) * The dispatcher is removed completely. (#166 (comment)) API changes: * <s>`composeStores`</s> is now `composeReducers`. * <s>`createDispatcher`</s> is gone. * <s>`createRedux`</s> is now `createStore`. * `<Provider>` now accepts `store` prop instead of <s>`redux`</s>. * The new `createStore` signature is `createStore(reducer: Function | Object, initialState: any, middlewares: Array | ({ getState, dispatch }) => Array)`. * If the first argument to `createStore` is an object, `composeReducers` is automatically applied to it. * The “smart” middleware signature changed. It now accepts an object instead of a single `getState` function. The `dispatch` function lets you “recurse” the middleware chain and is useful for async: #113 (comment). Correctness changes: * The `dispatch` provided by the default thunk middleware now walks the whole middleware chain. * It is enforced now that raw Actions at the end of the middleware chain have to be plain objects. * Nested dispatches are now handled gracefully. (#110) Internal changes: * The object in React context is renamed from <s>`redux`</s> to `store`. * Some tests are rewritten for clarity, focus and edge cases. * Redux in examples is now aliased to the source code for easier work on master.
Naming: * “Stateless Stores” are now called reducers. (#137 (comment)) * The “Redux instance” is now called “The Store”. (#137 (comment)) * The dispatcher is removed completely. (#166 (comment)) API changes: * <s>`composeStores`</s> is now `composeReducers`. * <s>`createDispatcher`</s> is gone. * <s>`createRedux`</s> is now `createStore`. * `<Provider>` now accepts `store` prop instead of <s>`redux`</s>. * The new `createStore` signature is `createStore(reducer: Function | Object, initialState: any, middlewares: Array | ({ getState, dispatch }) => Array)`. * If the first argument to `createStore` is an object, `composeReducers` is automatically applied to it. * The “smart” middleware signature changed. It now accepts an object instead of a single `getState` function. The `dispatch` function lets you “recurse” the middleware chain and is useful for async: #113 (comment). Correctness changes: * The `dispatch` provided by the default thunk middleware now walks the whole middleware chain. * It is enforced now that raw Actions at the end of the middleware chain have to be plain objects. * Nested dispatches are now handled gracefully. (#110) Internal changes: * The object in React context is renamed from <s>`redux`</s> to `store`. * Some tests are rewritten for clarity, focus and edge cases. * Redux in examples is now aliased to the source code for easier work on master.
Naming: * “Stateless Stores” are now called reducers. (reduxjs/redux#137 (comment)) * The “Redux instance” is now called “The Store”. (reduxjs/redux#137 (comment)) * The dispatcher is removed completely. (reduxjs/redux#166 (comment)) API changes: * <s>`composeStores`</s> is now `composeReducers`. * <s>`createDispatcher`</s> is gone. * <s>`createRedux`</s> is now `createStore`. * `<Provider>` now accepts `store` prop instead of <s>`redux`</s>. * The new `createStore` signature is `createStore(reducer: Function | Object, initialState: any, middlewares: Array | ({ getState, dispatch }) => Array)`. * If the first argument to `createStore` is an object, `composeReducers` is automatically applied to it. * The “smart” middleware signature changed. It now accepts an object instead of a single `getState` function. The `dispatch` function lets you “recurse” the middleware chain and is useful for async: reduxjs/redux#113 (comment). Correctness changes: * The `dispatch` provided by the default thunk middleware now walks the whole middleware chain. * It is enforced now that raw Actions at the end of the middleware chain have to be plain objects. * Nested dispatches are now handled gracefully. (reduxjs/redux#110) Internal changes: * The object in React context is renamed from <s>`redux`</s> to `store`. * Some tests are rewritten for clarity, focus and edge cases. * Redux in examples is now aliased to the source code for easier work on master.
Naming: * “Stateless Stores” are now called reducers. (reduxjs/redux#137 (comment)) * The “Redux instance” is now called “The Store”. (reduxjs/redux#137 (comment)) * The dispatcher is removed completely. (reduxjs/redux#166 (comment)) API changes: * <s>`composeStores`</s> is now `composeReducers`. * <s>`createDispatcher`</s> is gone. * <s>`createRedux`</s> is now `createStore`. * `<Provider>` now accepts `store` prop instead of <s>`redux`</s>. * The new `createStore` signature is `createStore(reducer: Function | Object, initialState: any, middlewares: Array | ({ getState, dispatch }) => Array)`. * If the first argument to `createStore` is an object, `composeReducers` is automatically applied to it. * The “smart” middleware signature changed. It now accepts an object instead of a single `getState` function. The `dispatch` function lets you “recurse” the middleware chain and is useful for async: reduxjs/redux#113 (comment). Correctness changes: * The `dispatch` provided by the default thunk middleware now walks the whole middleware chain. * It is enforced now that raw Actions at the end of the middleware chain have to be plain objects. * Nested dispatches are now handled gracefully. (reduxjs/redux#110) Internal changes: * The object in React context is renamed from <s>`redux`</s> to `store`. * Some tests are rewritten for clarity, focus and edge cases. * Redux in examples is now aliased to the source code for easier work on master.
Naming: * “Stateless Stores” are now called reducers. (reduxjs/redux#137 (comment)) * The “Redux instance” is now called “The Store”. (reduxjs/redux#137 (comment)) * The dispatcher is removed completely. (reduxjs/redux#166 (comment)) API changes: * <s>`composeStores`</s> is now `composeReducers`. * <s>`createDispatcher`</s> is gone. * <s>`createRedux`</s> is now `createStore`. * `<Provider>` now accepts `store` prop instead of <s>`redux`</s>. * The new `createStore` signature is `createStore(reducer: Function | Object, initialState: any, middlewares: Array | ({ getState, dispatch }) => Array)`. * If the first argument to `createStore` is an object, `composeReducers` is automatically applied to it. * The “smart” middleware signature changed. It now accepts an object instead of a single `getState` function. The `dispatch` function lets you “recurse” the middleware chain and is useful for async: reduxjs/redux#113 (comment). Correctness changes: * The `dispatch` provided by the default thunk middleware now walks the whole middleware chain. * It is enforced now that raw Actions at the end of the middleware chain have to be plain objects. * Nested dispatches are now handled gracefully. (reduxjs/redux#110) Internal changes: * The object in React context is renamed from <s>`redux`</s> to `store`. * Some tests are rewritten for clarity, focus and edge cases. * Redux in examples is now aliased to the source code for easier work on master.
Currently, when we happen to have nested dispatches, the last action's returned state prevails, even if they belong to different stores. The nested dispatch still sees the state as if the outer dispatch wasn't executed yet, so it ends up overwriting whatever that outer dispatch did.
Perhaps it's more clear with the failing test that I'm providing here. I'd expect there to either have both dispatches applied, or the dispatcher to throw an exception or a warning, as the original Facebook's Flux dispatcher.
The use case for running into nested dispatches is the classic "need data in componentWillMount", which triggers an action creator that dispatches an action immediately, and is executed as a consequence of a previous dispatch.
I'm leaving this unsolved until we decide the way to go, after that I can fix the test with the expected behavior