-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Optional route with matcher followed by rest parameters #7548
Comments
Added the bug label too soon - I think this works as expected. The matcher works greedily from left to right. The matcher says "home" isn't a valid language, so the rest of the route is not taken into account, because the rest is a, well, rest route. To me that's the intuitive behavior. In your case you probably want to only have a |
That was my opinion as well until I noticed that placing a static path after the optional one would make it work differently – which IMO makes it inconsistent. One way of looking at it is: if the matcher says "home" isn't a valid language it should not take into account anything inside. This is pretty much the scenario from the docs, but using a What seems buggy is that it does look inside if it's a static route, but it doesn't if it's a rest route, as shown in the two repros above. |
Thinking about this again, I lean more towards "works as designed" and "limitation of the Regex approach". The route is matched using a Regex, and that regex works greedily. In this situation on optional route segment is followed by other optional segments, so the Regex will always assign the first segment to the optional parameter, and then continue to look at the matcher. Doing it this way ensure route matching is very fast even for huge apps, and the greedy nature of it makes it more straightforward to reason about in your head. This specific case should probably mentioned in the docs though. |
Document a gotcha around optional parameter with matcher followed by rest parameter Closes #7548
@dummdidumm thank you for the clarification. I never thought of the On the other hand, this seems a very basic use case for any multilingual website that has all pages generated by a CMS. |
|
That's right. I see that we have access to params on every load function, even in a layout outside. Even tho I understand the limitation of the greedy Regex approach, it still feels a bit inconsistent when taking into account static routes. Feel free to close the issue if this is indeed working as designed. Thank you for all the support :) |
* add failing test for #7548 * refactor * fix * always add trailing slash, that logic is way out of date * fix tests * DRY out * track more param info * check for missing matcher at manifest creation time * working, i think * shuffle around * ok try this * update tests * fix tests * lint * fix * changeset * unit tests * make the code a bit more comprehensible * Update .changeset/forty-icons-switch.md * fix another case
Describe the bug
Given a
[[lang=lang]]/[...path]
route, kit will fail to catch non-matched URLs with the rest parameters route.The same doesn't happen if the optional route is followed by a static one:
[[lang=lang]]/t/[...path]
.I would expect:
In the
[[lang=lang]]/t/[...path]
scenario, it works as expected:A common use case for this would be an i18n site with dynamic pages being served by a CMS.
Reproduction
Here's a stackblitz reproducing the issue:
https://stackblitz.com/edit/sveltejs-kit-template-default-4srbff?file=src/routes/+layout.svelte
And here's another one showing the correct expected behaviour by placing a static route after the optional one:
https://stackblitz.com/edit/sveltejs-kit-template-default-a3vmdd?file=src/routes/+layout.svelte
Logs
No response
System Info
Severity
serious, but I can work around it
Additional Information
No response
The text was updated successfully, but these errors were encountered: