An extension of #109 for stopping cycles

[jlongster]

@kjbekkelund This is somewhat similar to #109 but takes the idea even farther. We can remove avoidRouterUpdate and changeId completely with this. It uses a local variable to tell the store listener not to update the router, but also uses a reference identity check of the router state for the store listener to know when to update the router.

Most tests pass; the only failing ones are with the devtools. The test about toggling routing actions fails, even though it works for me in the basic example. I can toggle route actions and it updates the router just fine.

What do you think?

[jlongster]

This diff looks kind of strange because I had to switch the order of history.listen and store.subscribe so that the latter would be run on the initial route update and set the local avoidRouterUpdate flag to false.

[kimjoar]

@jlongster Nice — I think we're onto something here. It's getting late in Norway so I'll take this for a spin tomorrow when I'm more awake ;)

[jlongster]

Cool! No rush, I probably won't be too active this weekend. Also I left changeId in there for now only because I haven't updated the tests, the final PR can remove it completely.

[jlongster]

Hey @kjbekkelund, do you think you'll be able to check this out in the next few days? I'd like to clean this up before I go off on holiday, but I don't want to invest too much in this if you see any problems.

Not a big deal if you don't have time.

[kimjoar]

I thought it could be related to async. With a small change I get the test running:

-      it('handles toggle after store change', () => {
+      it.only('handles toggle after history change', (done) => {
         let currentPath
+        let finished = false
         const historyUnsubscribe = history.listen(location => {
           currentPath = location.pathname
+
+          if (finished) {
+            expect(currentPath).toEqual('/foo2')
+            historyUnsubscribe()
+            done()
+          }
         })

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

         // When we toggle an action, the devtools will revert the action
         // and we therefore expect the history to update to the previous path
+        finished = true;
         devToolsStore.dispatch(ActionCreators.toggleAction(3))
-        expect(currentPath).toEqual('/foo2')
-
-        historyUnsubscribe()
       })

but for some strange reason it fails when run without .only 😭

If you add a console.log in the history.listen you can see that it's correct when run with .only, but not otherwise. So there's something strange going on.

I've tested the branch in my app, and as far as I can see everything works as expected (however, I don't rely on listenBefore and other such features).

I think this is an interesting approach and something we should clean up and try to get into master. Could you also bring in the additional tests in #109, so we're sure to cover that case?

[jlongster]

Thanks for taking a look. I will port over the tests from your PR. I've never heard of it.only before, that's interesting. I'll probably get this fixed up and merged some time next week, as its Christmas here.

[taion]

I'm not a Redux expert, but I feel like you'd be better off using a simpler middleware-based approach.

The idea is, you stamp the action with a bit indicating whether or not the action came from the Router (v. being manually initiated).

1. If the action doesn't have this bit set, then swallow the action, and instead make an appropriate call on the history.
2. If the action does have this bit set, then unset the bit and pass it on.

Anything coming through the router will hit (2). Anything coming from a user will hit (1), get dropped, then hit (2) via the history, including if you're replaying actions through (2).

I think this would get rid of most of your stateful book-keeping.

[jlongster]

Closing in favor of #141