From 5e41803298114438a6f7e6623f01ee85c7845ca4 Mon Sep 17 00:00:00 2001 From: Kim Joar Bekkelund Date: Thu, 19 Nov 2015 08:32:10 -0800 Subject: [PATCH 1/6] Handle routing state and replace --- package.json | 3 ++ src/index.js | 67 +++++++++++++++++-------- test/index.js | 136 ++++++++++++++++++++++++++++++++++++++------------ 3 files changed, 155 insertions(+), 51 deletions(-) diff --git a/package.json b/package.json index 6f99485..75ace7b 100644 --- a/package.json +++ b/package.json @@ -36,5 +36,8 @@ "isparta": "^4.0.0", "mocha": "^2.3.4", "redux": "^3.0.4" + }, + "dependencies": { + "deep-equal": "^1.0.1" } } diff --git a/src/index.js b/src/index.js index 31104d3..081f2f9 100644 --- a/src/index.js +++ b/src/index.js @@ -1,15 +1,28 @@ +const deepEqual = require('deep-equal'); -// constants +// Constants const UPDATE_PATH = "@@router/UPDATE_PATH"; const SELECT_STATE = state => state.routing; // Action creator -function updatePath(path, avoidRouterUpdate) { +function pushPath(path, state, avoidRouterUpdate) { return { type: UPDATE_PATH, path: path, + state: state, + replace: false, + avoidRouterUpdate: !!avoidRouterUpdate + }; +} + +function replacePath(path, state, avoidRouterUpdate) { + return { + type: UPDATE_PATH, + path: path, + state: state, + replace: true, avoidRouterUpdate: !!avoidRouterUpdate } } @@ -18,16 +31,18 @@ function updatePath(path, avoidRouterUpdate) { const initialState = { changeId: 1, - path: (typeof window !== 'undefined') ? - locationToString(window.location) : - '/' + path: undefined, + state: undefined, + replace: false }; function update(state=initialState, action) { if(action.type === UPDATE_PATH) { return Object.assign({}, state, { path: action.path, - changeId: state.changeId + (action.avoidRouterUpdate ? 0 : 1) + changeId: state.changeId + (action.avoidRouterUpdate ? 0 : 1), + state: action.state, + replace: action.replace }); } return state; @@ -35,8 +50,8 @@ function update(state=initialState, action) { // Syncing -function locationToString(location) { - return location.pathname + location.search + location.hash; +function locationsAreEqual(a, b) { + return a.path === b.path && deepEqual(a.state, b.state); } function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { @@ -50,25 +65,36 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { ); } + let historyLocation = {}; + const unsubscribeHistory = history.listen(location => { - const routePath = locationToString(location); + historyLocation = { + path: history.createPath(location), + state: location.state + }; + + const routing = getRouterState(); + + // Avoid dispatching an action if the store is already up-to-date, + // even if `history` wouldn't do anything if the location is the same + if(locationsAreEqual(routing, historyLocation)) return; - // Avoid dispatching an action if the store is already up-to-date - if(getRouterState().path !== routePath) { - store.dispatch(updatePath(routePath, { avoidRouterUpdate: true })); - } + store.dispatch(pushPath(historyLocation.path, historyLocation.state, { avoidRouterUpdate: true })); }); const unsubscribeStore = store.subscribe(() => { const routing = getRouterState(); - // Only update the router once per `updatePath` call. This is + // Only update the router once per `pushPath` call. This is // indicated by the `changeId` state; when that number changes, we - // should call `pushState`. - if(lastChangeId !== routing.changeId) { - lastChangeId = routing.changeId; - history.pushState(null, routing.path); - } + // should update the history. + if(lastChangeId === routing.changeId) return; + + lastChangeId = routing.changeId; + + const method = routing.replace ? 'replaceState' : 'pushState'; + + history[method](routing.state, routing.path); }); return function unsubscribe() { @@ -79,7 +105,8 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { module.exports = { UPDATE_PATH, - updatePath, + pushPath, + replacePath, syncReduxAndRouter, routeReducer: update }; diff --git a/test/index.js b/test/index.js index 1981f2f..72825d6 100644 --- a/test/index.js +++ b/test/index.js @@ -1,5 +1,5 @@ const expect = require('expect'); -const { updatePath, UPDATE_PATH, routeReducer, syncReduxAndRouter } = require('../src/index'); +const { pushPath, replacePath, UPDATE_PATH, routeReducer, syncReduxAndRouter } = require('../src/index'); const { createStore, combineReducers } = require('redux'); const { createMemoryHistory: createHistory } = require('history'); @@ -12,17 +12,41 @@ function createSyncedHistoryAndStore() { return { history, store }; } -describe('updatePath', () => { +describe('pushPath', () => { it('creates actions', () => { - expect(updatePath('/foo')).toEqual({ + expect(pushPath('/foo', { bar: 'baz' })).toEqual({ type: UPDATE_PATH, path: '/foo', + replace: false, + state: { bar: 'baz' }, avoidRouterUpdate: false }); - expect(updatePath('/foo', { avoidRouterUpdate: true })).toEqual({ + expect(pushPath('/foo', undefined, { avoidRouterUpdate: true })).toEqual({ type: UPDATE_PATH, path: '/foo', + state: undefined, + replace: false, + avoidRouterUpdate: true + }); + }); +}); + +describe('replacePath', () => { + it('creates actions', () => { + expect(replacePath('/foo', { bar: 'baz' })).toEqual({ + type: UPDATE_PATH, + path: '/foo', + replace: true, + state: { bar: 'baz' }, + avoidRouterUpdate: false + }); + + expect(replacePath('/foo', undefined, { avoidRouterUpdate: true })).toEqual({ + type: UPDATE_PATH, + path: '/foo', + state: undefined, + replace: true, avoidRouterUpdate: true }); }); @@ -37,9 +61,12 @@ describe('routeReducer', () => { it('updates the path', () => { expect(routeReducer(state, { type: UPDATE_PATH, - path: '/bar' + path: '/bar', + replace: false })).toEqual({ path: '/bar', + replace: false, + state: undefined, changeId: 2 }); }); @@ -48,9 +75,12 @@ describe('routeReducer', () => { expect(routeReducer(state, { type: UPDATE_PATH, path: '/bar', + replace: false, avoidRouterUpdate: true })).toEqual({ path: '/bar', + replace: false, + state: undefined, changeId: 1 }); }); @@ -63,9 +93,15 @@ describe('syncReduxAndRouter', () => { history.pushState(null, '/foo'); expect(store.getState().routing.path).toEqual('/foo'); + expect(store.getState().routing.state).toBe(null); + + history.pushState({ bar: 'baz' }, '/foo'); + expect(store.getState().routing.path).toEqual('/foo'); + expect(store.getState().routing.state).toEqual({ bar: 'baz' }); history.pushState(null, '/bar'); expect(store.getState().routing.path).toEqual('/bar'); + expect(store.getState().routing.state).toBe(null); history.pushState(null, '/bar?query=1'); expect(store.getState().routing.path).toEqual('/bar?query=1'); @@ -78,31 +114,49 @@ describe('syncReduxAndRouter', () => { const { history, store } = createSyncedHistoryAndStore(); expect(store.getState().routing).toEqual({ path: '/', - changeId: 1 + changeId: 1, + replace: false, + state: undefined }); - store.dispatch(updatePath('/foo')); + store.dispatch(pushPath('/foo')); expect(store.getState().routing).toEqual({ path: '/foo', - changeId: 2 + changeId: 2, + replace: false, + state: undefined + }); + + store.dispatch(pushPath('/foo', { bar: 'baz' })); + expect(store.getState().routing).toEqual({ + path: '/foo', + changeId: 3, + replace: false, + state: { bar: 'baz' } }); - store.dispatch(updatePath('/bar')); + store.dispatch(pushPath('/bar')); expect(store.getState().routing).toEqual({ path: '/bar', - changeId: 3 + changeId: 4, + replace: false, + state: undefined }); - store.dispatch(updatePath('/bar?query=1')); + store.dispatch(pushPath('/bar?query=1')); expect(store.getState().routing).toEqual({ path: '/bar?query=1', - changeId: 4 + changeId: 5, + replace: false, + state: undefined }); - store.dispatch(updatePath('/bar?query=1#hash=2')); + store.dispatch(pushPath('/bar?query=1#hash=2')); expect(store.getState().routing).toEqual({ path: '/bar?query=1#hash=2', - changeId: 5 + changeId: 6, + replace: false, + state: undefined }); }); @@ -110,19 +164,25 @@ describe('syncReduxAndRouter', () => { const { history, store } = createSyncedHistoryAndStore(); expect(store.getState().routing).toEqual({ path: '/', - changeId: 1 + changeId: 1, + replace: false, + state: undefined }); - store.dispatch(updatePath('/foo')); + store.dispatch(pushPath('/foo')); expect(store.getState().routing).toEqual({ path: '/foo', - changeId: 2 + changeId: 2, + replace: false, + state: undefined }); - store.dispatch(updatePath('/foo')); + store.dispatch(pushPath('/foo')); expect(store.getState().routing).toEqual({ path: '/foo', - changeId: 3 + changeId: 3, + replace: false, + state: undefined }); }); @@ -135,7 +195,9 @@ describe('syncReduxAndRouter', () => { expect(store.getState().routing).toEqual({ path: '/', - changeId: 1 + changeId: 1, + replace: false, + state: undefined }); }); @@ -143,7 +205,9 @@ describe('syncReduxAndRouter', () => { const { history, store } = createSyncedHistoryAndStore(); expect(store.getState().routing).toEqual({ path: '/', - changeId: 1 + changeId: 1, + replace: false, + state: undefined }); history.listenBefore(location => { @@ -154,10 +218,12 @@ describe('syncReduxAndRouter', () => { }); }); - store.dispatch(updatePath('/foo')); + store.dispatch(pushPath('/foo')); expect(store.getState().routing).toEqual({ path: '/foo', - changeId: 2 + changeId: 2, + replace: false, + state: undefined }); }); @@ -165,23 +231,29 @@ describe('syncReduxAndRouter', () => { const { history, store } = createSyncedHistoryAndStore(); expect(store.getState().routing).toEqual({ path: '/', - changeId: 1 + changeId: 1, + replace: false, + state: undefined }); history.listenBefore(location => { if(location.pathname === '/foo') { expect(store.getState().routing).toEqual({ path: '/foo', - changeId: 2 + changeId: 2, + replace: false, + state: undefined }); - store.dispatch(updatePath('/bar')); + store.dispatch(pushPath('/bar')); } }); - store.dispatch(updatePath('/foo')); + store.dispatch(pushPath('/foo')); expect(store.getState().routing).toEqual({ path: '/bar', - changeId: 3 + changeId: 3, + replace: false, + state: undefined }); }) @@ -215,10 +287,12 @@ describe('syncReduxAndRouter', () => { history.pushState(null, '/foo'); expect(store.getState().routing.path).toEqual('/foo'); - store.dispatch(updatePath('/bar')); + store.dispatch(pushPath('/bar')); expect(store.getState().routing).toEqual({ path: '/bar', - changeId: 2 + changeId: 2, + replace: false, + state: undefined }); unsubscribe(); @@ -230,7 +304,7 @@ describe('syncReduxAndRouter', () => { throw new Error() }); expect( - () => store.dispatch(updatePath('/foo')) + () => store.dispatch(pushPath('/foo')) ).toNotThrow(); }); }); From b7f10aa019adbf6eb089f780fe329a7952f4a926 Mon Sep 17 00:00:00 2001 From: Kim Joar Bekkelund Date: Wed, 25 Nov 2015 11:09:17 -0800 Subject: [PATCH 2/6] Better test coverage --- test/index.js | 61 ++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 3 deletions(-) diff --git a/test/index.js b/test/index.js index 72825d6..ad98bda 100644 --- a/test/index.js +++ b/test/index.js @@ -71,6 +71,20 @@ describe('routeReducer', () => { }); }); + it('respects replace', () => { + expect(routeReducer(state, { + type: UPDATE_PATH, + path: '/bar', + replace: true, + avoidRouterUpdate: false + })).toEqual({ + path: '/bar', + replace: true, + state: undefined, + changeId: 2 + }); + }); + it('respects `avoidRouterUpdate` flag', () => { expect(routeReducer(state, { type: UPDATE_PATH, @@ -99,6 +113,10 @@ describe('syncReduxAndRouter', () => { expect(store.getState().routing.path).toEqual('/foo'); expect(store.getState().routing.state).toEqual({ bar: 'baz' }); + history.replaceState(null, '/bar'); + expect(store.getState().routing.path).toEqual('/bar'); + expect(store.getState().routing.state).toBe(null); + history.pushState(null, '/bar'); expect(store.getState().routing.path).toEqual('/bar'); expect(store.getState().routing.state).toBe(null); @@ -106,6 +124,10 @@ describe('syncReduxAndRouter', () => { history.pushState(null, '/bar?query=1'); expect(store.getState().routing.path).toEqual('/bar?query=1'); + history.replaceState({ bar: 'baz' }, '/bar?query=1'); + expect(store.getState().routing.path).toEqual('/bar?query=1'); + expect(store.getState().routing.state).toEqual({ bar: 'baz' }); + history.pushState(null, '/bar?query=1#hash=2'); expect(store.getState().routing.path).toEqual('/bar?query=1#hash=2'); }); @@ -135,10 +157,18 @@ describe('syncReduxAndRouter', () => { state: { bar: 'baz' } }); - store.dispatch(pushPath('/bar')); + store.dispatch(replacePath('/bar', { bar: 'foo' })); expect(store.getState().routing).toEqual({ path: '/bar', changeId: 4, + replace: true, + state: { bar: 'foo' } + }); + + store.dispatch(pushPath('/bar')); + expect(store.getState().routing).toEqual({ + path: '/bar', + changeId: 5, replace: false, state: undefined }); @@ -146,7 +176,7 @@ describe('syncReduxAndRouter', () => { store.dispatch(pushPath('/bar?query=1')); expect(store.getState().routing).toEqual({ path: '/bar?query=1', - changeId: 5, + changeId: 6, replace: false, state: undefined }); @@ -154,7 +184,7 @@ describe('syncReduxAndRouter', () => { store.dispatch(pushPath('/bar?query=1#hash=2')); expect(store.getState().routing).toEqual({ path: '/bar?query=1#hash=2', - changeId: 6, + changeId: 7, replace: false, state: undefined }); @@ -184,6 +214,14 @@ describe('syncReduxAndRouter', () => { replace: false, state: undefined }); + + store.dispatch(replacePath('/foo')); + expect(store.getState().routing).toEqual({ + path: '/foo', + changeId: 4, + replace: true, + state: undefined + }); }); it('does not update the router for other state changes', () => { @@ -246,6 +284,15 @@ describe('syncReduxAndRouter', () => { }); store.dispatch(pushPath('/bar')); } + else if(location.pathname === '/replace') { + expect(store.getState().routing).toEqual({ + path: '/replace', + changeId: 4, + replace: false, + state: { bar: 'baz' } + }); + store.dispatch(replacePath('/baz', { foo: 'bar' })); + } }); store.dispatch(pushPath('/foo')); @@ -255,6 +302,14 @@ describe('syncReduxAndRouter', () => { replace: false, state: undefined }); + + store.dispatch(pushPath('/replace', { bar: 'baz' })); + expect(store.getState().routing).toEqual({ + path: '/baz', + changeId: 5, + replace: true, + state: { foo: 'bar' } + }); }) it('throws if "routing" key is missing with default selectRouteState', () => { From a0d6945f94f3ca1ec45d180f43a8c8ccbbab4fac Mon Sep 17 00:00:00 2001 From: Kim Joar Bekkelund Date: Wed, 25 Nov 2015 11:15:32 -0800 Subject: [PATCH 3/6] Fix avoidRouterUpdate bug --- src/index.js | 10 ++++++---- test/index.js | 8 ++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/src/index.js b/src/index.js index 081f2f9..a784a81 100644 --- a/src/index.js +++ b/src/index.js @@ -7,23 +7,25 @@ const SELECT_STATE = state => state.routing; // Action creator -function pushPath(path, state, avoidRouterUpdate) { +function pushPath(path, state, opts) { + opts = opts || {}; return { type: UPDATE_PATH, path: path, state: state, replace: false, - avoidRouterUpdate: !!avoidRouterUpdate + avoidRouterUpdate: !!opts.avoidRouterUpdate }; } -function replacePath(path, state, avoidRouterUpdate) { +function replacePath(path, state, opts) { + opts = opts || {}; return { type: UPDATE_PATH, path: path, state: state, replace: true, - avoidRouterUpdate: !!avoidRouterUpdate + avoidRouterUpdate: !!opts.avoidRouterUpdate } } diff --git a/test/index.js b/test/index.js index ad98bda..3e36ef5 100644 --- a/test/index.js +++ b/test/index.js @@ -49,6 +49,14 @@ describe('replacePath', () => { replace: true, avoidRouterUpdate: true }); + + expect(replacePath('/foo', undefined, { avoidRouterUpdate: false })).toEqual({ + type: UPDATE_PATH, + path: '/foo', + state: undefined, + replace: true, + avoidRouterUpdate: false + }); }); }); From be81be686afb99c7bee621d828e51f0dfbcffb04 Mon Sep 17 00:00:00 2001 From: Kim Joar Bekkelund Date: Wed, 25 Nov 2015 11:29:03 -0800 Subject: [PATCH 4/6] Move unnecessary let into const --- src/index.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/index.js b/src/index.js index a784a81..305ee85 100644 --- a/src/index.js +++ b/src/index.js @@ -67,10 +67,8 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { ); } - let historyLocation = {}; - const unsubscribeHistory = history.listen(location => { - historyLocation = { + const historyLocation = { path: history.createPath(location), state: location.state }; From 3581f20799de44967a9ce82ed6a1c549dbf3fa85 Mon Sep 17 00:00:00 2001 From: Kim Joar Bekkelund Date: Wed, 25 Nov 2015 11:32:17 -0800 Subject: [PATCH 5/6] Better naming --- src/index.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/index.js b/src/index.js index 305ee85..3401b8a 100644 --- a/src/index.js +++ b/src/index.js @@ -68,18 +68,16 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { } const unsubscribeHistory = history.listen(location => { - const historyLocation = { + const route = { path: history.createPath(location), state: location.state }; - const routing = getRouterState(); - // Avoid dispatching an action if the store is already up-to-date, // even if `history` wouldn't do anything if the location is the same - if(locationsAreEqual(routing, historyLocation)) return; + if(locationsAreEqual(getRouterState(), route)) return; - store.dispatch(pushPath(historyLocation.path, historyLocation.state, { avoidRouterUpdate: true })); + store.dispatch(pushPath(route.path, route.state, { avoidRouterUpdate: true })); }); const unsubscribeStore = store.subscribe(() => { From 2a65890e0141320d634bdf29207be8cd2cbe163c Mon Sep 17 00:00:00 2001 From: Kim Joar Bekkelund Date: Wed, 25 Nov 2015 12:07:37 -0800 Subject: [PATCH 6/6] Add test that checks store updates --- test/index.js | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/test/index.js b/test/index.js index 3e36ef5..efefe13 100644 --- a/test/index.js +++ b/test/index.js @@ -273,6 +273,67 @@ describe('syncReduxAndRouter', () => { }); }); + it('does not unnecessarily update the store', () => { + const { history, store } = createSyncedHistoryAndStore(); + const updates = []; + + const unsubscribe = store.subscribe(() => { + updates.push(store.getState()) + }); + + store.dispatch(pushPath('/foo')); + store.dispatch(pushPath('/foo')); + store.dispatch(pushPath('/foo', { bar: 'baz' })); + store.dispatch(replacePath('/bar')); + store.dispatch(replacePath('/bar', { bar: 'foo' })); + + unsubscribe(); + + expect(updates.length).toBe(5); + expect(updates).toEqual([ + { + routing: { + changeId: 2, + path: '/foo', + state: undefined, + replace: false + } + }, + { + routing: { + changeId: 3, + path: '/foo', + state: undefined, + replace: false + } + }, + { + routing: { + changeId: 4, + path: '/foo', + state: { bar: 'baz' }, + replace: false + } + }, + { + routing: { + changeId: 5, + path: '/bar', + state: undefined, + replace: true + } + }, + { + routing: { + changeId: 6, + path: '/bar', + state: { bar: 'foo' }, + replace: true + } + } + ]); + }); + it('allows updating the route from within `listenBefore`', () => { const { history, store } = createSyncedHistoryAndStore(); expect(store.getState().routing).toEqual({