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
feat!: ignore queryparams in matching #61
base: v2
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests β
Additional details and impacted files@@ Coverage Diff @@
## v2 #61 +/- ##
==========================================
+ Coverage 95.63% 95.66% +0.02%
==========================================
Files 4 4
Lines 435 438 +3
Branches 84 84
==========================================
+ Hits 416 419 +3
Misses 19 19 β View full report in Codecov by Sentry. |
@@ -85,6 +85,9 @@ function _matchRoutes(path: string, table: RouteTable): RadixNodeData[] { | |||
// Order should be from less specific to most specific | |||
const matches = []; | |||
|
|||
// Ignore query parameters | |||
path = path.split("?")[0]; |
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.
We need to use index check (like https://github.com/unjs/h3/blob/main/src/router.ts#L86) to handle URL like /login?returnTo=/path?foo=bar
(context: unjs/h3#190)
Thanks for PR dear @kingtimm and sorry it is delayed. Main reason is that, the consumer libs (mainly unjs/h3) are already doing this normalization. It is not a breaking-change per-se but doing this means performance overhead of iterating string twice. Because of this i plan this in next major release to be included so we can adjust h3 router behavior while upgrading. |
π Linked issue
fixes #54 - inclusion of queryparameters on a path make it fail to match.
β Type of change
π Description
Removed query parameters from the path before it is matched. The router did not seem to support them anyway.
My use-case was in nuxt-vitest's registerEndpoint but found others running into it.
Resolves #54
π Checklist