feat: allow resolve() to accept pathnames with query strings and hash fragments#15458
Conversation
… fragments
- Add `ResolvablePath` type: `Pathname | ${Pathname}?${string} | ${Pathname}#${string}`
- Update `resolve()` and `resolveRoute()` signatures to accept `ResolvablePath`
- No runtime changes needed — `resolve_route()` already passes through `?` and `#`
- Enables `goto(resolve('/products?page=2'))` to satisfy both type checker and `svelte/no-navigation-without-resolve` lint rule
Closes sveltejs#14750
🦋 Changeset detectedLatest commit: 977191d 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 |
- Update resolve() in types/index.d.ts (was still using Pathname) - Update JSDoc template in client.js
teemingc
left a comment
There was a problem hiding this comment.
Thank you for submitting this. I've added a few comments to help align things with our current conventions. Other than that, it looks good to me.
elliott-with-the-longest-name-on-github
left a comment
There was a problem hiding this comment.
I love this, but it's missing one crucial bit. Because this only applies to pathnames, it means you can't do this:
resolve('/[slug]?foo=bar', { slug: 'banana' });...because route parameter notation like this falls under the RouteId type. I think I'd prefer if the ?{string} and #{string} variants applied to the RouteId version as well, though... that might be challenging. want to give it a crack?
Would that also be something like: export type RouteIdWithSearchOrHash = RouteId |
`${RouteId}?${string}` |
`${RouteId}#${string}` |
|
Yeah I think so, but I'm not sure how it would affect our ability to extract the types of the route parameters for the second argument... |
- Rename ResolvablePath to PathnameWithSearchOrHash
- Remove redundant ${Pathname}?${string}#${string} variant
- Add RouteIdWithSearchOrHash type so resolve('/[slug]?foo=bar', { slug: 'banana' }) works
- Update ResolveArgs to extract params from RouteId with search/hash suffix
- Shorten changeset text
The contributed solution really is quite brilliant 😄 https://github.com/sveltejs/kit/pull/15458/changes#diff-ca363a5801bb1240cce13fcdf36af610cfa8d5aae2007e77244a0d91f9ce5b4dR8-R12 I've tested it along with params that have matchers and it works well! |
Summary
Adds a
ResolvablePathtype that extendsPathnamewith optional?${string}and#${string}suffixes, allowingresolve()to accept pathnames with query strings and hash fragments.resolve_route()already passes through?and#correctlyPathnametype is unchanged (still used forpage.url.pathname)resolve()behavior with route IDs and params is unaffectedMotivation
Currently there is no way to use
goto()with query strings or hash fragments without either disabling the lint rule or bypassingresolve():resolve('/products?page=2')fails type-checking (Pathnamedoesn't include?)goto(resolve('/products') + '?page=2')fails thesvelte/no-navigation-without-resolvelint rule (argument isn't aresolve()call)Per @teemingc's suggestion in #14750:
This is a minimal type-only fix that doesn't conflict with #14756 (which adds richer
{ hash, searchParams }options).What this enables
Test plan
pnpm checkpassespnpm lintpassesCloses #14750