-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Fix inadvertent support for partial dynamic parameters #9506
Fix inadvertent support for partial dynamic parameters #9506
Conversation
🦋 Changeset detectedLatest commit: db7a8e6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
<NullRenderer routes={routes} location={{ pathname: "/three" }} /> | ||
<NullRenderer | ||
routes={routes} | ||
location={{ pathname: "/three", search: "", hash: "" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lack of search/hash in this test case was causing weird console.warn
of /threeundefinedundefined
`always follow a \`/\` in the pattern. To get rid of this warning, ` + | ||
`please change the route path to "${path.replace(/\*$/, "/*")}".` | ||
); | ||
path = path.replace(/\*$/, "/*") as Path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same logic as we use in compilePath
for partial splats
.replace(/:(\w+)/g, (_, key: PathParam<Path>) => { | ||
.replace(/^:(\w+)/g, (_, key: PathParam<Path>) => { | ||
invariant(params[key] != null, `Missing ":${key}" param`); | ||
return params[key]!; | ||
}) | ||
.replace(/\/:(\w+)/g, (_, key: PathParam<Path>) => { | ||
invariant(params[key] != null, `Missing ":${key}" param`); | ||
return `/${params[key]!}`; | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generatePath
does 2 separate checks here to avoid a nasty regex to differentiate :param
from /thing/:param
.replace(/:(\w+)/g, (_: string, paramName: string) => { | ||
.replace(/\/:(\w+)/g, (_: string, paramName: string) => { | ||
paramNames.push(paramName); | ||
return "([^\\/]+)"; | ||
return "/([^\\/]+)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only match params that are at the start of a URL segment. We do not need to handle ^:
here since line 693 ensures our regexpSource
always has a leading slash by this point. So we just only collect params when they come immediately following a slash
For matching types in TS, I think it'd be good to replace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks, @brophdawg11
@brophdawg11 I'm looking at your suggested workaround for when this functionality is required, but I don't understand how the workaround would work with the rest of the Routing system. E.g.: I want to "route" requests to According to your workaround, I need to create a route for |
If you're using let router = createBrowserRouter([{
path: ":username",
loader({ params }) {
if (!params.username.startsWith('@')) {
throw new Response("Not Found", { status: 404 });
}
let user = await getUser(params.username);
return { user };
},
element: <ProfilePage />,
errorElement: <NotFound />
}]); If you're using // <Route path=":username" element={<GuardedProfilePage />} >
function GuardedProfilePage() {
let params = useParams();
if (!params.username.startsWith("@")) {
return <NotFound />
}
return <ProfilePage username={params.username} />
} |
@brophdawg11 Thanks. The use-case I'd like to solve is this:
Now, this is currently not possible, because So the next step would be:
This would mean |
The second example above is what you want, instead of splitting that logic across two ambiguous routes, you just fork in the // <Route path=":userParam" element={<ProfilePageElement />} />
function ProfilePageElement() {
let { userParam } = useParams();
return userParam.startsWith("@") ? <ProfilePage /> : <Fallback>;
} |
Needed for the react-router update because of [ #9506 - Fix inadvertent support for partial dynamic parameters](remix-run/react-router#9506). JIRA Ticket: MB-15974 Co-authored-by: Kyle Hill <kyle@truss.works>
Needed for the react-router update because of [ #9506 - Fix inadvertent support for partial dynamic parameters](remix-run/react-router#9506). JIRA Ticket: MB-15974 Co-authored-by: Kyle Hill <kyle@truss.works>
The intention was never to match partial dynamic or splat parameters, but we had some bugs and inconsistencies in the implementation:
matchPath
would not match partial splat parameters (/prefix*
), and would warn + treat them as/prefix/*
generatePath
would match partial splat parametersmatchPath
/generatePath
would match partial named parameters (/prefix:param
)This PR removes the buggy support for partials matching such that we always require dynamic/splat params to be an entire URL segment. This will simplify our future plans to support optional params as well as the intended optimization of the path-matching algorithm.
If an application happened to be relying on this, then the suggestion should be to extract the static prefix at the
useParams
call-site:Replaces #9238