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

Consider the full path in changing PUSH to REPLACE #167

Merged
merged 1 commit into from
Dec 1, 2015

Conversation

taion
Copy link
Contributor

@taion taion commented Dec 1, 2015

Fixes #166

Organizing the changelog a bit ahead of adding something for #141 to hopefully make it a bit more usable.

let { pathname, search } = getCurrentLocation()
let currentPath = pathname + search
let path = nextLocation.pathname + nextLocation.search
const prevPath = createPath(location)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not 100% correct, since it doesn't e.g. account for basename when using browser history. In practice that's not an issue, though, since basename doesn't change.

This should be the "final" createPath, but I can't access that here.

@agundermann
Copy link
Contributor

LGTM. Thanks for tackling that.

As for your question why I used getCurrentLocation(), I think I wanted to avoid race conditions and mimic the default browser behavior as close as possible. Right now I can't think of any realistic condition where using location instead would be a problem, so I'm good with that change.

@taion
Copy link
Contributor Author

taion commented Dec 1, 2015

I believe in a base history, transitions are fully synchronous, so I don't think race conditions are an issue. Let's go with this for now and patch later if needed.

taion added a commit that referenced this pull request Dec 1, 2015
Consider the full path in changing PUSH to REPLACE
@taion taion merged commit 331ee28 into remix-run:master Dec 1, 2015
@taion taion deleted the hash-replace branch December 1, 2015 15:01
@taion
Copy link
Contributor Author

taion commented Dec 1, 2015

@taurose I think there might be a separate related race condition when using async routes in ReactRouter.useRoutes, though - i.e. where the transition is async, so the user can e.g. change their mind and try to PUSH a separate location while the old routing state is still shown... for browser-mimicking that should get changed to a REPLACE, but we can't even handle that in history since the route state management is all downstream.

@agundermann
Copy link
Contributor

@taion Yeah that seems like a feature we lost from 0.13. Since history doesn't deal with data loading (or anything that happens after the URL changes), I'd say this should be handled by the router.

@taion
Copy link
Contributor Author

taion commented Dec 4, 2015

Yeah, the router can just use listenBefore.

@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.

2 participants