-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Allow chained optional parameters #7753
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
Conversation
π¦ Changeset detectedLatest commit: 7c91626 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
Note to self - replace the integration test with unit tests |
| if (param.matcher && !matchers[param.matcher](value)) { | ||
| // in the `/[[a=b]]/[[c=d]]` case, if the value didn't satisfy the `b` matcher, | ||
| // try again with the next segment by shifting values rightwards | ||
| if (param.optional && param.chained) { |
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.
What about /[[a=b]]/[[c=d]]/e? chained is only true for rest in that case right now
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.
It works, though in the process of double-checking that I found it gives you a false positive if you have a path like /not-b/d/e. Fixed in 7c91626
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.
Ah woops I misread the chained: true logic - but I guess that was a good thing in this case π
Closes #7548, alternative to #7549.
This is late night code β it's not exactly self-explanatory, but it works. I think. It might be possible to neaten it up a bit, but not now π΄
It solves the
/[[lang=lang]]/[...rest]case, where you want/some-pathto result in{ rest: 'some-path' }. Currently, that won't match anything, because the[[lang=lang]]param 'eats' the first segment even if it doesn't match, which violates intuitions about how this stuff should work.The solution is more general than just that case, however β any time an optional parameter with a matcher fails to match, we try it again with the next one by essentially shifting the regex matches rightwards.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0