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.

Sign up for GitHub

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

Jump to bottom

Support base64 encoded ids on nested routes #8291

Merged
merged 2 commits into from
Dec 9, 2021
Merged

Support base64 encoded ids on nested routes #8291

merged 2 commits into from
Dec 9, 2021

Conversation

mikeldking
Copy link
Contributor

@mikeldking mikeldking commented Nov 9, 2021 •
edited
Loading

Resolves #8288

This PR modifies compilePath's non-capturing group to match up to the proceeding / so that paths with base64 encoded ids (e.x.users/RGFzaGJvYXJkOjE1Nzk=/edit when using graphql-relay globalIds) properly match up to the / and not at the word break before the = character.

@mikeldking mikeldking mentioned this pull request Nov 9, 2021
Closed
@mikeldking mikeldking changed the title Failing test for base64 encoded id on nested routes Support base64 encoded ids on nested routes Nov 9, 2021
@mikeldking mikeldking force-pushed the 8288-nested-routes-w-regex branch from 71b74da to f56b142 Compare November 9, 2021 07:41
@mikeldking
Modify the non-capturing group at the end of compilePath to include t…
fd83937 
…he proceeding /

This is necessary for paths that include base64 included ids such as relay globalIds
(e.x. users/RGFzaGJvYXJkOjE1Nzk=/edit)
@mikeldking mikeldking force-pushed the 8288-nested-routes-w-regex branch from f56b142 to fd83937 Compare November 9, 2021 07:47
@mjackson
Copy link
Member

mjackson commented Nov 9, 2021

Thanks for the PR, @mikeldking! I'll review this today.

Just making a note for myself here: I think the word boundary check should come when we know we are replacing a :id-style URL param. So I'm going to try making that adjustment myself when this gets merged.

@mikeldking
Copy link
Contributor Author

Thanks for taking a look @mjackson ! Looking forward to contributing.

Agreed on the boundary check - doesn't need to be there when the path is terminating in a :id style param. Went for the smallest change that satisfied the use case for not matching /home2 for /home but also matching /users/:id/edit for /users/Axy=/edit.

@iMoses-Apiiro
Copy link

seems like this would also solve #7987

@chaance chaance changed the base branch from main to dev November 11, 2021 01:14
@mikeldking
Copy link
Contributor Author

Hey @mjackson @chaance super gentle nudge on this PR! Excited about the news w/ remix-run! I'm currently working around this bug by adding a * at the end of my routes to avoid the over-eager non-matching group. Very excited to be an early adopter of the new router and more than happy to make adjustments to this PR as needed! Thanks again for your work.

@ryanflorence ryanflorence merged commit a0a5ea4 into remix-run:dev Dec 9, 2021
@ryanflorence
Copy link
Member

Thanks @mikeldking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjackson mjackson mjackson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Nested routes under an id with a trailing = fail to match
4 participants
@mikeldking @mjackson @iMoses-Apiiro @ryanflorence