Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected difference between clicking a <Link> or clicking the back button #5072

Closed
deiucanta opened this issue May 4, 2017 · 28 comments
Closed

Comments

@deiucanta
Copy link

Test Case

https://codesandbox.io/s/G6nWE3X0r

Steps to reproduce

  • click on "page 1" link
  • click on "back to home" link
  • click on "page 2" link
  • press the browser's back button

Expected Behavior

It should go back to /.

Actual Behavior

It's stuck showing "page 2".


The <App> component is not blocking the update since it has the 'location' injected via the connect() function. However, if I change connect() to withRouter() it works.

Also, when I remove the <Switch> component the back button works as expected.

@pshrmn
Copy link
Contributor

pshrmn commented May 4, 2017

@deiucanta You can safely ignore this comment.

@timdorr I went ahead and made a react-router-redux label so that issues/PRs for it can be easily identified. If you don't want this, feel free to remove it, but I thought that it could be helpful. Another label for react-router-config could probably also be made (and maybe the other packages as well).

@timdorr
Copy link
Member

timdorr commented May 4, 2017

@pshrmn I had created Projects for all of them, but they aren't really that easy to use I've found. The labels work better.

@deiucanta That appears to be a side effect of using that site. I guess their browser emulation isn't complete or to spec.

@timdorr timdorr closed this as completed May 4, 2017
@deiucanta
Copy link
Author

@timdorr I initially got this behaviour working a project on my local machine and then I replicated it in codesandbox.io. Please reopen the issue and let me know if you have other hints on where the issue might be.

@agundermann
Copy link
Contributor

agundermann commented May 4, 2017

Getting the same behavior after downloading and running it locally.

I looked into it a bit, and I think the main problem is that, while both <ConnectedRouter> and <Router> subscribe to history, <ConnectedRouter> is called first, causing the app to update before <Router> gets a chance to update the context of the app. The update that would then be caused by <Router> is prevented by connect(..) (sCU), because the store's location has already been changed by the first update.

The reason it works on the first transition (clicking Page x) is that React batches the first update since the update results from an event handled by React, so connect(..) will only be rendered once at the end and not block any updates.

When appending setTimeout(() => history.push('/page/2')) to index.js, for example, it doesn't properly update either as there is no batching of the two updates.

@deiucanta
Copy link
Author

@taurose Inspecting with ReactDevTools I can see that <App>s location prop is changing when I hit the back button but inside the <Switch> it matches the previous route.

The component structure is like this

<ConnectedRouter>
  <Router>
    <App>
      <Switch>

On a regular location change, this is what happens

  1. ConnectedRouter listens to history and it dispatches a redux action to change the location in store
  2. Router listens to history and updates the context values
  3. App listens to the redux store and updates because there is a new location

So, you're basically saying that step 3 somehow happens before step 2 and when the Router updates the context values is too late because App will not render again since the props don't change.

Am I right? Should this be fixed in the library or should I change my code to make it work?

@timdorr
Copy link
Member

timdorr commented May 5, 2017

@taurose Thanks for the investigation. This sounds like a legit bug with the library.

@deiucanta Sorry for the fast close. It's still in alpha, so things like this need to be worked out.

@timdorr timdorr reopened this May 5, 2017
@chrismllr
Copy link

Hey guys - can confirm another case of this... I've noticed the location object within the router reducer Does change, so I am able to assert there is a discrepancy between the two and call a history.go() in the meantime

@agundermann
Copy link
Contributor

agundermann commented May 5, 2017

@deiucanta Yep, that sums it up well.

By the way, I think it should work fine with controlled <Route>s, <Switch>es and <NavLink>s (ie. passing location as a prop to them yourself). Seems more reasonable to me anyways,

@pmwisdom
Copy link

pmwisdom commented May 10, 2017

I think I was having this same problem and "solved" this for myself by connecting my switches to the store. Or you could pass the router location down through its props manually (since Switch allows you to pass in your own location object).

//connected-switch.js

import {connect} from 'react-redux';
import {Switch} from 'react-router';

const mapStateToProps = state => {
	return {location: state.router.location};
};

const ConnectedSwitch = connect(mapStateToProps)(Switch);

export default ConnectedSwitch;

If you look in the React developer tools (at your switch component), you can actually see where the Switch component hangs up on the old location object without this hack.

@xzilja
Copy link
Contributor

xzilja commented May 12, 2017

@pmwisdom Came to same solution as well. However, to me it feels like Switches wrapped in ConnectedRouter should do this on their own.

@deiucanta
Copy link
Author

@iljadaderko totally agree

@deiucanta
Copy link
Author

@timdorr did you manage to look into this? If you give me some guidance I'm happy to make a PR.

@dak0rn
Copy link

dak0rn commented May 30, 2017

I think I have the same problem.

I use react-router with the Redux package in a web application. I noticed
the same behaviour; when using <Link />s, routes are rendered correctly.
Upon using the back button, the UI is one step behind (pressing back again
renders the page it should have rendered in the first step).

I added logging statements to Route, Router and my component wrapper.
Upon clicking a <Link />, the order of operations looks as follows:

normal

The child context is updated with the new location and all <Route />s rerender.

When using the back button, however, it looks differently:

back

All Routes are rerendered first, the context (context_path) contains the old
path while history contains the correct path (actual_path). The context providing
the former is updated like in the middle.

The location is set correctly in the Store, by the way, so the navigation highlights
the actual path while the main UI is not updated.

I do not really have an idea, yet, why the order is different. It is also interesting
that the route contained in the route for /admin also gets the wrong path
through context although getChildContext was already invoked (last three lines).

Why does it help to connect Switch?

Because it passes down the location prop which is then used by Route instead of context.

@Geoff-Ford
Copy link

FYI: I have been having the same issue but with react-router-redux v5 and react-router-dom. The suggestion from @pmwisdom to create a ConnectedSwitch container component and swapping this in place of Switch resolves the issue.

@hawksdoves
Copy link

hawksdoves commented Jun 5, 2017

Experiencing this as well.

Using ‘react-router-redux’, the <ConnectedRouter> location state/prop is updating correctly as route changes. However, the location prop passed to the <Route> render methods are not updating in-sync. Example: after pressing the back button, the location.pathname has not been updated and will continue to be out of sync by one, such that when using <Route children={match}> you can see that match returns null, when the route does actually match.

As previously mentioned, in the mean time passing down location from the connectedRouter as a prop to <Route>, <Switch> etc will solve this problem.

@kamronbatman
Copy link

@Geoff-Ford, Can you set up a gist with your ConnectedSwitch code? I would love to see how you did the workaround. Thanks!

@deiucanta
Copy link
Author

import { connect } from 'react-redux'
import { Switch } from 'react-router-dom'

const mapStateToProps = state => ({
  location: state.router.location,
})

const ConnectedSwitch = connect(mapStateToProps)(Switch)

export default ConnectedSwitch

@kamronbatman
Copy link

kamronbatman commented Jun 21, 2017

Thanks @deiucanta. After I posted that question, I did find this (which is similar).
ConnectedSwitch.js

@Geoff-Ford
Copy link

Sorry for the tarde response. What @deiucanta has provided there is exactly the same as I have implemented too.

@timdorr
Copy link
Member

timdorr commented Jul 12, 2017

I wonder if #5203 fixed this unintentionally...

@DanHarman
Copy link

I hit this too on a set of Routes outside a switch. So I guess it means a ConnectedRoute is needed if there isn't a cleaner fix.

Just goes to show why context is not recommended by the react team!

@comerc
Copy link

comerc commented Jul 21, 2017

My 50 cents. Do not send location to Switch:

import React from 'react'
import { connect } from 'react-redux'
import { Switch, Route } from 'react-router-dom'

const addLocation = connect(state => ({
  location: state.routing.location,
}))

export const ConnectedSwitch = addLocation(({ location, ...props }) => <Switch {...props} />)
export const ConnectedRoute = addLocation(props => <Route {...props} />)

@davidcalhoun
Copy link

davidcalhoun commented Jul 27, 2017

Thanks @pmwisdom! I thought I was going nuts! I ended up using method 2, passing location directly to Switch.

It appeared to work ok, until in my case I realized that each route page was undergoing an FULL rerender on updates to the store for some strange reason, and this really wreaked havoc. Instead of connecting Switch, I ended up connecting each individual page component instead. Not ideal, but at least the rendering is working as expected now (efficient partial DOM updates).

Not sure if I have stuff setup wrong.. wouldn't be surprised...

@solominh
Copy link

solominh commented Sep 4, 2017

I have came across this library and article (from Facebook docs recommendation)
https://github.com/ReactTraining/react-broadcast
https://medium.com/@mweststrate/how-to-safely-use-react-context-b7e343eff076

I have a question: Have react-router started using this observable pattern yet?
I've looked for it in react-router V4 source code but apparently react-router haven't applied it yet.

@athomann
Copy link

athomann commented Sep 7, 2017

@timdorr this may be because of the connect in App.js. Adding withRouter on

export default withRouter(connect(mapStateToProps)(App));

fixes the issue. Example

This is also mentioned in https://github.com/ReactTraining/react-router/blob/master/packages/react-router/docs/guides/blocked-updates.md

and

https://reacttraining.com/react-router/core/guides/redux-integration/blocked-updates

@SadPandaBear
Copy link
Contributor

SadPandaBear commented Sep 12, 2017

I wonder if we should be creating the ConnectedSwitch everytime we want to provide a Switch in order to not to mess up with the connected location state or open up a pull request so React Router Redux can provide it's own ConnectedSwitch...

@jduthon
Copy link

jduthon commented Sep 20, 2017

Another related bug : https://codesandbox.io/s/z6ozxy053
Following the same process (so first clicking on Page X, then going back with the browser back button), the Page component will be rendered with an incorrect location for a moment ('/') before unmounting.
Which is kind of annoying when you are relaying on router state to display components which need props from the route. Then you need to add some extra protection on said components because the unmounting will only happen later.

As for the ConnectedSwitch, I believe it would work, but I am also using react-router-config, which uses Switch internally in renderRoutes, and I don't know of a way to pass your own location object to the underlying switch in ConnectedSwitch.

@timdorr
Copy link
Member

timdorr commented Jun 6, 2018

Closing this out since react-router-redux is deprecated. Use connected-react-router instead!

@timdorr timdorr closed this as completed Jun 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests