From 9045a0c1732ab266a1ff09926420f266e1a74d65 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 13 Jul 2015 19:10:03 +0300 Subject: [PATCH] Handling private actions is an anti-pattern. Enforce it. (Fixes #186) --- src/Store.js | 9 ++++- src/utils/combineReducers.js | 32 ++++++++++++----- test/utils/combineReducers.spec.js | 56 ++++++++++++++++++++---------- 3 files changed, 68 insertions(+), 29 deletions(-) diff --git a/src/Store.js b/src/Store.js index ed85b987fc..2459fe8d31 100644 --- a/src/Store.js +++ b/src/Store.js @@ -1,6 +1,13 @@ import invariant from 'invariant'; import isPlainObject from './utils/isPlainObject'; +// Don't ever try to handle these action types in your code. They are private. +// For any unknown actions, you must return the current state. +// If the current state is undefined, you must return the initial state. +export const ActionTypes = { + INIT: '@@redux/INIT' +}; + export default class Store { constructor(reducer, initialState) { invariant( @@ -19,7 +26,7 @@ export default class Store { replaceReducer(nextReducer) { this.reducer = nextReducer; - this.dispatch({ type: '@@INIT' }); + this.dispatch({ type: ActionTypes.INIT }); } dispatch(action) { diff --git a/src/utils/combineReducers.js b/src/utils/combineReducers.js index fe65c056cc..4008a5587e 100644 --- a/src/utils/combineReducers.js +++ b/src/utils/combineReducers.js @@ -1,19 +1,11 @@ import mapValues from '../utils/mapValues'; import pick from '../utils/pick'; import invariant from 'invariant'; +import { ActionTypes } from '../Store'; function getErrorMessage(key, action) { const actionType = action && action.type; const actionName = actionType && `"${actionType}"` || 'an action'; - const reducerName = `Reducer "${key}"`; - - if (actionType === '@@INIT') { - return ( - `${reducerName} returned undefined during initialization. ` + - `If the state passed to the reducer is undefined, ` + - `you must explicitly return the initial state.` - ); - } return ( `Reducer "${key}" returned undefined handling ${actionName}. ` + @@ -24,6 +16,28 @@ function getErrorMessage(key, action) { export default function combineReducers(reducers) { const finalReducers = pick(reducers, (val) => typeof val === 'function'); + Object.keys(finalReducers).forEach(key => { + const reducer = finalReducers[key]; + invariant( + typeof reducer(undefined, { type: ActionTypes.INIT }) !== 'undefined', + `Reducer "${key}" returned undefined during initialization. ` + + `If the state passed to the reducer is undefined, you must ` + + `explicitly return the initial state. The initial state may ` + + `not be undefined.` + ); + + const type = Math.random().toString(36).substring(7).split('').join('.'); + invariant( + typeof reducer(undefined, { type }) !== 'undefined', + `Reducer "${key}" returned undefined when probed with a random type. ` + + `Don't try to handle ${ActionTypes.INIT} or other actions in "redux/*" ` + + `namespace. They are considered private. Instead, you must return the ` + + `current state for any unknown actions, unless it is undefined, ` + + `in which case you must return the initial state, regardless of the ` + + `action type. The initial state may not be undefined.` + ); + }); + return function composition(state = {}, action) { return mapValues(finalReducers, (reducer, key) => { const newState = reducer(state[key], action); diff --git a/test/utils/combineReducers.spec.js b/test/utils/combineReducers.spec.js index 63dc58bbd8..b72429e38a 100644 --- a/test/utils/combineReducers.spec.js +++ b/test/utils/combineReducers.spec.js @@ -1,5 +1,6 @@ import expect from 'expect'; import { combineReducers } from '../../src'; +import { ActionTypes } from '../../src/Store'; describe('Utils', () => { describe('combineReducers', () => { @@ -30,43 +31,44 @@ describe('Utils', () => { ).toEqual(['stack']); }); - it('should throw an error if a reducer returns undefined', () => { + it('should throw an error if a reducer returns undefined handling an action', () => { const reducer = combineReducers({ - undefinedByDefault(state = 0, action) { + counter(state = 0, action) { switch (action && action.type) { case 'increment': return state + 1; case 'decrement': return state - 1; - case '@@INIT': - return state; - default: + case 'whatever': + case null: + case undefined: return undefined; + default: + return state; } } }); - const initialState = reducer(undefined, { type: '@@INIT' }); expect( - () => reducer(initialState, { type: 'whatever' }) + () => reducer({ counter: 0 }, { type: 'whatever' }) ).toThrow( - /"undefinedByDefault".*"whatever"/ + /"counter".*"whatever"/ ); expect( - () => reducer(initialState, null) + () => reducer({ counter: 0 }, null) ).toThrow( - /"undefinedByDefault".*an action/ + /"counter".*an action/ ); expect( - () => reducer(initialState, {}) + () => reducer({ counter: 0 }, {}) ).toThrow( - /"undefinedByDefault".*an action/ + /"counter".*an action/ ); }); it('should throw an error if a reducer returns undefined initializing', () => { - const reducer = combineReducers({ - undefinedInitially(state, action) { + expect(() => combineReducers({ + counter(state, action) { switch (action.type) { case 'increment': return state + 1; @@ -76,12 +78,28 @@ describe('Utils', () => { return state; } } - }); + })).toThrow( + /"counter".*initialization/ + ); + }); - expect( - () => reducer(undefined, { type: '@@INIT' }) - ).toThrow( - /"undefinedInitially".*initialization/ + it('should throw an error if a reducer attempts to handle a private action', () => { + expect(() => combineReducers({ + counter(state, action) { + switch (action.type) { + case 'increment': + return state + 1; + case 'decrement': + return state - 1; + // Never do this in your code: + case ActionTypes.INIT: + return 0; + default: + return undefined; + } + } + })).toThrow( + /"counter".*private/ ); }); });