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

Strip basename from location provided to getKey #10550

Merged
merged 8 commits into from
Jun 6, 2023

Conversation

brophdawg11
Copy link
Contributor

Fixes issue raised in #9565 (reply in thread)

We were calling getKey inconsistently. From the usePageHide hook it was the useLocation value (not including basename) but from inside the router it was the router.state.location (including basename). Now it's always the user-land version without the basename

@changeset-bot
Copy link

changeset-bot bot commented Jun 1, 2023

🦋 Changeset detected

Latest commit: 797fcb1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@remix-run/router Patch
react-router Patch
react-router-dom Patch
react-router-dom-v5-compat Patch
react-router-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jrnail23
Copy link

jrnail23 commented Jun 1, 2023

Thanks for jumping on this so quickly, @brophdawg11. Any reason stripping basename isn't the useLocation behavior all the time? It seems like it should be consistent everywhere, but I've had to strip it myself in another case where I was capturing a location to use for a "back" button in my UI (similar to what I've talked about in the Events discussion).

@brophdawg11
Copy link
Contributor Author

It is the default - you should never be getting a path back with the basename from useLocation. If you are, a reproduction would be great to see why...

let loc = {
...location,
pathname:
stripBasename(location.pathname, basename) || location.pathname,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: see about moving this to the RR layer since that's where the basename stripping should happen to stay consistent

@jrnail23
Copy link

jrnail23 commented Jun 2, 2023

I'll see what I can do to get you a repro, but the fact that useLocation (in at least some cases) returns pathname with the basename is the basis for the issue this PR is addressing, right?

@brophdawg11
Copy link
Contributor Author

@jrnail23 Not quite. The location kept inside the @remix-run/router, which is basically a mirror of window.location, contains the basename (to match window.location) and that is what is being sent, unstripped, to getKey.

@jrnail23
Copy link

jrnail23 commented Jun 2, 2023

Okay yeah, my bad @brophdawg11, I had it backwards.

@jrnail23
Copy link

jrnail23 commented Jun 2, 2023

As a suggestion, distinct TypeScript types for internal location vs. user location may be helpful to ensure the correct kind of location is used in the correct places. Specifically (and I know this may be something of a controversial topic in TS), nominal/branded/opaque types, particularly in your internal-only stuff go a long way toward this goal.

I use ts-essentials' Opaque type pretty extensively in my own apps, and I've been really happy with it.

@jrnail23
Copy link

jrnail23 commented Jun 2, 2023

It turns out that when I mentioned that I had to stripBasename myself was in an unstable_useBlocker callback -- those locations are not stripped.

@brophdawg11
Copy link
Contributor Author

unstable_useBlocker probably has the same bug as getKey - I'll check that one out too

@brophdawg11 brophdawg11 merged commit 08e2f76 into dev Jun 6, 2023
@brophdawg11 brophdawg11 deleted the brophdawg11/strip-basename-getkey branch June 6, 2023 20:23
@brophdawg11 brophdawg11 added the awaiting release This issue have been fixed and will be released soon label Jun 6, 2023
@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.14.0-pre.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 6.14.0 which includes this pull request. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@brophdawg11 brophdawg11 removed the awaiting release This issue have been fixed and will be released soon label Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants