Warns when type of reducers passed is invalid to avoid silent failure #2636
Conversation
…id silent failure
test/combineReducers.spec.js
Outdated
| ) | ||
|
|
||
| expect(spy.mock.calls[1][0]).toMatch( | ||
| /Reducer provided for "broken" is not a function/ |
| it('warns on props which are not a function and excludes them', () => { | ||
| const preSpy = console.error | ||
| const spy = jest.fn() | ||
| console.error = spy |
There was a problem hiding this comment.
You've overridden console.error, but there's no mechanism here to put it back after the test.
The Jest way to do this is to make a mock of utils/warning to capture any calls to warning().
jest.mock('../src/helpers/warning')https://facebook.github.io/jest/docs/en/manual-mocks.html has more info :)
There was a problem hiding this comment.
They can do what we do in other tests. Just add this at the end:
spy.mockClear()
console.error = preSpy
|
(Maintainers: sorry for the impromptu review; @jekku is a colleague of mine who's asked his coworkers to review his contribution.) |
test/combineReducers.spec.js
Outdated
| const reducer = combineReducers({ | ||
| fake: true, | ||
| broken: 'string', | ||
| another: { nested: 'object' }, | ||
| stack: (state = []) => state | ||
| }) | ||
|
|
||
| expect(spy.mock.calls[0][0]).toMatch( |
There was a problem hiding this comment.
Another (less-precise, but terser) way to do this is
expect(spy).toHaveBeenCalledWith('Reducer provided for "fake" is not a function')...which you can use if you're stubbing warning() instead of console.error.
src/combineReducers.js
Outdated
| @@ -106,8 +106,11 @@ export default function combineReducers(reducers) { | |||
| const key = reducerKeys[i] | |||
|
|
|||
| if (process.env.NODE_ENV !== 'production') { | |||
| if (typeof reducers[key] === 'undefined') { | |||
| const reducerUnderInspection = reducers[key] | |||
There was a problem hiding this comment.
You shouldn't need to extract this out. Just keep using reducers[key]. It's probably not as fast, but this is dev-only code.
|
Whoops, got a little cocky with those test updates. Serves me right for trying to use the web interface to edit 😄 Feel free to revert that! |
jekku
force-pushed
the
warn-on-invalid-reducer-types
branch
from
703761f to
c169608
Compare
|
One thing to note: We tried this on bindActionCreators and had to revert there as well: #2279 Unfortunately, with most people transpiling modules through Babel, there are side effects when the pattern of |
|
@timdorr would throwing out an exception when an |
|
@jekku Sorry for not responding. At least currently, there isn't a rock-solid, cross-platform way of doing that. You can check for Unfortunately, fixing for this kind of issue isn't going to be possible in the current ecosystem. I had to revert a similar warning in a previous release because it caused other sorts of headaches like this. Unless someone can think up a novel way to resolve it that covers both native and loosely transpiled environments, I'm not sure there's a solution. |
Attempts to solve issue #1398 .
Undefined reducers are still checked for intuitive debugging.
Warnings are now given to the console if a passed reducer is not a function.
Warnings are also given to the console if a whole es module imported via the * syntax is passed to combineReducers.