From 151154f27859257d6ebc615b007143524e7588cc Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 3 Feb 2016 04:53:44 +0000 Subject: [PATCH 1/2] Tweak the API to handle initial state correctly --- README.md | 18 +++-- examples/basic/app.js | 18 +++-- examples/basic/package.json | 4 +- package.json | 2 +- src/index.js | 103 ++++++++++++++++------------ test/createTests.js | 133 +++++++++++++++++++++++++++++++----- 6 files changed, 202 insertions(+), 76 deletions(-) diff --git a/README.md b/README.md index 98bf28a..0b0781a 100644 --- a/README.md +++ b/README.md @@ -61,14 +61,18 @@ const reducer = combineReducers(Object.assign({}, reducers, { routing: routeReducer })) -// Sync dispatched route actions to the history +// specify the history to listen to const reduxRouterMiddleware = syncHistory(browserHistory) -const createStoreWithMiddleware = applyMiddleware(reduxRouterMiddleware)(createStore) - -const store = createStoreWithMiddleware(reducer) +const store = createStore( + reducer, + applyMiddleware(reduxRouterMiddleware) +) -// Required for replaying actions from devtools to work -reduxRouterMiddleware.listenForReplays(store) +// begin syncing +reduxRouterMiddleware.syncWith(store, { + urlToState: true, // route changes will appear in state + stateToUrl: false // set to true for time travel in DevTools +}) ReactDOM.render( @@ -140,7 +144,7 @@ Examples from the community: _Have an example to add? Send us a PR!_ -### API +### API (TODO) #### `syncHistory(history: History) => ReduxMiddleware` diff --git a/examples/basic/app.js b/examples/basic/app.js index 00f5b3d..894f352 100644 --- a/examples/basic/app.js +++ b/examples/basic/app.js @@ -27,12 +27,18 @@ const DevTools = createDevTools( ) -const finalCreateStore = compose( - applyMiddleware(middleware), - DevTools.instrument() -)(createStore) -const store = finalCreateStore(reducer) -middleware.listenForReplays(store) +const store = createStore( + reducer, + compose( + applyMiddleware(middleware), + DevTools.instrument() + ) +) + +middleware.syncWith(store, { + urlToState: true, + stateToUrl: true +}) ReactDOM.render( diff --git a/examples/basic/package.json b/examples/basic/package.json index bbebb67..e807752 100644 --- a/examples/basic/package.json +++ b/examples/basic/package.json @@ -9,8 +9,8 @@ "react-dom": "^0.14.2", "react-redux": "^4.0.0", "react-router": "^1.0.0", - "redux": "^3.0.4", - "react-router-redux": "^2.1.0" + "react-router-redux": "^2.1.0", + "redux": "^3.2.1" }, "devDependencies": { "babel-core": "^6.1.21", diff --git a/package.json b/package.json index a4ebdbf..925b20a 100644 --- a/package.json +++ b/package.json @@ -62,7 +62,7 @@ "karma-webpack": "^1.7.0", "mocha": "^2.3.4", "react": "^0.14.3", - "redux": "^3.0.4", + "redux": "^3.2.1", "redux-devtools": "^3.0.0", "redux-devtools-dock-monitor": "^1.0.1", "redux-devtools-log-monitor": "^1.0.1", diff --git a/src/index.js b/src/index.js index 8c62ec8..01dafad 100644 --- a/src/index.js +++ b/src/index.js @@ -49,19 +49,7 @@ export function syncHistory(history) { history.listen(location => { initialState.location = location })() - function middleware(store) { - unsubscribeHistory = history.listen(location => { - currentKey = location.key - if (syncing) { - // Don't dispatch a new action if we're replaying location. - return - } - - store.dispatch(updateLocation(location)) - }) - - connected = true - + function middleware() { return next => action => { if (action.type !== TRANSITION || !connected) { return next(action) @@ -72,41 +60,72 @@ export function syncHistory(history) { } } - middleware.listenForReplays = - (store, selectLocationState = SELECT_LOCATION) => { - const getLocationState = () => selectLocationState(store.getState()) - const initialLocation = getLocationState() - - unsubscribeStore = store.subscribe(() => { - const location = getLocationState() + middleware.syncWith = + (store, { + urlToState = false, + stateToUrl = false, + selectLocationState = SELECT_LOCATION + } = {}) => { + if (!urlToState && !stateToUrl) { + throw new Error( + 'At least one of "urlToState" and "stateToUrl" options must be true.' + ) + } - // If we're resetting to the beginning, use the saved initial value. We - // need to dispatch a new action at this point to populate the store - // appropriately. - if (location.key === initialLocation.key) { - history.replace(initialLocation) - return + if (stateToUrl) { + const getLocationState = () => selectLocationState(store.getState()) + const initialLocation = getLocationState() + + const reconcileLocationWithState = () => { + const location = getLocationState() + + // If we're resetting to the beginning, use the saved initial value. We + // need to dispatch a new action at this point to populate the store + // appropriately. + if (location.key === initialLocation.key) { + history.replace(initialLocation) + return + } + + // Otherwise, if we need to update the history location, do so without + // dispatching a new action, as we're just bringing history in sync + // with the store. + if (location.key !== currentKey) { + syncing = true + history.transitionTo(location) + syncing = false + } } - // Otherwise, if we need to update the history location, do so without - // dispatching a new action, as we're just bringing history in sync - // with the store. - if (location.key !== currentKey) { - syncing = true - history.transitionTo(location) - syncing = false + unsubscribeStore = store.subscribe(reconcileLocationWithState) + reconcileLocationWithState() + } + + if (urlToState) { + unsubscribeHistory = history.listen(location => { + currentKey = location.key + if (syncing) { + // Don't dispatch a new action if we're replaying location. + return + } + + store.dispatch(updateLocation(location)) + }) + + connected = true + } + + return () => { + if (stateToUrl) { + unsubscribeStore() } - }) - } - middleware.unsubscribe = () => { - unsubscribeHistory() - if (unsubscribeStore) { - unsubscribeStore() + if (urlToState) { + unsubscribeHistory() + connected = false + } + } } - connected = false - } - return middleware } diff --git a/test/createTests.js b/test/createTests.js index dcfbfad..77b73f9 100644 --- a/test/createTests.js +++ b/test/createTests.js @@ -28,15 +28,18 @@ expect.extend({ } }) -function createSyncedHistoryAndStore(createHistory) { +function createSyncedHistoryAndStore(createHistory, syncOptions, initialState) { const history = createHistory() const middleware = syncHistory(history) - const { unsubscribe } = middleware - - const createStoreWithMiddleware = applyMiddleware(middleware)(createStore) - const store = createStoreWithMiddleware(combineReducers({ + const reducer = combineReducers({ routing: routeReducer - })) + }) + const store = createStore( + reducer, + initialState, + applyMiddleware(middleware) + ) + const unsubscribe = middleware.syncWith(store, syncOptions) return { history, store, unsubscribe } } @@ -197,18 +200,17 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) history.push('/foo') const middleware = syncHistory(history) - unsubscribe = middleware.unsubscribe - - const finalCreateStore = compose( + store = createStore(combineReducers({ + routing: routeReducer + }), compose( applyMiddleware(middleware), instrument() - )(createStore) - store = finalCreateStore(combineReducers({ - routing: routeReducer - })) + )) devToolsStore = store.liftedStore - - middleware.listenForReplays(store) + unsubscribe = middleware.syncWith(store, { + stateToUrl: true, + urlToState: true + }) }) afterEach(() => { @@ -273,11 +275,103 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) }) }) + describe('initialState', () => { + it('does not respect initialState when syncing url to state', () => { + let synced = createSyncedHistoryAndStore(createHistory, { + urlToState: true + }, { + routing: { + location: { + pathname: '/init', + search: '', + hash: '', + state: null, + action: 'PUSH', + key: 'abcde' + } + } + }) + + let history = synced.history + let unsubscribe = synced.unsubscribe + + let currentPath + const historyUnsubscribe = history.listen(location => { + currentPath = location.pathname + }) + + expect(currentPath).toEqual('/') + historyUnsubscribe() + unsubscribe() + }) + + it('respects initialState when syncing state to url', () => { + let synced = createSyncedHistoryAndStore(createHistory, { + stateToUrl: true + }, { + routing: { + location: { + pathname: '/init', + search: '', + hash: '', + state: null, + action: 'PUSH', + key: 'abcde' + } + } + }) + + let history = synced.history + let unsubscribe = synced.unsubscribe + + let currentPath + const historyUnsubscribe = history.listen(location => { + currentPath = location.pathname + }) + + expect(currentPath).toEqual('/init') + historyUnsubscribe() + unsubscribe() + }) + + it('respects initialState when syncing both ways', () => { + let synced = createSyncedHistoryAndStore(createHistory, { + stateToUrl: true, + urlToState: true + }, { + routing: { + location: { + pathname: '/init', + search: '', + hash: '', + state: null, + action: 'PUSH', + key: 'abcde' + } + } + }) + + let history = synced.history + let unsubscribe = synced.unsubscribe + + let currentPath + const historyUnsubscribe = history.listen(location => { + currentPath = location.pathname + }) + + expect(currentPath).toEqual('/init') + historyUnsubscribe() + unsubscribe() + }) + }) + describe('syncReduxAndRouter', () => { let history, store, unsubscribe beforeEach(() => { - let synced = createSyncedHistoryAndStore(createHistory) + let synced = createSyncedHistoryAndStore(createHistory, { + urlToState: true + }) history = synced.history store = synced.store unsubscribe = synced.unsubscribe @@ -545,7 +639,9 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) let history, store, unsubscribe beforeEach(() => { - const synced = createSyncedHistoryAndStore(useQueries(createHistory)) + const synced = createSyncedHistoryAndStore(useQueries(createHistory), { + urlToState: true + }) history = synced.history store = synced.store unsubscribe = synced.unsubscribe @@ -583,7 +679,8 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) beforeEach(() => { const synced = createSyncedHistoryAndStore( - () => useBasename(createHistory)({ basename: '/foobar' }) + () => useBasename(createHistory)({ basename: '/foobar' }), + { urlToState: true } ) history = synced.history store = synced.store From 291609107c0176aeb213d37647d2966f85954294 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 3 Feb 2016 14:21:10 +0000 Subject: [PATCH 2/2] Rewrite as a store enhancer --- examples/basic/app.js | 14 ++-- src/index.js | 145 ++++++++++++++++++++++-------------------- test/createTests.js | 68 ++++++++++---------- 3 files changed, 115 insertions(+), 112 deletions(-) diff --git a/examples/basic/app.js b/examples/basic/app.js index 894f352..10eba5d 100644 --- a/examples/basic/app.js +++ b/examples/basic/app.js @@ -4,7 +4,7 @@ import DockMonitor from 'redux-devtools-dock-monitor' import React from 'react' import ReactDOM from 'react-dom' -import { applyMiddleware, compose, createStore, combineReducers } from 'redux' +import { compose, createStore, combineReducers } from 'redux' import { Provider } from 'react-redux' import { Router, Route, IndexRoute } from 'react-router' import createHistory from 'history/lib/createHashHistory' @@ -14,7 +14,10 @@ import * as reducers from './reducers' import { App, Home, Foo, Bar } from './components' const history = createHistory() -const middleware = syncHistory(history) +const enhancer = syncHistory(history, { + urlToState: true, + stateToUrl: true +}) const reducer = combineReducers({ ...reducers, routing: routeReducer @@ -30,16 +33,11 @@ const DevTools = createDevTools( const store = createStore( reducer, compose( - applyMiddleware(middleware), + enhancer, DevTools.instrument() ) ) -middleware.syncWith(store, { - urlToState: true, - stateToUrl: true -}) - ReactDOM.render(
diff --git a/src/index.js b/src/index.js index 01dafad..9c07059 100644 --- a/src/index.js +++ b/src/index.js @@ -43,89 +43,98 @@ export function routeReducer(state = initialState, { type, payload: location }) // Syncing -export function syncHistory(history) { - let unsubscribeHistory, currentKey, unsubscribeStore - let connected = false, syncing = false +export function syncHistory(history, { + urlToState = false, + stateToUrl = false, + selectLocationState = SELECT_LOCATION +} = {}) { + if (!urlToState && !stateToUrl) { + return createStore => createStore + } history.listen(location => { initialState.location = location })() - function middleware() { - return next => action => { - if (action.type !== TRANSITION || !connected) { - return next(action) + let currentKey, syncing = false + let unsubscribeStore, unsubscribeHistory + + const updateStoreOnHistoryChange = (store) => { + unsubscribeHistory = history.listen(location => { + currentKey = location.key + if (syncing) { + // Don't dispatch a new action if we're replaying location. + return } - const { payload: { method, args } } = action - history[method](...args) - } + store.dispatch(updateLocation(location)) + }) } - - middleware.syncWith = - (store, { - urlToState = false, - stateToUrl = false, - selectLocationState = SELECT_LOCATION - } = {}) => { - if (!urlToState && !stateToUrl) { - throw new Error( - 'At least one of "urlToState" and "stateToUrl" options must be true.' - ) + const updateHistoryOnStoreChange = (store) => { + const getLocationState = () => selectLocationState(store.getState()) + const initialLocation = getLocationState() + + const reconcileLocationWithState = () => { + const location = getLocationState() + + // If we're resetting to the beginning, use the saved initial value. We + // need to dispatch a new action at this point to populate the store + // appropriately. + if (location.key === initialLocation.key) { + history.replace(initialLocation) + return } - if (stateToUrl) { - const getLocationState = () => selectLocationState(store.getState()) - const initialLocation = getLocationState() - - const reconcileLocationWithState = () => { - const location = getLocationState() - - // If we're resetting to the beginning, use the saved initial value. We - // need to dispatch a new action at this point to populate the store - // appropriately. - if (location.key === initialLocation.key) { - history.replace(initialLocation) - return - } - - // Otherwise, if we need to update the history location, do so without - // dispatching a new action, as we're just bringing history in sync - // with the store. - if (location.key !== currentKey) { - syncing = true - history.transitionTo(location) - syncing = false - } - } - - unsubscribeStore = store.subscribe(reconcileLocationWithState) - reconcileLocationWithState() + // Otherwise, if we need to update the history location, do so without + // dispatching a new action, as we're just bringing history in sync + // with the store. + if (location.key !== currentKey) { + syncing = true + history.transitionTo(location) + syncing = false } + } - if (urlToState) { - unsubscribeHistory = history.listen(location => { - currentKey = location.key - if (syncing) { - // Don't dispatch a new action if we're replaying location. - return - } + reconcileLocationWithState() + unsubscribeStore = store.subscribe(reconcileLocationWithState) + } - store.dispatch(updateLocation(location)) - }) + const enhancer = createStore => (reducer, initialState, enhancer) => { + const store = createStore(reducer, initialState, enhancer) + const { dispatch: originalDispatch } = store - connected = true + const dispatch = (action) => { + if (action.type !== TRANSITION || !unsubscribeHistory) { + return originalDispatch(action) } - return () => { - if (stateToUrl) { - unsubscribeStore() - } + const { payload: { method, args } } = action + history[method](...args) + } + + if (stateToUrl) { + updateHistoryOnStoreChange(store) + } - if (urlToState) { - unsubscribeHistory() - connected = false - } - } + if (urlToState) { + updateStoreOnHistoryChange(store) } - return middleware + return { + ...store, + dispatch + } + } + + enhancer.dispose = () => { + if (unsubscribeStore) { + unsubscribeStore() + unsubscribeStore = null + } + + if (unsubscribeHistory) { + unsubscribeHistory() + unsubscribeHistory = null + } + } + + return enhancer } diff --git a/test/createTests.js b/test/createTests.js index 77b73f9..94e3129 100644 --- a/test/createTests.js +++ b/test/createTests.js @@ -4,7 +4,7 @@ import expect from 'expect' import { routeActions, TRANSITION, UPDATE_LOCATION, routeReducer, syncHistory } from '../src/index' -import { applyMiddleware, createStore, combineReducers, compose } from 'redux' +import { createStore, combineReducers, compose } from 'redux' import { ActionCreators, instrument } from 'redux-devtools' import { useBasename, useQueries } from 'history' @@ -30,18 +30,14 @@ expect.extend({ function createSyncedHistoryAndStore(createHistory, syncOptions, initialState) { const history = createHistory() - const middleware = syncHistory(history) + const enhancer = syncHistory(history, syncOptions) const reducer = combineReducers({ routing: routeReducer }) - const store = createStore( - reducer, - initialState, - applyMiddleware(middleware) - ) - const unsubscribe = middleware.syncWith(store, syncOptions) - - return { history, store, unsubscribe } + const store = createStore(reducer, initialState, enhancer) + const { dispose } = enhancer + + return { history, store, dispose } } const defaultReset = () => {} @@ -191,7 +187,7 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) // rely directly on the DevTools, as they implement these actions as // middleware, and we don't want to implement this ourselves. describe('devtools', () => { - let history, store, devToolsStore, unsubscribe + let history, store, devToolsStore, dispose beforeEach(() => { history = createHistory() @@ -199,22 +195,22 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) // Set initial URL before syncing history.push('/foo') - const middleware = syncHistory(history) + const enhancer = syncHistory(history, { + stateToUrl: true, + urlToState: true + }) store = createStore(combineReducers({ routing: routeReducer }), compose( - applyMiddleware(middleware), + enhancer, instrument() )) devToolsStore = store.liftedStore - unsubscribe = middleware.syncWith(store, { - stateToUrl: true, - urlToState: true - }) + dispose = enhancer.dispose }) afterEach(() => { - unsubscribe() + dispose() }) it('resets to the initial url', () => { @@ -293,7 +289,7 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) }) let history = synced.history - let unsubscribe = synced.unsubscribe + let dispose = synced.dispose let currentPath const historyUnsubscribe = history.listen(location => { @@ -302,7 +298,7 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) expect(currentPath).toEqual('/') historyUnsubscribe() - unsubscribe() + dispose() }) it('respects initialState when syncing state to url', () => { @@ -322,7 +318,7 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) }) let history = synced.history - let unsubscribe = synced.unsubscribe + let dispose = synced.dispose let currentPath const historyUnsubscribe = history.listen(location => { @@ -331,7 +327,7 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) expect(currentPath).toEqual('/init') historyUnsubscribe() - unsubscribe() + dispose() }) it('respects initialState when syncing both ways', () => { @@ -352,7 +348,7 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) }) let history = synced.history - let unsubscribe = synced.unsubscribe + let dispose = synced.dispose let currentPath const historyUnsubscribe = history.listen(location => { @@ -361,12 +357,12 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) expect(currentPath).toEqual('/init') historyUnsubscribe() - unsubscribe() + dispose() }) }) describe('syncReduxAndRouter', () => { - let history, store, unsubscribe + let history, store, dispose beforeEach(() => { let synced = createSyncedHistoryAndStore(createHistory, { @@ -374,11 +370,11 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) }) history = synced.history store = synced.store - unsubscribe = synced.unsubscribe + dispose = synced.dispose }) afterEach(() => { - unsubscribe() + dispose() }) it('syncs router -> redux', () => { @@ -578,7 +574,7 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) expect(updates).toEqual([ '/', '/bar', '/baz' ]) }) - it('returns unsubscribe to stop listening to history and store', () => { + it('returns dispose to stop listening to history and store', () => { history.push('/foo') expect(store).toContainLocation({ pathname: '/foo' @@ -589,10 +585,10 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) pathname: '/bar' }) - unsubscribe() + dispose() // Make the teardown a no-op. - unsubscribe = () => {} + dispose = () => {} history.push('/foo') expect(store).toContainLocation({ @@ -636,7 +632,7 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) }) describe('query support', () => { - let history, store, unsubscribe + let history, store, dispose beforeEach(() => { const synced = createSyncedHistoryAndStore(useQueries(createHistory), { @@ -644,11 +640,11 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) }) history = synced.history store = synced.store - unsubscribe = synced.unsubscribe + dispose = synced.dispose }) afterEach(() => { - unsubscribe() + dispose() }) it('handles location queries', () => { @@ -675,7 +671,7 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) }) describe('basename support', () => { - let history, store, unsubscribe + let history, store, dispose beforeEach(() => { const synced = createSyncedHistoryAndStore( @@ -684,11 +680,11 @@ module.exports = function createTests(createHistory, name, reset = defaultReset) ) history = synced.history store = synced.store - unsubscribe = synced.unsubscribe + dispose = synced.dispose }) afterEach(() => { - unsubscribe() + dispose() }) it('handles basename history option', () => {