Checking that the return value of every store function inside compose… #174
Conversation
…Stores is not undefined
|
|
||
| export default function composeStores(stores) { | ||
| const finalStores = pick(stores, (val) => typeof val === 'function'); | ||
|
|
||
| Object.keys(finalStores).forEach(key => { | ||
| invariant( |
There was a problem hiding this comment.
Should this be a hard error, or a warning in dev?
There was a problem hiding this comment.
@gaearon I guess the answer to that is: would undefined be a valid state? If so, then I think that we should warn, otherwise fail :)
There was a problem hiding this comment.
I think it's a symptom of problem but I'm not sure it's worth failing in production. It's like wrong propTypes.
…Stores is not undefined, production triggers warning for undefined actions, reducers act like identity function
|
@honzatrtik @gaearon I feel this is only needed during development. I think it would be best if we don't have this check in production. Wrap the whole thing in Instead of just changing the error type. It will make production performant and not need to go through unnecessary code checks. |
| Object.keys(finalStores).forEach(key => { | ||
| if (finalStores[key](dummyStore, dummyAction) !== dummyStore) { | ||
| const message = `Your ${key} Store must return the state given to it for any unknown actions.`; | ||
| if (process.env.NODE_ENV === 'production') { |
There was a problem hiding this comment.
Let's do it the other way.
In development, we warn. In production, we skip this whole check (lines 10 to 20).
|
I gave this some thought. This change might cause some issues. If someone has runtime type checking in their reducers, this will cause an error. Also, If they have some type of reducer that counts the number of actions that go through the reducer no matter the action. This change will make their code always show a warning. If we don't think either of those are likely, then the benefits outweigh those downsides. |
What kind of type checking are you referring to?
Reducers must be pure in Redux. That's a hard constraint but it's not currently enforced. It's better if we log a warning than just behave weirdly. |
I am not sure if default Flow would allow this to pass if your reducer function doesn't have type Object for state. For instance in the counter example, the reducer always expects a number type state. This check is passing an object to all reducers. |
|
Oh, I see now.. I haven't really thought about Flow in the context of this change. |
|
My suggestion: pass |
|
Yup, i will try to implement #191 & apropriate tests tonight... |
|
I think @taylorhakes is working on this already: #193 |
|
oh, i see! ok then. |
…Stores is not undefined