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

Fixes onNavigation not being called when changing to identical route. #133

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixes onNavigation not being called when changing to identical route. #133

wants to merge 1 commit into from

Conversation

wali-s
Copy link

@wali-s wali-s commented Apr 15, 2015

onNavigation should still be called even if you're navigating to an identical route because react-router-component does create an entry in its history for it.

I don't think it should just ignore it if they have the same path.

`onNavigation` should still be called even if you're navigating to an identical route because `react-router-component` does create an entry in its history for it.

I don't think it should just ignore it if they have the same path.
@STRML
Copy link
Owner

STRML commented Apr 17, 2015

What's the usecase for this?

If we make this change, it would need to be made in all environments, not just PathNameEnvironment.

There is no internal history in RRC (only the window history api). There's nothing stopping you from clicking a link to an identical route, which actually will be pushed to history (and whether or not we should be doing that is debatable, since it makes hitting the back button annoying). The only time this code path will get hit is on a back button hit, and it seems like a nice optimization to make to just avoid all navigation handlers since really, nothing has happened.

It seems to me that a check in setPath to see if we're actually navigating somewhere would be useful to avoid the duplicate entries in history; in that case, popping to an identical route wouldn't be possible.

The current behavior on popstate is identical to how backbone does it.

@wali-s
Copy link
Author

wali-s commented Apr 17, 2015

I have a form that, when submitted, takes you back to the same page with data from the server on the page, along with that form available to be submitted again. The URL stays the same and you can resubmit the form any number of times.

So that users can press back and not come to a blank page, I store the props in sessionStorage and restore them onNavigation. This works fine if the URL changes, but doesn't when it stays the same.

I don't fully know if the way RRC is doing it is necessarily wrong, but I do know that it doesn't work for my particular use-case, which may be similar to others'.

@STRML
Copy link
Owner

STRML commented Apr 17, 2015

I'm hesitant to change the behavior since it matches the behavior of other routers and I want to maintain least surprise.

You could sidestep the issue by returning a unique submission ID from the POST and appending it to the url (/form/submit/:id) so that the URL reflects the difference between an empty unsubmitted form and a submitted one.

@wali-s
Copy link
Author

wali-s commented Apr 24, 2015

Alright, that's fair. My use-case is probably a pretty uncommon one.

Your suggested solution doesn't work for me because they can specify to save the form data on the server, in which case they get the unique submission ID in the URL so that they could navigate to it later. Doing that in every case, even when they don't opt to save the form data, would confuse my users.

You're welcome to close the PR though; I can continue to use my fork.

Thanks.

@jsg2021
Copy link
Contributor

jsg2021 commented May 22, 2017

@STRML the {isPopState: true} should prevent the odd behaviour you were talking about... setPath() short circuits if isPopState is true so that it does not re-push the path to history. the pathname check seems superfluous at this point.

@jsg2021
Copy link
Contributor

jsg2021 commented May 22, 2017

I'm not sure how you would implement this behavior change in the other environments.

@jsg2021
Copy link
Contributor

jsg2021 commented May 22, 2017

Actually, the main three can know "isPopState"... so nm, all of the environments would be ok :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants