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

Fix issue when initial location redirects #289

Merged
merged 2 commits into from
Feb 18, 2016

Conversation

decafdennis
Copy link
Contributor

We've been using the cutting edge version in a new project, and all work that went into v4 is much appreciated!

Today we found a bug: if the initial route redirects to another location, then the Router component doesn't update properly and leaves the user with an empty page. It's common to use onEnter on a Route to redirect the user to a login page when she is not logged. The syncs history -> components when the initial route gets replaced test in this PR replicates the problem.

The problem lies in the syncHistoryWithStore's custom listening logic. It makes the first synchronous call to each listener before it subscribes to the store changes, so if the listener causes a location change and thus a store change (e.g. when a location redirects), then the listener is not notified of that change. This PR fixes the problem by subscribing to the store before making the first call to the listener.

Hopefully this bug can be fixed before the final v4 is released!

@schickling
Copy link

Hit the same problem. Thanks for creating this PR! :shipit:

@btmills
Copy link

btmills commented Feb 18, 2016

Thanks for this @decafdennis!

For those who need a workaround until this fix is released, you can use the asynchronous callback:

-function requireAuth(nextState, replace) {
+function requireAuth(nextState, replace, callback) {
     if (!isAuthenticated()) {
         replace({
             pathname: '/login',
             state: {
                 nextPathname: nextState.location.pathname
             }
         });
     }
+ 
+    setTimeout(callback, 0); // TODO: Remove this once https://github.com/reactjs/react-router-redux/pull/289 is released
 }

@decafdennis
Copy link
Contributor Author

Thanks for the workaround @btmills!

I also published @decafdennis/react-router-redux to NPM as a temporary workaround. It's 4.0.0-rc.1 with the fix. You can alias the package name in your webpack config if you're using that.

@timdorr
Copy link
Member

timdorr commented Feb 18, 2016

Interesting! Thanks for writing a solid test case for this too.

timdorr added a commit that referenced this pull request Feb 18, 2016
Fix issue when initial location redirects
@timdorr timdorr merged commit 0c11a85 into reactjs:master Feb 18, 2016
@schickling
Copy link

Awesome! Will there be a new version for this?

@timdorr
Copy link
Member

timdorr commented Feb 18, 2016

Yeah, I'll do a quick rc2 tonight.

@schickling
Copy link

Thanks @timdorr! 👍

@decafdennis
Copy link
Contributor Author

@timdorr: thanks for the quick merge!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants