Forbid dispatch from inside a reducer

[gaearon]

We should throw if you attempt to dispatch while the reducer is being called.

Note that this will not cause the dreaded Cannot dispatch in a middle of dispatch if you dispatch from component's componentDidMount or componentWillReceiveProps. Unlike Flux, we don't have this problem, because we call all subscribers after the reducer is called.

This means we can set a flag immediately before currentState = reducer(currentState, action) in createStore and reset it on the next line, so everything happens before the subscribers are called. The subscribers should be able to call dispatch just fine.

I'd very much like to see this as a PR as I don't have much time. It needs to:

• Introduce the error if you dispatch while the reducer is evaluating.
• Include a test for this.
• Should not fail the existing “should handle nested dispatches gracefully” test

Please build it on top of breaking-changes-1.0 branch.

[gaearon]

Another note: need another test to check that an error inside a reducer resets the flag back, i.e. there should be try/finally in the implementation to reset the flag.

[knowbody]

I'm on it @gaearon

[phated]

@gaearon what is the reason for adding this? I've disliked it in every Flux implementation that has it, and not just because the lifecycle issues.

[gaearon]

@phated Simply because this is never a valid use case. Reducers must be pure.

[gaearon]

In Flux there are valid-ish situations when you'd rather wish this check didn't exist, but in Redux there's just no excuse for dispatching inside reducer. It's never the right solution, and it only indicates a problem in the code.

[phated]

Can't it be implemented as middleware and be opt-in or opt-out. Having explicit throws that you can't turn off in library code is a pain.

[jide]

For complicated edge cases there are middlewares.

[phated]

Maybe it should just be a warning then?

[gaearon]

If it's a middleware, I doubt anybody who'd be affected by the error (beginners) would know to use it.
Why would you want to dispatch inside a reducer? It's grossly misusing the library.
It's exactly the same as React doesn't allow you to setState inside render.

[gaearon]

In fact a custom middleware can opt you “out” of this behavior by queueing the “bad” dispatch on a next tick. But I still can't envision a scenario where you'd need it.

[phated]

I've been thinking about API design a lot lately and I've come to determine that authors can't see all use cases that consumers will do, so I think avoiding things that completely break them (like throwing) should be avoided. I agree that it is incorrect usage, so I think there should be a friendly warning (like React's dev warnings) but not an explicit throw.

[gaearon]

Generally I agree with your statement. It's a tricky line. For example, Redux doesn't try to enforce that you don't mutate the state. Mutating is the wrong thing to do and is a common source of mistakes, but we recognize that people sometimes might have valid reasons to mutate. We might warn them in the future, we might offer opt-in freeze-all-state plugins, but we won't disallow it.

However there are some fundamental assumptions a library can make too. In case of Redux, it's a fundamental assumption that reducers don't cause side effects. You can always trivially have an upgrade path by moving this dispatch call into an action creator. It's not like we're getting in somebody's way.

As you said, React has dev warnings, it's true. But if you try to setState from render it will throw hard. Precisely because “render doesn't have side effects” is a fundamental assumption React makes, and if you're not following it, you can't write a correct React app.

Dispatching from inside a reducer is never correct because you'll overwrite the result of your first dispatch. It will be lost and unaccounted for, and will seem like a weird bug to the consumer. This is the reason it's better to throw and educate and offer an upgrade path, than to allow the wrong behavior.

[gaearon]

In other words, throwing will break them explicitly, but allowing to do that will break them implicitly.

[knowbody]

looks good @quicksnap

@gaearon going to move to the next things

[quicksnap]

@knowbody Sorry about the collision of work! I was almost done with it and then needed to run to a meeting.

[knowbody]

@quicksnap it's totally cool. I don't mind. and we are another step closer to the 1.0 release 😄

[gaearon]

Fixed by #372.

[bpmckee]

@gaearon When starting with redux I was angry I couldn't dispatch from a reducer in certain cases. I agree it shouldn't be done to keep reducers pure but I never really found a good workaround, all my workarounds suck.

For example, if a REST call fails, I'd want to display a basic error modal.
What I originally wanted to do was this:

export function fooBarReducer(state = {}, action) {
    switch (action.type) {
        case types.GET_SPECIFIC_DATA_ERROR:
            dispatch(openErrorModal(errorTypes.FAILED_REST_CALL));
            return {
                ...state,
                loading: false,
            };
        default:
            return state;
    }
}

It seems silly but the only alternative I've come up with is this:

// reducer
export function fooBarReducer(state = {}, action) {
    switch (action.type) {
        case types.GET_SPECIFIC_DATA_ERROR:
            return {
                ...state,
                loading: false,
                error: true,
            };
        default:
            return state;
    }
}

// component
export class FooBarLayout extends React.Component {
    componentDidUpdate(nextProps) {
        const { error, dispatch } = this.props;
        if (!error && nextProps.error) {
            dispatch(openErrorModal(errorTypes.FAILED_REST_CALL));
        }
    }

    render() {
        // note: <ErrorModal /> is near the root level.
        return (
            <div>
                <FooBarComponent />
                <LoadingOverlay loading={this.props.loading} />
            </div>
        );
    }
}

This works just fine but I can't use a function for the component anymore because I need the react lifecycle events. There are different variations of this code where the error modal is in this component but it require the component to be rendered back to back twice.

Do you have a good solution for this problem? It seems like most async events have this problem and it's hard to find a nice clean solution.

Other examples would be:

• Submit data and show a loading indicator. On complete navigate to a different state, on error show an error.
• Delete data, show an undo snackbar for a few seconds.

[gaearon]

This is one alternative. Another alternative is to use Redux Thunk and add this logic to your thunk action creator. The "async" example in the repo shows this. Finally you can check out redux-saga project which addresses this specific problem.

[sompylasar]

@bpmckee, try thinking of your modal not like you need to "openErrorModal" (imperative state transition) but like "isErrorModalOpen" currently and which "error" should it display (declarative state), and then connect your React component that displays the modal to that state--you'll get this state into the component props and will render the modal like any other component. You need to store the error type in the state, too, not just "error: true", and reset the error with an action when it's time to dismiss the modal.

[bpmckee]

@gaearon redux-saga looks very interesting I'll have to experiment with it. My first impressions are that it looks very promising though.

@sompylasar I was thinking about the declarative state for the modal but the more I thought about it, the more complex I'd have to make the state object for that modal. In hindsight I probably should have went with it but there were too many issues going through my mind earlier. Such as what happens if the user hits back, what if I want a different part of the view to still reflect an error occurred even after the error modal was closed, etc.

Anyway, thank you both for the helpful info! Sorry it ended up hijacking this thread.

[kirkstrobeck]

Also see https://github.com/redux-loop/redux-loop
A common sense approach to this need.