1.0.0 breaks if history uses basename option

[dtryon]

Hi,
We are currently using react-router version 1.0.0 createHistory which imports the browser history strategy.

With 0.0.10 all was fine, but upgrading causes a pushState to loop forever. :(

We are wiring up basename like this:

const history = useBasename(createHistory)({basename: '/foobar'});

Now, when we use pushState it loops. I think that the problem is in this code:

} else if(!locationsAreEqual(getRouterState(), route)) {
      // The above check avoids dispatching an action if the store is
      // already up-to-date
      const method = location.action === 'REPLACE' ? replacePath : pushPath
      store.dispatch(method(route.path, route.state, { avoidRouterUpdate: true }))
    }

The !locationsAreEqual always returns true because the call to history.createPath(location) comes back with the basename prepended.

If I debug in the browser, I can see it looping over this case.

I think the solution would be to add this type of thing lastRoute.changeId !== routing.changeId like in unsubscribeStore.

I'll see if I can reproduce in a unit test and issue a PR.

[hunterc]

Running into this also with #102 and just found the same thing.

[kimjoar]

Interesting. I'm taking a look too, @hunterc @dtryon. Would be awesome if you're able to create a failing unit test for it!

[dtryon]

We have a failing test:

it('handles basename history option', () => {
        const store = createStore(combineReducers({
          routing: routeReducer
        }))
        const history = useBasename(createHistory)({basename:'/foobar'});
        syncReduxAndRouter(history, store);

        store.dispatch(pushPath('/bar'))
        expect(store).toContainRoute({
          path: '/bar',
          changeId: 2,
          replace: false,
          state: undefined
        })
      })

*Warning: this just blows the call stack

In order to set this up we passed in useBasename from node/index.js:

const { createMemoryHistory, useBasename } = require('history')
const createTests = require('../createTests.js')

createTests(createMemoryHistory, useBasename, 'Memory History')

[kimjoar]

@dtryon Nice, thanks. I got @hunterc's example to run locally with this change:

diff --git a/src/index.js b/src/index.js
index 10cb737..495ec76 100644
--- a/src/index.js
+++ b/src/index.js
@@ -71,6 +71,16 @@ function locationsAreEqual(a, b) {
   return a != null && b != null && a.path === b.path && deepEqual(a.state, b.state)
 }

+function createPath(location) {
+  const { pathname, search, hash } = location
+  let result = pathname
+  if (search)
+    result += search
+  if (hash)
+    result += hash
+  return result
+}
+
 function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) {
   const getRouterState = () => selectRouterState(store.getState())

@@ -92,7 +102,7 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) {

   const unsubscribeHistory = history.listen(location => {
     const route = {
-      path: history.createPath(location),
+      path: createPath(location),
       state: location.state
     }

I'll add your test to the code base and play around with potential solutions. Thanks!

[hunterc]

@dtryon @kjbekkelund thanks for the quick responses!

[dtryon]

That passes the test for me.

[kimjoar]

Yep, I think that might the solution for now. I'll create a PR.

[kimjoar]

@dtryon @hunterc Fixed in master. I don't have access to release a patch release, so for now you have to rely on master if you want this asap. You can easily set that up in package.json:

"redux-simple-router": "https://github.com/jlongster/redux-simple-router.git#master"

After npm install you have to

cd node_modules/redux-simple-router
npm install
npm run build

Then you'll have the latest code.

[AllSpeeds]

+1 for releasing this patch. Thanks!

[jlongster]

Can we close this issue if it's been fixed? I'm fine releasing a point release if it's not a breaking change.

[kimjoar]

It shouldn't break anything, so should be safe to release a patch.

[smazurov]

Ran into this, really not a fan of having to use master, any chance of a point release happening?

[AndrewIngram]

ditto, ran into this today, and had to roll back

[dtryon]

@kjbekkelund Can we reopen this until it is released under a point release?

[jlongster]

Sure! Sorry about this, I will organize a release today.

[hunterc]

thanks @jlongster

[jlongster]

Sorry, the past few days have been pretty crazy. We are ready to move to the rackt org and I'm doing that first thing tomorrow, and I'm going to do a point release right before I do that.

[jlongster]

Just released 1.0.1 with this fix.