Move safety checks inside combineReducer to first invocation

[gaearon]

Currently we make a bunch of invariant tests inside combineReducers() to make sure it's not being misused in common ways. These tests currently occur at the combineReducers() call time.

This makes Redux DevTools unable to catch the errors inside reducers, as they often trigger those invariants to blow up during module definition, and abort hot reloading. Instead, I propose to move the tests to the first real invocation of the combined reducer. In most cases that would be createStore call because it causes an implicit initial dispatch. This way Redux DevTools can catch and display such errors.

Shall we do that? The code would change to be like

  var reducerSanityVerified;
  var stateShapeVerified;

  // don't do it here

  return function combination(state = defaultState, action) {
    if (!reducerSanityVerified) {
      Object.keys(finalReducers).forEach(key => {
        // do that stuff

[gaearon]

Another possible alternative is to create errors and console.error() them ASAP during combineReducers() call, but throw during first dispatch. This way we both have a nice stack and keep DevTools happy.

[gaearon]

Would anybody like to work on this?
@ellbee, do you think you might have the time?

[ellbee]

Sure, I'll be able to take a look in the next couple of days.

[ellbee]

@gaearon I have been working on this, but I am new to the dark arts of HMR and I have a question.

In the following module taken from the counter example, I think delaying the sanity check until the dispatch of the @@init action will still blow up the module definition because the init action is getting dispatched either inside createStore or replaceReducer. This module is going to get re-evaluated too when a reducer changes I think?

import React, { Component } from 'react';
import CounterApp from './CounterApp';
import { createStore, applyMiddleware, combineReducers, compose } from 'redux';
import { devTools, persistState } from 'redux-devtools';
import { DevTools, DebugPanel, LogMonitor } from 'redux-devtools/lib/react';
import thunk from 'redux-thunk';
import { Provider } from 'react-redux';
import * as reducers from '../reducers';

const finalCreateStore = compose(
  applyMiddleware(thunk),
  devTools(),
  persistState(window.location.href.match(/[?&]debug_session=([^&]+)\b/))
)(createStore);

const reducer = combineReducers(reducers);
const store = finalCreateStore(reducer);

if (module.hot) {
  module.hot.accept('../reducers', () =>
    store.replaceReducer(combineReducers(require('../reducers')))
  );
}

export default class App extends Component {
  render() {
    return (
      <div>
        <Provider store={store}>
          {() => <CounterApp />}
        </Provider>
        <DebugPanel top right bottom>
          <DevTools store={store}
                    monitor={LogMonitor}
                    visibleOnLoad={true} />
        </DebugPanel>
      </div>
    );
  }
}

I've got a couple of potential ideas to get around this if it is indeed a problem, just wanted to check that I am not misunderstanding how the HMR works.

[gaearon]

It will below up the first time you launch the app but I don't think it's a big deal. It's iteration that I'm concerned about (changing reducers or adding new ones). In case of iteration only the reducer file and replaceReducer() call will be evaluated.

[ellbee]

Ah, thanks! That makes total sense. I should read my error messages. The error thrown on iteration is caused by the Connect already being hooked up and mapStateToProps being triggered by the dispatch in replaceReducers with undefined state.

[gaearon]

Yeah—that's a different thing, reduxjs/redux-devtools#106

[ellbee]

So to achieve our goal that devtools issue will need to be addressed too I guess. Having this error thrown from the dispatch in replaceReducers is resulting in [HMR] Cannot apply update. Need to do a full reload!, so devtools isn't displaying the errors like we want.

[gaearon]

Yes, but these are two separate issues. When I tested, fixing them both locally fixed the problem.

[ellbee]

Yes, agreed.

[gaearon]

Done in #717.

[gaearon]

Released in 3.0.1.

[gaearon]

There is still a problem with the scenario I described above. If the reducer throws, then the reducer sanity check itself will throw. In other words,

  Object.keys(finalReducers).forEach(key => {
    var reducer = finalReducers[key];
    if (!sanityError && typeof reducer(undefined, { type: ActionTypes.INIT }) === 'undefined') {

See that typeof reducer(...) call? It will blow up if reducer throws, blowing up combineReducers anyway. I'd say that the way sanity check should be changed is the following:

• Let sanity check throw
• Catch the error outside Object.keys(...) and store as sanityError
• Like now, throw it on first dispatch

@ellbee Can you look into this?

[ellbee]

Ah yes, I see. Quick fix attempt here #807

[gaearon]

Fixed via #807.

[gaearon]

Final fix in 3.0.2, confirmed as working.
Thanks again.