-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
breaking: remove leading / from AssetPath and Path
#16430
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
Changes from all commits
23f7e77
1ef146b
4069cb9
c95c354
fdf6cd0
78364f8
2a7e2a7
e7e457c
6151153
0420ce7
2b886a8
a8f0674
e43a181
836a445
41aa08d
2fad891
1171076
9956b0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@sveltejs/kit': major | ||
| --- | ||
|
|
||
| breaking: rename `Pathname` type to `Path` and `Asset` to `AssetPath` | ||
| breaking: remove leading `/` from `Path` and `AssetPath` |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there no good way to share the logic between client and server here? Weird to have this specific pathname handling duplicated
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not really, no. one uses shared global state, one needs |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,23 +1,15 @@ | ||
| import { | ||
| PathnameWithSearchOrHash, | ||
| RouteId, | ||
| RouteIdWithSearchOrHash, | ||
| RouteParams | ||
| } from '$app/types'; | ||
| import { RouteId, RouteParams } from '$app/types'; | ||
|
|
||
| type StripSearchOrHash<T extends string> = T extends `${infer Pathname}?${string}` | ||
| ? Pathname | ||
| : T extends `${infer Pathname}#${string}` | ||
| ? Pathname | ||
| type StripSearchOrHash<T extends string> = T extends `${infer U}?${string}` | ||
| ? U | ||
| : T extends `${infer U}#${string}` | ||
| ? U | ||
| : T; | ||
|
|
||
| export type ResolveArgs<T extends RouteIdWithSearchOrHash | PathnameWithSearchOrHash> = | ||
| T extends RouteId | ||
| ? RouteParams<T> extends Record<string, never> | ||
| export type ResolveArgs<T> = T extends `/${string}` | ||
| ? StripSearchOrHash<T> extends infer U extends RouteId | ||
| ? RouteParams<U> extends Record<string, never> | ||
| ? [route: T] | ||
| : [route: T, params: RouteParams<T>] | ||
| : StripSearchOrHash<T> extends infer U extends RouteId | ||
| ? RouteParams<U> extends Record<string, never> | ||
| ? [route: T] | ||
| : [route: T, params: RouteParams<U>] | ||
| : [route: T]; | ||
| : [route: T, params: RouteParams<U>] | ||
| : [never] | ||
| : [pathname: T]; |
Uh oh!
There was an error while loading. Please reload this page.
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.
Should we keep this check but return early on
pathname === ''? Because the trailing slash handling below it doesn't make sense for the root pageThere 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.
I think it's already correct — previously the root would become
'/', but now it becomes''fortrailingSlash = 'never','/'foralways, and'' | '/'forignore, which I think is what we want. if we reinstated this and returned early it would always be''Uh oh!
There was an error while loading. Please reload this page.
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.
Mm you're right about that. But this also causes ResolvedPathname to accept
'//'right now. Atkit/packages/kit/src/core/sync/write_non_ambient.js
Line 248 in 9956b0a
'/'plus the values ofPathso we need to adjust that logic insteadThere 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.
I think that's probably unavoidable, sadly, because of
/${string}/. It's already like this todayThere 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.
(but if there's a fix I'm missing we can always patch later)
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 right. I'll just open an issue