From eb711baffcc3c910347610df9c9b64c9b24bcb91 Mon Sep 17 00:00:00 2001 From: Mike Wilcox Date: Sat, 2 Apr 2016 14:42:29 -0400 Subject: [PATCH 1/5] throw error if getState, subscribe, or unsubscribe called while dispatching --- src/createStore.js | 12 +++++++++++ test/createStore.spec.js | 35 +++++++++++++++++++++++++++++- test/helpers/actionCreators.js | 31 ++++++++++++++++++++++++++- test/helpers/actionTypes.js | 3 +++ test/helpers/reducers.js | 39 +++++++++++++++++++++++++++++++++- 5 files changed, 117 insertions(+), 3 deletions(-) diff --git a/src/createStore.js b/src/createStore.js index 68097d1ce3..8b1d60749a 100644 --- a/src/createStore.js +++ b/src/createStore.js @@ -71,6 +71,10 @@ export default function createStore(reducer, initialState, enhancer) { * @returns {any} The current state tree of your application. */ function getState() { + if (isDispatching) { + throw new Error('Reducers may not access store state.') + } + return currentState } @@ -102,12 +106,20 @@ export default function createStore(reducer, initialState, enhancer) { throw new Error('Expected listener to be a function.') } + if (isDispatching) { + throw new Error('Reducers may not subscribe.') + } + var isSubscribed = true ensureCanMutateNextListeners() nextListeners.push(listener) return function unsubscribe() { + if (isDispatching) { + throw new Error('Reducers may not unsubscribe.') + } + if (!isSubscribed) { return } diff --git a/test/createStore.spec.js b/test/createStore.spec.js index 3a8af0630b..39abc23871 100644 --- a/test/createStore.spec.js +++ b/test/createStore.spec.js @@ -1,6 +1,14 @@ import expect from 'expect' import { createStore, combineReducers } from '../src/index' -import { addTodo, dispatchInMiddle, throwError, unknownAction } from './helpers/actionCreators' +import { + addTodo, + dispatchInMiddle, + getStateInMiddle, + subscribeInMiddle, + unsubscribeInMiddle, + throwError, + unknownAction +} from './helpers/actionCreators' import * as reducers from './helpers/reducers' describe('createStore', () => { @@ -451,6 +459,31 @@ describe('createStore', () => { ).toThrow(/may not dispatch/) }) + it('does not allow getState() from within a reducer', () => { + const store = createStore(reducers.getStateInTheMiddleOfReducer) + + expect(() => + store.dispatch(getStateInMiddle(store.getState.bind(store))) + ).toThrow(/may not access store state/) + }) + + it('does not allow subscribe() from within a reducer', () => { + const store = createStore(reducers.subscribeInTheMiddleOfReducer) + + expect(() => + store.dispatch(subscribeInMiddle(store.subscribe.bind(store, () => {}))) + ).toThrow(/may not subscribe/) + }) + + it('does not allow unsubscribe from subscribe() from within a reducer', () => { + const store = createStore(reducers.unsubscribeInTheMiddleOfReducer) + const unsubscribe = store.subscribe(() => {}) + + expect(() => + store.dispatch(unsubscribeInMiddle(unsubscribe.bind(store))) + ).toThrow(/may not unsubscribe/) + }) + it('recovers from an error within a reducer', () => { const store = createStore(reducers.errorThrowingReducer) expect(() => diff --git a/test/helpers/actionCreators.js b/test/helpers/actionCreators.js index 198f61be20..5c26cdcc41 100644 --- a/test/helpers/actionCreators.js +++ b/test/helpers/actionCreators.js @@ -1,4 +1,12 @@ -import { ADD_TODO, DISPATCH_IN_MIDDLE, THROW_ERROR, UNKNOWN_ACTION } from './actionTypes' +import { + ADD_TODO, + DISPATCH_IN_MIDDLE, + GET_STATE_IN_MIDDLE, + SUBSCRIBE_IN_MIDDLE, + UNSUBSCRIBE_IN_MIDDLE, + THROW_ERROR, + UNKNOWN_ACTION +} from './actionTypes' export function addTodo(text) { return { type: ADD_TODO, text } @@ -26,6 +34,27 @@ export function dispatchInMiddle(boundDispatchFn) { } } +export function getStateInMiddle(boundGetStateFn) { + return { + type: GET_STATE_IN_MIDDLE, + boundGetStateFn + } +} + +export function subscribeInMiddle(boundSubscribeFn) { + return { + type: SUBSCRIBE_IN_MIDDLE, + boundSubscribeFn + } +} + +export function unsubscribeInMiddle(boundUnsubscribeFn) { + return { + type: UNSUBSCRIBE_IN_MIDDLE, + boundUnsubscribeFn + } +} + export function throwError() { return { type: THROW_ERROR diff --git a/test/helpers/actionTypes.js b/test/helpers/actionTypes.js index 00092962f2..2e6104345c 100644 --- a/test/helpers/actionTypes.js +++ b/test/helpers/actionTypes.js @@ -1,4 +1,7 @@ export const ADD_TODO = 'ADD_TODO' export const DISPATCH_IN_MIDDLE = 'DISPATCH_IN_MIDDLE' +export const GET_STATE_IN_MIDDLE = 'GET_STATE_IN_MIDDLE' +export const SUBSCRIBE_IN_MIDDLE = 'SUBSCRIBE_IN_MIDDLE' +export const UNSUBSCRIBE_IN_MIDDLE = 'UNSUBSCRIBE_IN_MIDDLE' export const THROW_ERROR = 'THROW_ERROR' export const UNKNOWN_ACTION = 'UNKNOWN_ACTION' diff --git a/test/helpers/reducers.js b/test/helpers/reducers.js index 8e9c7321ec..31ce7b99e1 100644 --- a/test/helpers/reducers.js +++ b/test/helpers/reducers.js @@ -1,4 +1,11 @@ -import { ADD_TODO, DISPATCH_IN_MIDDLE, THROW_ERROR } from './actionTypes' +import { + ADD_TODO, + DISPATCH_IN_MIDDLE, + GET_STATE_IN_MIDDLE, + SUBSCRIBE_IN_MIDDLE, + UNSUBSCRIBE_IN_MIDDLE, + THROW_ERROR +} from './actionTypes' function id(state = []) { @@ -46,6 +53,36 @@ export function dispatchInTheMiddleOfReducer(state = [], action) { } } +export function getStateInTheMiddleOfReducer(state = [], action) { + switch (action.type) { + case GET_STATE_IN_MIDDLE: + action.boundGetStateFn() + return state + default: + return state + } +} + +export function subscribeInTheMiddleOfReducer(state = [], action) { + switch (action.type) { + case SUBSCRIBE_IN_MIDDLE: + action.boundSubscribeFn() + return state + default: + return state + } +} + +export function unsubscribeInTheMiddleOfReducer(state = [], action) { + switch (action.type) { + case UNSUBSCRIBE_IN_MIDDLE: + action.boundUnsubscribeFn() + return state + default: + return state + } +} + export function errorThrowingReducer(state = [], action) { switch (action.type) { case THROW_ERROR: From 7507eaea1b67f5874df31f150933d35b1b001be9 Mon Sep 17 00:00:00 2001 From: Mike Wilcox Date: Sat, 2 Apr 2016 14:56:15 -0400 Subject: [PATCH 2/5] prevent throwing if not subscribed --- src/createStore.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/createStore.js b/src/createStore.js index 8b1d60749a..525d1313ae 100644 --- a/src/createStore.js +++ b/src/createStore.js @@ -115,15 +115,15 @@ export default function createStore(reducer, initialState, enhancer) { ensureCanMutateNextListeners() nextListeners.push(listener) - return function unsubscribe() { - if (isDispatching) { - throw new Error('Reducers may not unsubscribe.') - } - + return function unsubscribe() { if (!isSubscribed) { return } + if (isDispatching) { + throw new Error('Reducers may not unsubscribe.') + } + isSubscribed = false ensureCanMutateNextListeners() From f957b9fef40200362b24c3738eba4f8d8db1e010 Mon Sep 17 00:00:00 2001 From: Mike Wilcox Date: Sat, 2 Apr 2016 15:44:39 -0400 Subject: [PATCH 3/5] update getState error message --- src/createStore.js | 8 ++++++-- test/createStore.spec.js | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/createStore.js b/src/createStore.js index 525d1313ae..e52ecf2db4 100644 --- a/src/createStore.js +++ b/src/createStore.js @@ -72,7 +72,11 @@ export default function createStore(reducer, initialState, enhancer) { */ function getState() { if (isDispatching) { - throw new Error('Reducers may not access store state.') + throw new Error( + 'You may not call store.getState() while the reducer is executing.' + + 'The reducer has already received the state as an argument.' + + 'Pass it down from the top reducer instead of reading it from the store.' + ) } return currentState @@ -115,7 +119,7 @@ export default function createStore(reducer, initialState, enhancer) { ensureCanMutateNextListeners() nextListeners.push(listener) - return function unsubscribe() { + return function unsubscribe() { if (!isSubscribed) { return } diff --git a/test/createStore.spec.js b/test/createStore.spec.js index 39abc23871..9c49b4e7d4 100644 --- a/test/createStore.spec.js +++ b/test/createStore.spec.js @@ -464,7 +464,7 @@ describe('createStore', () => { expect(() => store.dispatch(getStateInMiddle(store.getState.bind(store))) - ).toThrow(/may not access store state/) + ).toThrow(/You may not call store.getState()/) }) it('does not allow subscribe() from within a reducer', () => { From efd0d2e18bc019105d313b6b038e22bfb76e52c8 Mon Sep 17 00:00:00 2001 From: Mike Wilcox Date: Sat, 2 Apr 2016 17:01:25 -0400 Subject: [PATCH 4/5] fix space after period --- src/createStore.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/createStore.js b/src/createStore.js index e52ecf2db4..b89d77f419 100644 --- a/src/createStore.js +++ b/src/createStore.js @@ -73,8 +73,8 @@ export default function createStore(reducer, initialState, enhancer) { function getState() { if (isDispatching) { throw new Error( - 'You may not call store.getState() while the reducer is executing.' + - 'The reducer has already received the state as an argument.' + + 'You may not call store.getState() while the reducer is executing. ' + + 'The reducer has already received the state as an argument. ' + 'Pass it down from the top reducer instead of reading it from the store.' ) } From 5065cab676f58846a8597cb3a38781b700c6fac0 Mon Sep 17 00:00:00 2001 From: Mike Wilcox Date: Tue, 5 Apr 2016 22:29:19 -0400 Subject: [PATCH 5/5] update subscribe/unsubscribe error messages --- src/createStore.js | 12 ++++++++++-- test/createStore.spec.js | 4 ++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/createStore.js b/src/createStore.js index b89d77f419..238157b4d8 100644 --- a/src/createStore.js +++ b/src/createStore.js @@ -111,7 +111,12 @@ export default function createStore(reducer, initialState, enhancer) { } if (isDispatching) { - throw new Error('Reducers may not subscribe.') + throw new Error( + 'You may not call store.subscribe() while the reducer is executing. ' + + 'If you would like to be notified after the store has been updated, subscribe from a ' + + 'component and invoke store.getState() in the callback to access the latest state. ' + + 'See http://redux.js.org/docs/api/Store.html#subscribe for more details.' + ) } var isSubscribed = true @@ -125,7 +130,10 @@ export default function createStore(reducer, initialState, enhancer) { } if (isDispatching) { - throw new Error('Reducers may not unsubscribe.') + throw new Error( + 'You may not unsubscribe from a store listener while the reducer is executing. ' + + 'See http://redux.js.org/docs/api/Store.html#subscribe for more details.' + ) } isSubscribed = false diff --git a/test/createStore.spec.js b/test/createStore.spec.js index 9c49b4e7d4..b2b6105bf2 100644 --- a/test/createStore.spec.js +++ b/test/createStore.spec.js @@ -472,7 +472,7 @@ describe('createStore', () => { expect(() => store.dispatch(subscribeInMiddle(store.subscribe.bind(store, () => {}))) - ).toThrow(/may not subscribe/) + ).toThrow(/You may not call store.subscribe()/) }) it('does not allow unsubscribe from subscribe() from within a reducer', () => { @@ -481,7 +481,7 @@ describe('createStore', () => { expect(() => store.dispatch(unsubscribeInMiddle(unsubscribe.bind(store))) - ).toThrow(/may not unsubscribe/) + ).toThrow(/You may not unsubscribe from a store/) }) it('recovers from an error within a reducer', () => {