Skip to content

Dispatch even if path has not changed & normalize Locations #1031

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

Closed
wants to merge 2 commits into from

Conversation

agundermann
Copy link
Contributor

This PR does two things:

  1. Always dispatch even if the path is the same as the one that was rendered last.
  2. Normalize behavior of Location push/replace in order to
    • not create an additional history entry if path has not changed (HistoryLocation and TestLocation)
    • trigger refresh (new LocationAction type) even if path has not changed (currently HashLocation just ignores those)

For 1), I needed to update some tests to avoid recursion loops and adapt to the new behavior of causing more dispatches.

For 2), it would have also been possible to implement that in Router.transitionTo/replaceWith. I opted for changing the locations because the changes actually handle two different problems as outlined above. So it seems more logical to me to normalize them this way, instead of having the router work around those discrepancies.

Notes:

  • this will make it so that aborting a transition will cause the previously rendered route to be refreshed
  • edge case for HashLocation: using browser navigation to go back or forward (multiple steps) to the same path that's currently active will not trigger a refresh, since this won't trigger the hashchange event

Fixes #826 and #1027

Would be great if someone could look over this :)

@hiddentao
Copy link

+1

@ryanflorence
Copy link
Member

I'm going really fast through the issues right now, so I'm not sure if this is still a problem on master. The API and implementation is basically a rewrite for 1.0, can you verify on master if the problems that led to this PR are still an issue?

After a pass or two of cleaning up the issues, I'll come back and try to understand this more.

Thanks :)

@agundermann
Copy link
Contributor Author

As far as I can tell, the issue of not re-rendering when transitioning to the previously rendered path is fixed, ie. there's no more if(prevPath === curPath) return;. Also, HashHistory.push doesn't rely on onhashchange anymore to trigger change events.

The remaining issue is that transitionTo(currentPath) creates a new history/back button entry with BrowserHistory, but not with HashHistory. That's because setting location.hash to the current value has no effect, while pushState doesn't care about the current URL and always creates a new entry.

So I think all that's left would be to make BrowserHistory.push use replaceState instead of pushState if the given path is the same as the current one, perhaps with the possibility of opting out.

@ryanflorence
Copy link
Member

So I think all that's left would be to make BrowserHistory.push use replaceState instead of pushState if the given path is the same as the current one, perhaps with the possibility of opting out.

Seems good, should we close this and open a new issue/pr for that change?

@mjackson
Copy link
Member

@taurose That makes sense. Would you be willing to create a new PR for that change? I'll let you close this if so, or just push it here. Up to you.

@mjackson
Copy link
Member

@taurose Is this completely superseded by #1363? If so, let's close.

@agundermann
Copy link
Contributor Author

What's left of it, yeah ;)

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.

Behavior when transitioning to same path
4 participants