Skip to content
This repository has been archived by the owner on Oct 26, 2018. It is now read-only.

Get devtools working #73

Merged
merged 3 commits into from
Dec 6, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
"karma-webpack": "^1.7.0",
"mocha": "^2.3.4",
"redux": "^3.0.4",
"redux-devtools": "^2.1.5",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this dev dependency? It should only be a dependency in the examples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rely on redux-devtools in the tests to ensure things work as expected, otherwise reset is difficult to test (kimjoar@4091b1e#diff-413f2628e9c93c46cec6db202ddb8ea4R165). If we only dispatch the initial state as payload it will become a new object and won't be === initialState (kimjoar@fb1508c#diff-1fdf421c05c1140f6d71444ea2b27638R111)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can of course deep-equal check with initial state instead. Then we could remove devtools as a dep

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh right, I forgot about the tests. I don't really understand those tests (haven't dug into how the devtools works), but it seems cool to me to do that.

"webpack": "^1.12.9"
},
"dependencies": {
Expand Down
42 changes: 34 additions & 8 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,26 @@ function update(state=initialState, { type, payload }) {
// Syncing

function locationsAreEqual(a, b) {
return a.path === b.path && deepEqual(a.state, b.state);
return a != null && b != null && a.path === b.path && deepEqual(a.state, b.state);
}

function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) {
const getRouterState = () => selectRouterState(store.getState());
let lastChangeId = 0;

// Because we're not able to set the initial path in `initialState` we need a
// "hack" to get "Revert" in Redux DevTools to work. We solve this by keeping
// the first route so we can revert to this route when the initial state is
// replayed to reset the state. Basically, we treat the first route as our
// initial state.
let firstRoute = undefined;

// To properly handle store updates we need to track the last route. This
// route contains a `changeId` which is updated on every `pushPath` and
// `replacePath`. If this id changes we always trigger a history update.
// However, if the id does not change, we check if the location has changed,
// and if it is we trigger a history update. (If these are out of sync it's
// likely because of React DevTools.)
let lastRoute = undefined;

if(!getRouterState()) {
throw new Error(
Expand All @@ -75,6 +89,10 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) {
state: location.state
};

if (firstRoute === undefined) {
firstRoute = route;
}

// 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(getRouterState(), route)) return;
Expand All @@ -87,14 +105,22 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) {
});

const unsubscribeStore = store.subscribe(() => {
const routing = getRouterState();
let routing = getRouterState();

// Only update the router once per `pushPath` call. This is
// indicated by the `changeId` state; when that number changes, we
// should update the history.
if(lastChangeId === routing.changeId) return;
// Treat `firstRoute` as our `initialState`
if(routing === initialState) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll merge this like it is, but I would like to keep thinking about this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. It's by no means optimal, it was just the easiest way I could get devtools to work as expected :/ The problem is of course that we can't read the initial location synchronously at the beginning.

routing = firstRoute;
}

// Only trigger history update is this is a new change or the location
// has changed.
if(lastRoute !== undefined &&
lastRoute.changeId === routing.changeId &&
locationsAreEqual(lastRoute, routing)) {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really prefer to avoid early returns. I'm fine merging this and changing it after if you want, but generally it makes the code flow clearer to avoid them.

}

lastChangeId = routing.changeId;
lastRoute = routing;

const method = routing.replace ? 'replaceState' : 'pushState';

Expand Down
75 changes: 74 additions & 1 deletion test/createTests.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
const expect = require('expect');
const { pushPath, replacePath, UPDATE_PATH, routeReducer, syncReduxAndRouter } = require('../src/index');
const { createStore, combineReducers } = require('redux');
const { createStore, combineReducers, compose } = require('redux');
const { devTools } = require('redux-devtools');
const { ActionCreators } = require('redux-devtools/lib/devTools');

function createSyncedHistoryAndStore(createHistory) {
const store = createStore(combineReducers({
Expand Down Expand Up @@ -130,6 +132,77 @@ module.exports = function createTests(createHistory, name, reset = defaultReset)
});
});

describe('devtools', () => {
let history, store, devToolsStore, unsubscribe;

beforeEach(() => {
history = createHistory();
const finalCreateStore = compose(devTools())(createStore);
store = finalCreateStore(combineReducers({
routing: routeReducer
}));
devToolsStore = store.devToolsStore;

// Set initial URL before syncing
history.pushState(null, '/foo');

unsubscribe = syncReduxAndRouter(history, store);
});

afterEach(() => {
unsubscribe();
});

it('resets to the initial url', () => {
let lastPath;
const historyUnsubscribe = history.listen(location => {
lastPath = location.pathname;
});

history.pushState(null, '/bar');
store.dispatch(pushPath('/baz'));

devToolsStore.dispatch(ActionCreators.reset());

expect(store.getState().routing.path).toEqual('/foo');
expect(lastPath).toEqual('/foo');
});

it('handles toggle after store change', () => {
let lastPath;
const historyUnsubscribe = history.listen(location => {
lastPath = location.pathname;
});

// action 2
history.pushState(null, '/foo2');
// action 3
history.pushState(null, '/foo3');

devToolsStore.dispatch(ActionCreators.toggleAction(3));
expect(lastPath).toEqual('/foo2');

historyUnsubscribe();
});

it('handles toggle after store change', () => {
let lastPath;
const historyUnsubscribe = history.listen(location => {
lastPath = location.pathname;
});

// action 2
store.dispatch(pushPath('/foo2'));
// action 3
store.dispatch(pushPath('/foo3'));

devToolsStore.dispatch(ActionCreators.toggleAction(3));
expect(lastPath).toEqual('/foo2');

historyUnsubscribe();
});
});

describe('syncReduxAndRouter', () => {
let history, store, unsubscribe;

Expand Down