From 54de0070c5fbdd78dbfd9e63be72306d07b963a2 Mon Sep 17 00:00:00 2001 From: James Long Date: Sat, 21 Nov 2015 22:51:54 -0600 Subject: [PATCH 1/3] improve how we sync --- src/index.js | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/src/index.js b/src/index.js index 2799747..78158ef 100644 --- a/src/index.js +++ b/src/index.js @@ -37,7 +37,8 @@ function locationToString(location) { } function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { - let lastRoute; + let isTransitioning = false; + let currentLocation; const getRouterState = () => selectRouterState(store.getState()); if(!getRouterState()) { @@ -48,11 +49,12 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { } const unsubscribeHistory = history.listen(location => { - const newLocation = locationToString(location); + currentLocation = location; + isTransitioning = false; + // 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(getRouterState().path !== newLocation) { - lastRoute = newLocation; + if(getRouterState().path !== locationToString(location)) { store.dispatch(updatePath(newLocation)); } }); @@ -60,14 +62,18 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { const unsubscribeStore = store.subscribe(() => { const routing = getRouterState(); - // Don't update the router if the routing state hasn't changed or the new routing path - // is already the current location. - // The `noRouterUpdate` flag can be set to avoid updating altogether, - // which is useful for things like loading snapshots or very special - // edge cases. - if(lastRoute !== routing.path && routing.path !== locationToString(window.location) && + // Don't update the router if they are already in sync, or if + // we've already triggered an update for this path. The latter can + // happen if any state changes happen during transitions (for + // example: updating app state during `listenBefore`). + // + // The `noRouterUpdate` flag can be set to avoid updating + // altogether, which is useful for things like loading snapshots + // or very special edge cases. + if(!isTransitioning && + routing.path !== locationToString(currentLocation) && !routing.noRouterUpdate) { - lastRoute = routing.path; + isTransitioning = true; history.pushState(null, routing.path); } }); From 4557515fd848da66310cf1078e3da3aefc400563 Mon Sep 17 00:00:00 2001 From: James Long Date: Sat, 21 Nov 2015 23:29:33 -0600 Subject: [PATCH 2/3] simplify how we track when to tell the router to update --- src/index.js | 39 +++++++++++++++------------------------ 1 file changed, 15 insertions(+), 24 deletions(-) diff --git a/src/index.js b/src/index.js index 78158ef..6a31e6c 100644 --- a/src/index.js +++ b/src/index.js @@ -6,25 +6,26 @@ const SELECT_STATE = state => state.routing; // Action creator -function updatePath(path, noRouterUpdate) { +function updatePath(path, avoidRouterUpdate) { return { type: UPDATE_PATH, path: path, - noRouterUpdate: noRouterUpdate + avoidRouterUpdate: !!avoidRouterUpdate } } // Reducer const initialState = typeof window === 'undefined' ? {} : { - path: locationToString(window.location) + path: locationToString(window.location), + changeId: 1 }; function update(state=initialState, action) { if(action.type === UPDATE_PATH) { return Object.assign({}, state, { path: action.path, - noRouterUpdate: action.noRouterUpdate + changeId: state.changeId + (action.avoidRouterUpdate ? 0 : 1) }); } return state; @@ -37,8 +38,7 @@ function locationToString(location) { } function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { - let isTransitioning = false; - let currentLocation; + let lastChangeId = 0; const getRouterState = () => selectRouterState(store.getState()); if(!getRouterState()) { @@ -49,31 +49,22 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { } const unsubscribeHistory = history.listen(location => { - currentLocation = location; - isTransitioning = false; + const routePath = locationToString(location); - // 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(getRouterState().path !== locationToString(location)) { - store.dispatch(updatePath(newLocation)); + // Avoid dispatching an action if the store is already up-to-date + if(getRouterState().path !== routePath) { + store.dispatch(updatePath(routePath, { avoidRouterUpdate: true })); } }); const unsubscribeStore = store.subscribe(() => { const routing = getRouterState(); - // Don't update the router if they are already in sync, or if - // we've already triggered an update for this path. The latter can - // happen if any state changes happen during transitions (for - // example: updating app state during `listenBefore`). - // - // The `noRouterUpdate` flag can be set to avoid updating - // altogether, which is useful for things like loading snapshots - // or very special edge cases. - if(!isTransitioning && - routing.path !== locationToString(currentLocation) && - !routing.noRouterUpdate) { - isTransitioning = true; + // Only update the router once per `updatePath` 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); } }); From c72dd571884a18b64a05262a36843852d74eb608 Mon Sep 17 00:00:00 2001 From: James Long Date: Tue, 24 Nov 2015 12:32:20 -0500 Subject: [PATCH 3/3] add a testing suite --- .babelrc | 3 + package.json | 10 ++- src/index.js | 10 +-- test/index.js | 187 ++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 204 insertions(+), 6 deletions(-) create mode 100644 .babelrc create mode 100644 test/index.js diff --git a/.babelrc b/.babelrc new file mode 100644 index 0000000..dc1bc4f --- /dev/null +++ b/.babelrc @@ -0,0 +1,3 @@ +{ + "presets": ["es2015"] +} \ No newline at end of file diff --git a/package.json b/package.json index a941e87..47cae02 100644 --- a/package.json +++ b/package.json @@ -12,7 +12,8 @@ ], "license": "MIT", "scripts": { - "build": "mkdir -p lib && babel ./src/index.js --plugins transform-object-assign --presets babel-preset-es2015 --out-file ./lib/index.js", + "build": "mkdir -p lib && babel ./src/index.js --plugins transform-object-assign --out-file ./lib/index.js", + "test": "mocha --compilers js:babel-core/register --recursive", "prepublish": "npm run build" }, "tags": [ @@ -26,7 +27,12 @@ ], "devDependencies": { "babel-cli": "^6.1.2", + "babel-core": "^6.2.1", "babel-plugin-transform-object-assign": "^6.0.14", - "babel-preset-es2015": "^6.1.2" + "babel-preset-es2015": "^6.1.2", + "expect": "^1.13.0", + "history": "^1.13.1", + "mocha": "^2.3.4", + "redux": "^3.0.4" } } diff --git a/src/index.js b/src/index.js index 6a31e6c..31104d3 100644 --- a/src/index.js +++ b/src/index.js @@ -16,9 +16,11 @@ function updatePath(path, avoidRouterUpdate) { // Reducer -const initialState = typeof window === 'undefined' ? {} : { - path: locationToString(window.location), - changeId: 1 +const initialState = { + changeId: 1, + path: (typeof window !== 'undefined') ? + locationToString(window.location) : + '/' }; function update(state=initialState, action) { @@ -38,8 +40,8 @@ function locationToString(location) { } function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { - let lastChangeId = 0; const getRouterState = () => selectRouterState(store.getState()); + let lastChangeId = 0; if(!getRouterState()) { throw new Error( diff --git a/test/index.js b/test/index.js new file mode 100644 index 0000000..2aa051c --- /dev/null +++ b/test/index.js @@ -0,0 +1,187 @@ +const expect = require('expect'); +const { updatePath, UPDATE_PATH, routeReducer, syncReduxAndRouter } = require('../src/index'); +const { createStore, combineReducers } = require('redux'); +const { createMemoryHistory: createHistory } = require('history'); + +function createSyncedHistoryAndStore() { + const store = createStore(combineReducers({ + routing: routeReducer + })); + const history = createHistory(); + syncReduxAndRouter(history, store); + return { history, store }; +} + +describe('updatePath', () => { + it('creates actions', () => { + expect(updatePath('/foo')).toEqual({ + type: UPDATE_PATH, + path: '/foo', + avoidRouterUpdate: false + }); + + expect(updatePath('/foo', { avoidRouterUpdate: true })).toEqual({ + type: UPDATE_PATH, + path: '/foo', + avoidRouterUpdate: true + }); + }); +}); + +describe('routeReducer', () => { + const state = { + path: '/foo', + changeId: 1 + }; + + it('updates the path', () => { + expect(routeReducer(state, { + type: UPDATE_PATH, + path: '/bar' + })).toEqual({ + path: '/bar', + changeId: 2 + }); + }); + + it('respects `avoidRouterUpdate` flag', () => { + expect(routeReducer(state, { + type: UPDATE_PATH, + path: '/bar', + avoidRouterUpdate: true + })).toEqual({ + path: '/bar', + changeId: 1 + }); + }); +}); + +describe('syncReduxAndRouter', () => { + it('syncs router -> redux', () => { + const { history, store } = createSyncedHistoryAndStore(); + expect(store.getState().routing.path).toEqual('/'); + + history.pushState(null, '/foo'); + expect(store.getState().routing.path).toEqual('/foo'); + + history.pushState(null, '/bar'); + expect(store.getState().routing.path).toEqual('/bar'); + + history.pushState(null, '/bar?query=1'); + expect(store.getState().routing.path).toEqual('/bar?query=1'); + + history.pushState(null, '/bar?query=1#hash=2'); + expect(store.getState().routing.path).toEqual('/bar?query=1#hash=2'); + }); + + it('syncs redux -> router', () => { + const { history, store } = createSyncedHistoryAndStore(); + expect(store.getState().routing).toEqual({ + path: '/', + changeId: 1 + }); + + store.dispatch(updatePath('/foo')); + expect(store.getState().routing).toEqual({ + path: '/foo', + changeId: 2 + }); + + store.dispatch(updatePath('/bar')); + expect(store.getState().routing).toEqual({ + path: '/bar', + changeId: 3 + }); + + store.dispatch(updatePath('/bar?query=1')); + expect(store.getState().routing).toEqual({ + path: '/bar?query=1', + changeId: 4 + }); + + store.dispatch(updatePath('/bar?query=1#hash=2')); + expect(store.getState().routing).toEqual({ + path: '/bar?query=1#hash=2', + changeId: 5 + }); + }); + + it('updates the router even if path is the same', () => { + const { history, store } = createSyncedHistoryAndStore(); + expect(store.getState().routing).toEqual({ + path: '/', + changeId: 1 + }); + + store.dispatch(updatePath('/foo')); + expect(store.getState().routing).toEqual({ + path: '/foo', + changeId: 2 + }); + + store.dispatch(updatePath('/foo')); + expect(store.getState().routing).toEqual({ + path: '/foo', + changeId: 3 + }); + }); + + it('does not update the router for other state changes', () => { + const { history, store } = createSyncedHistoryAndStore(); + store.dispatch({ + type: 'RANDOM_ACTION', + value: 5 + }); + + expect(store.getState().routing).toEqual({ + path: '/', + changeId: 1 + }); + }); + + it('only updates the router once when dispatching from `listenBefore`', () => { + const { history, store } = createSyncedHistoryAndStore(); + expect(store.getState().routing).toEqual({ + path: '/', + changeId: 1 + }); + + history.listenBefore(location => { + expect(location.pathname).toEqual('/foo'); + store.dispatch({ + type: 'RANDOM_ACTION', + value: 5 + }); + }); + + store.dispatch(updatePath('/foo')); + expect(store.getState().routing).toEqual({ + path: '/foo', + changeId: 2 + }); + }); + + it('allows updating the route from within `listenBefore`', () => { + const { history, store } = createSyncedHistoryAndStore(); + expect(store.getState().routing).toEqual({ + path: '/', + changeId: 1 + }); + + history.listenBefore(location => { + if(location.pathname === '/foo') { + expect(store.getState().routing).toEqual({ + path: '/foo', + changeId: 2 + }); + store.dispatch(updatePath('/bar')); + } + }); + + store.dispatch(updatePath('/foo')); + expect(store.getState().routing).toEqual({ + path: '/bar', + changeId: 3 + }); + }) +});