Get devtools working

[kimjoar]

This is a WIP based on #70. With this change both devtools and the tests run (I didn't remove changeId, so might be there are something in the tests that actually isn't working as expected, of course). It solves the problems in #61, as far as I can see.

Anyone else want to play around with it? Checkout this PR (help in https://help.github.com/articles/checking-out-pull-requests-locally/), copy in these changes, then run npm run build in root and then run npm start in examples/basic/, then open up examples/basic/index.html and test.

@jlongster Thoughts on kimjoar@ef42b0f? Might be the change from changeId has broken something subtle. The tests should probably rely less on checking these ids, and instead focus on the actual store and history changes. That way we can change the internal implementation and still be sure the code does what it should (i.e. its external api).

[ellbee]

Looks good. I made the following quick mod locally which fixes this problem:

@@ -33,8 +33,8 @@ function replacePath(path, state, { avoidRouterUpdate = false } = {}) {

 // Reducer

-const initialState = {
-  changeId: 1,
+let initialState = {
+  changeId: 0,
   path: undefined,
   state: undefined,
   replace: false
@@ -70,6 +70,7 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) {
   }

   const unsubscribeHistory = history.listen(location => {
+
     const route = {
       path: history.createPath(location),
       state: location.state
@@ -86,6 +87,15 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) {
       ? replacePath
       : pushPath;

+    if (initialState.changeId === 0) {
+      initialState = {
+        changeId: 1,
+        path: route.path,
+        state: route.state,
+        replace: !updatePath
+      };
+    };
+
     store.dispatch(updatePath(route.path, route.state, { avoidRouterUpdate: true }));
   });

It would be nice to fix this as well while you are looking at the devtools. Can you think of a neater way to do it?

[kimjoar]

@ellbee Ah, yes, we need to think of something to solve that problem too. Changing the initialState feels very hacky, so I really think we need to do something cleaner. Another solution could be to not update the path if path is undefined. Not sure how that would work, need to play around with this.

[ellbee]

Not updating the path if path is undefined doesn't seem like it is going to take the user back to the page that they initially navigated to, but maybe I am misunderstanding.

Devtools is going to set the routing state to initialState on 'RESET', so I think that if we are to return the user to the path that they originally navigated to it does need to be contained in initialState. The awkward thing is that we are only notified of the initial route when we listen to history in syncReduxAndRouter, by which time the store is already initialised.

[kimjoar]

Yeah, as of right now I don't have any good ideas on how to solve the problem. I really don't think we should override initialState, it makes the code harder to reason about. To me it's better to not support revert than to make the code more difficult to understand, but might be @jlongster disagrees, of course.

One option is to keep the first location change and use that if called with the initial state. It kinda feels a little bit easier to reason about, but not much :/

diff --git a/src/index.js b/src/index.js
index 8b34983..0ec0df2 100644
--- a/src/index.js
+++ b/src/index.js
@@ -60,6 +60,7 @@ function locationsAreEqual(a, b) {

 function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) {
   const getRouterState = () => selectRouterState(store.getState());
+  let firstRoute = undefined;
   let lastRoute = {};

   if(!getRouterState()) {
@@ -74,6 +75,10 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) {
       path: history.createPath(location),
       state: location.state
     };
+
+    if (firstRoute === undefined) {
+      firstRoute = route;
+    }
     console.log('HISTORY', route);

     // Avoid dispatching an action if the store is already up-to-date,
@@ -90,9 +95,13 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) {
   });

   const unsubscribeStore = store.subscribe(() => {
-    const routing = getRouterState();
+    let routing = getRouterState();
     console.log('STORE', routing);

+    if (routing === initialState) {
+      routing = firstRoute;
+    }
+
     // Only update the router once per `pushPath` call. This is
     // indicated by the `changeId` state; when that number changes, we
     // should update the history.

[ellbee]

I don't like the mutation either. Your idea there is how I initially proposed fixing this back when the project was new. It certainly works, although I'm not sure that it is any clearer.

@jlongster proposed something similar to what I have in that first diff here, the difference being that initialState is left undefined until we get the initial callback from history. I am going to have a play with that, I'm keen to get this fixed.

[kimjoar]

Yeah, none of them are very clear :/

[jlongster]

@jlongster Thoughts on kimjoar/redux-simple-router@ef42b0f? Might be the change from changeId has broken something subtle. The tests should probably rely less on checking these ids, and instead focus on the actual store and history changes. That way we can change the internal implementation and still be sure the code does what it should (i.e. its external api).

This looks like a regression. If you call pushPath twice with the same URL it needs to call history.pushState twice. This is so that history can do whatever browsers do whenever you click the same link twice (I think the history library does a URL replace).

changeId was nice because you could easily do that and tests could make sure that it happened. I'll try out the devtools locally though to fully understand why it isn't working.

[kimjoar]

@jlongster Ah, good catch — maybe we should add a test for that which does not rely on changeId?

[jlongster]

changeId is the perfect way to test for it IMHO, any other ways would be pretty complicated. But I dunno, maybe integrating changeId in the tests makes it too hard to actually change the library. I'm open to avoiding it in the tests.

[kimjoar]

@jlongster Yeah, good point. Btw, can you try the latest commit? It seems to work as expected for me, but still not sure I have understood all the subtleties around changeId+history.

[kimjoar]

(This includes my "Revert" fix, which I don't particularly like, but at least I think devtools work as expected with these changes. I can remove the firstRoute stuff if you only want a fix for the other stuff now.)

[fkrauthan]

Do we have an ETA for a new version with a fix? This is right now blocking a important work project for me.

[kimjoar]

@fkrauthan If you need this asap, please help out with testing and coming up with ideas. Otherwise: it's done when it's done. As you can see we're working on a solution.

[fkrauthan]

@kjbekkelund Why don't we just use a simple solution like this: https://gist.github.com/fkrauthan/0fe48db6891a15ea451c (I haven't tested it but from a logic point of view that should work perfectly)

[ellbee]

(This includes my "Revert" fix, which I don't particularly like, but at least I think devtools work as expected with these changes. I can remove the firstRoute stuff if you only want a fix for the other stuff now.)

@kjbekkelund Just been playing with the PR and it seems to be working as expected with devtools. There is a tiny problem in that if you disable the initial router action, a new one gets added because the state is undefined, and if you then disable that you get another etc. This doesn't really bother me too much. And as from a code point of view I don't particularly like any of the proposed "Revert" solutions better than any of the others (including the one in the PR I opened earlier), going with this solution, at least for now, would get a thumbs up from me.

[jlongster]

@fkrauthan hard to see what you actually changed. but it appears that it would call history.pushState for every single state change in the entire redux store.

I will try to commit a fix by the weekend, whether it's @kjbekkelund's PR or a modified version of it.

[fkrauthan]

Sorry @jlongster I've introduced let avoidAvoidRouterUpdate = false; in Line 72 and use it in line 87 and 100

[jlongster]

@kjbekkelund latest version looks promising!

[kimjoar]

@jlongster Nice! I can try to write up some tests for it and remove the console.logs. (I can also give you access to push to my fork if you want to add or change anything to this PR before merging.)

[kimjoar]

Cleaned it up a little bit more. This diff doesn't look too bad. Will look at tests.

[kimjoar]

@jlongster @ellbee @fkrauthan Added some tests — do they look sane?

(Wasn't sure about the best way to test devtools toggle and reset, so took inspiration from https://github.com/gaearon/redux-devtools/blob/master/test/devTools.spec.js)

[fkrauthan]

@kjbekkelund Looks good to me.

[jlongster]

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

[kimjoar]

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)

[kimjoar]

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

[jlongster]

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.

[jlongster]

Ok I'll merge this tomorrow once the devtools dependency is removed. I've never used the devtools API before so it's cool that you even wrote tests for it!

[kimjoar]

I'll have to re-think the tests (or the implementation) if the devtools dep is removed. Any reason you don't want it as a dev dep?

[jlongster]

I just thought you may have accidentally installed it to test it. I'm totally fine if it's there for a reason. Thanks for all your work!

[jlongster]

Looks good, thanks a lot! I may tweak a few styles tomorrow but I should be able to push a new release.

What version should it be? I'll definitely write a changelop, but the API is completely different. Should we just make it 1.0.0?

[kimjoar]

I can create a small PR than adds some comments to the DevTools related tests. Just to make it easier to understand what the intention is.

I think it might be time for 1.0. If we've done something stupid we can just create a 2.0 later. For now it seems to solve the problems we've seen mentioned in the issues at least.

[fkrauthan]

@jlongster when do you plan to release a new version on NPM so we can pick up the fix? Unfortunately based on company restrictions I can only use official released versions.

[jlongster]

Very soon; I was traveling all day yesterday and we just need to update docs.