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

skip pushing to history when location matches link (#5362) #5363

Closed
wants to merge 1 commit into from
Closed

skip pushing to history when location matches link (#5362) #5363

wants to merge 1 commit into from

Conversation

jasonmacgowan
Copy link

No description provided.

@pshrmn
Copy link
Contributor

pshrmn commented Jul 21, 2017

This is something that would have to be handled by history because you'll still want to trigger a re-render. I had opened this PR over there remix-run/history#472 to essentially do this, but I ended up closing it for a couple of reasons.

@vladshcherbin
Copy link
Contributor

@pshrmn current behavior differs from default browser one. Currently, it is unexpected and breaks navigation logic for page transitions for example.

It was working correctly in React Router 3.

@vladshcherbin
Copy link
Contributor

@pshrmn is it possible to reopen your PR in history or patch Link until history fixes this?

I'd consider this a bug since if differs from what browser does.

cc @mjackson @timdorr

@pshrmn
Copy link
Contributor

pshrmn commented Jul 28, 2017

I agree that the current implementation is incorrect, but I'm a bit wait and see on re-opening the PR. Is the "correct" behavior to replace instead of push or to just cancel the transition?

When I was writing hickory I implemented the same API as in the above PR (although I decided I preferred the name update). However, I just took a quick look at what vue-routerdoes and it aborts the transition when you attempt to navigate to the same route. I want to say that replacing is better than aborting because if someone clicks a link, they expect navigation to actually happen (allowing you to reload the page).

I'm not actually sure how it worked in v3 to prevent duplicate locations from causing duplicate entries. @vladshcherbin do you know if it was history or RR that prevented re-routing to a duplicate location?

@vladshcherbin
Copy link
Contributor

@pshrmn when I found out duplication, I checked my old project and it's not duplicating same location. The project is using RR v3.0.2 and history v3.2.1.

From the source code of that version, here is probably how this case was resolved: https://github.com/ReactTraining/history/blob/v3.2.1/modules/createHistory.js#L99

@pshrmn
Copy link
Contributor

pshrmn commented Jul 28, 2017

Alright, so v3 still emits a new location when the location is that same. I'll re-open my issue in history. I'm not sure whether my PR is the way to go (they might prefer not to introduce a new API), but at least discussion should happen over there.

@vladshcherbin
Copy link
Contributor

@pshrmn thank you, it'll be really great to restore how it worked in v3.

We are trying to create an example of page transition using RR v4 and React Transition Group v2 and current history behavior is really complicating the task as we rely on location as a component key.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
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.

3 participants