Warn if the initial state shape doesn't match the reducer names #302

ellbee · 2015-07-22T22:17:58Z

Ok, not super happy with this but it is a start.

I have written tests for verifyStateShape, but I couldn't think of a neat way to test the code in createStore so there is currently a hole in the test coverage.
I don't really like the function name verifyStateShape. Is stateShapeWarning better?
I haven't used an external warning module. Would it be a good idea? I'm currently just calling console.warn.

Happy to take some direction on this.

gaearon · 2015-07-23T07:37:23Z

Let's do this in combineReducers instead?

michalkvasnicak · 2015-07-23T08:41:16Z

I think it's enough to put it here before return https://github.com/gaearon/redux/blob/breaking-changes-1.0/src/utils/combineReducers.js#L63

gaearon · 2015-07-23T08:44:34Z

I think we should change state = {} to state = mapValues(finalReducers, () => undefined) so if the state isn't rehydrated, the warning is skipped.

michalkvasnicak · 2015-07-23T08:45:41Z

I think that it needs an extra check because typeof [] or typeof null return object too.

ellbee@0fee440#diff-bc52e10bb986be8700c6e212e9bc581cR22

gaearon · 2015-07-23T08:50:11Z

We don't need to worry about this if the check is moved to combineReducers because the root state is then guaranteed to be an object.

michalkvasnicak · 2015-07-23T08:51:22Z

@gaearon Yeah you are right, it needs to call verifyState after mapValues are done, otherwise it'll warn everytime. I did't get mapValues right at first look.

gaearon · 2015-07-23T08:52:03Z

Hmm. The initial state can still be wrong. I guess we should just check isPlainObject on it, shouldn't we? If it's not a plain object something is already bad.

michalkvasnicak · 2015-07-23T08:53:36Z

@gaearon I agree. Initial state should be checked to be a plain object.

emmenko · 2015-07-23T10:11:49Z

PS: this is a duplicate of #276 . We can close mine, but maybe you can keep the failing test?

ellbee · 2015-07-23T10:56:52Z

@gaearon Yes, no problem. I've moved the check to combineReducers.

gaearon · 2015-07-23T10:58:21Z

src/utils/verifyStateShape.js

+      'match shape of state object returned by reducer';
+  }
+
+  return '';


Better return null IMO.

ellbee · 2015-07-23T12:02:45Z

@gaearon I've hopefully addressed your comments in the latest commit

@emmenko Do you think we still need the failing test in createStore? I'll certainly add it if so.

gaearon · 2015-07-23T12:47:19Z

src/utils/combineReducers.js

+    warning(
+      verifyStateShape(state, finalState),
+      'Shape of initial state object given to createStore does not ' +
+      'match shape of state object returned by reducer'


By this point the user does not really define a reducer. All they do is pass reducers object (which they might get from import * as something ES6 import). Therefore they won't understand which reducer the warning message is referring to.

The warning should instead say something like Unexpected key "<key>" in the initialState will be ignored. Expected to find one of the known reducer keys instead: "oneReducer", "otherReducer", ....

ellbee · 2015-07-23T15:09:53Z

Updated with better warning message

gaearon · 2015-07-23T15:45:20Z

Looks really good. I'll test this in a day or two and get this into 1.0 release.

👍

gaearon · 2015-07-23T15:45:44Z

Please rebase on breaking-changes-1.0 and squash this into a single commit.

emmenko · 2015-07-23T16:12:21Z

@ellbee I think it's ok like this, just it's not really the complete flow. Usually you do createStore(reducer, initialState) and store.getState().
But I'm not sure if it's really necessary...

gaearon · 2015-07-23T16:51:36Z

@emmenko Do you mean the tests? I don't think it's a problem because this is behavior completely specific to combineReducers.

ellbee · 2015-07-23T17:11:26Z

Ok, thanks @gaearon! Rebased and squashed.

emmenko · 2015-07-23T17:12:04Z

👍

gaearon · 2015-07-23T17:15:02Z

src/utils/combineReducers.js

 import mapValues from '../utils/mapValues';
 import pick from '../utils/pick';
-import invariant from 'invariant';
-import { ActionTypes } from '../createStore';
+import warning from 'warning';


Nitpick: let's move invariant here so their imports are together.

Move invariant next to warning in the import order? Can do, I just put them into alphabetical order.

gaearon · 2015-07-24T20:32:48Z

I played with this a little bit more, and I think we could still improve this a little bit:

When the initial state is plain object, there is no warning. What I meant is, we should always warn if the initial state is not a plain object: that's a symptom of a mistake. There is no way for combineReducers to do anything useful with something other than a plain object.
If I remove export default from todos.js in todomvc example, I get a cryptic error message: Unexpected key "todos" in the initialState will be ignored. Expected to find one of the known reducer keys instead: "". What does "" mean? It means Object.keys(finalReducers).length === 0. But it's definitely a symptom of a mistake! Evidently the user to forgot to export any reducer, so we should probably have a better warning in this situation.
The check is being performed at every invocation, not just at the initial one. This feels like a waste even in development environment. Can we only emit the warning the first time the function is invoked? I understand it makes it impure, but it's no big deal in development environment: we just want to warn (and check) only once. Does this make sense?

emmenko · 2015-07-24T20:43:10Z

Good points!

Can we only emit the warning the first time the function is invoked?

emmenko · 2015-07-24T20:44:41Z

Sorry, my finger slipped...

I wanted to ask if it would be a problem with hot reloading?

Can we only emit the warning the first time the function is invoked?

gaearon · 2015-07-24T22:16:34Z

I don't think this would be a problem? On hot reloading the function will be re-generated so the code will run again.

emmenko · 2015-07-24T23:04:10Z

Ok, just wanted to make sure :)

ellbee · 2015-07-25T10:57:19Z

Updated pull request to add extra warnings and only call verifyStateShape once as suggested. Ideas for improvements to the warning messages are welcome!

I have been wondering though if these new warnings should be invariants instead?

ellbee · 2015-07-30T11:30:25Z

@gaearon I'll have some spare time later today, is there anything else you would like me to do with this PR?

gaearon · 2015-07-30T12:46:29Z

@ellbee

I don't have time to look at it right now, but please do what I'd do:

Take a sample app
Turn your mind off and make stupid mistakes with initialState, exporting reducers, export default, etc
See if the warnings help you or not

ellbee · 2015-07-30T13:01:30Z

@gaearon OK, will do. Thanks!

gaearon · 2015-07-31T10:07:38Z

Hey, awesome job. Thanks.

nuragic · 2015-08-26T13:58:55Z

Hi! I just get this warning and I don't understand what's the problem with this one..

I mean, I have an initial state from the server that looks like this for example:

{
  foo: [/* ... */], // ok, I've a reducer called 'foo'
  bar: true,        // no reducer
  baz: false,       // same here

}

Should I have one reducer for every single state property in order to access it from my components?

gaearon · 2015-08-26T14:01:45Z

Yes, if you use combineReducers, because that's exactly what it does. It lets you assign a single reducer to each state property.

You can also write the root reducer by hand, without combineReducers. Then it's up to you how to structure it.

https://github.com/rackt/redux/blob/master/docs/api/combineReducers.md

https://github.com/rackt/redux/blob/master/docs/basics/Reducers.md

nuragic · 2015-08-26T14:16:27Z

Thanks for the quick response!

Ok, point taken... Maybe I need to think a bit more about my app state in order to know if I need the 1:1 relation between state properties and reducers... Another option could be to have 1 reducer to manage the entire state right?

gaearon · 2015-08-26T14:17:30Z

Yes. In fact that's what combineReducers generates — a single reducer. (And instead of using combineReducers you can write it by hand.)

nuragic · 2015-08-26T14:35:28Z

Ok, I'll evaluate if I need to write my own or simply use one "master" reducer (I'm doing a small app btw eheh).

Many thanks for the support! :)

ellbee changed the title ~~Warn if the initial state shape doesn't match the reducer names WIP~~ Warn if the initial state shape doesn't match the reducer names #272 WIP Jul 22, 2015

gaearon reviewed Jul 23, 2015
View reviewed changes

src/utils/verifyStateShape.js

'match shape of state object returned by reducer';

}

return '';

Copy link

Contributor

gaearon Jul 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better return null IMO.

gaearon reviewed Jul 23, 2015
View reviewed changes

emmenko mentioned this pull request Jul 23, 2015

[WIP] Warn if initial state shape doesn't match reducer name #276

Closed

2 tasks

gaearon reviewed Jul 23, 2015
View reviewed changes

gaearon changed the title ~~Warn if the initial state shape doesn't match the reducer names #272 WIP~~ Warn if the initial state shape doesn't match the reducer names Jul 24, 2015

emmenko closed this Jul 24, 2015

emmenko reopened this Jul 24, 2015

add verifyStateShape function and tests

fca60c8

This was referenced Jul 30, 2015

Umbrella 1.0 #164

Closed

Warn in combineReducers if the initial state shape doesn't match the reducer names #272

Closed

gaearon merged commit fca60c8 into reduxjs:breaking-changes-1.0 Jul 31, 2015

Warn if the initial state shape doesn't match the reducer names #302

Warn if the initial state shape doesn't match the reducer names #302

Conversation

ellbee commented Jul 22, 2015

gaearon commented Jul 23, 2015

michalkvasnicak commented Jul 23, 2015

gaearon commented Jul 23, 2015

michalkvasnicak commented Jul 23, 2015

gaearon commented Jul 23, 2015

michalkvasnicak commented Jul 23, 2015

gaearon commented Jul 23, 2015

michalkvasnicak commented Jul 23, 2015

emmenko commented Jul 23, 2015

ellbee commented Jul 23, 2015

gaearon Jul 23, 2015

Choose a reason for hiding this comment

ellbee commented Jul 23, 2015

gaearon Jul 23, 2015

Choose a reason for hiding this comment

ellbee commented Jul 23, 2015

gaearon commented Jul 23, 2015

gaearon commented Jul 23, 2015

emmenko commented Jul 23, 2015

gaearon commented Jul 23, 2015

ellbee commented Jul 23, 2015

emmenko commented Jul 23, 2015

gaearon Jul 23, 2015

Choose a reason for hiding this comment

ellbee Jul 23, 2015

Choose a reason for hiding this comment

ellbee Jul 23, 2015

Choose a reason for hiding this comment

gaearon commented Jul 24, 2015

emmenko commented Jul 24, 2015

emmenko commented Jul 24, 2015

gaearon commented Jul 24, 2015

emmenko commented Jul 24, 2015

ellbee commented Jul 25, 2015

ellbee commented Jul 30, 2015

gaearon commented Jul 30, 2015

ellbee commented Jul 30, 2015

gaearon commented Jul 31, 2015

nuragic commented Aug 26, 2015

gaearon commented Aug 26, 2015

nuragic commented Aug 26, 2015

gaearon commented Aug 26, 2015

nuragic commented Aug 26, 2015