breaking: remove leading / from AssetPath and Path

[Rich-Harris]

While working on #16372 I had a realisation that we shouldn't prefix Asset and Pathname strings with a /. It makes no sense — these are intended to be relative to the base path, and so having them be root-relative (/favicon.png instead of favicon.png) is wrong.

This change means Pathname is the wrong name, since it wrongly implies a root-relative url.pathname. As well as removing the leading slash this PR renames Pathname to Path, and Asset to AssetPath. (This allows future us to add a new Asset type containing path: AssetPath, and possibly other stuff like type and size.)

There are practical benefits to removing the leading slash:

• In feat: add $app/manifest module #16372, we're adding an $app/manifest module with immutable and assets exports. The latter is an array of objects with a path property — rather than typing it as string, we can type it as AssetPath. But if it had a leading slash then it would be unsuitable for its main purpose, which is for use with cache.add(path) inside a service worker. We can either make it base-path-relative (as this PR proposes) or root-relative (i.e. /my-basepath/favicon.png) which is inherently less portable.
• Similarly, we can type the path elements in the prerendered array as Path rather than string
• We can more easily distinguish between resolve(path) and resolve(route_id), both type-wise and at runtime. At the moment they're sort of smushed together awkwardly

I also think that asset('favicon.png') is just nicer to look at than asset('/favicon.png') which is either weird or pointless depending on whether there's a configured base.

Anyway: this is all somewhat independent of the $app/manifest stuff so I figured it deserved its own PR.

Closes #16425, by tightening up the type of AssetPath (no string & {}).

---

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

• Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

[pkg-svelte-dev]

Install the latest version of @sveltejs/kit from 9956b0a:

pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/9956b0a3f9a410f71f28ed94e6dd9deff8a51cb4

Open in pkg.svelte.dev: https://pkg.svelte.dev/repos/kit/pr/16430

[changeset-bot]

🦋 Changeset detected

Latest commit: 9956b0a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Major

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

[svelte-docs-bot]

Preview: https://svelte-dev-git-preview-kit-16430-svelte.vercel.app/

[teemingc]

Closes #16430, by tightening up the type of AssetPath (no string & {}).

This PR is 16430. Did you mean another issue?

Also, the test type check is erroring because we can no longer do the asset('') hack to get the asset base path

[Rich-Harris]

Yep, I meant #16425. The check failure is annoying as I'd got rid of the red squigglies locally with /** @type {any} */ '', but apparently we need to be more invasive

[teemingc]

Should we keep this check but return early on pathname === ''? Because the trailing slash handling below it doesn't make sense for the root page

[Rich-Harris]

I think it's already correct — previously the root would become '/', but now it becomes '' for trailingSlash = 'never', '/' for always, and '' | '/' for ignore, which I think is what we want. if we reinstated this and returned early it would always be ''

[teemingc]

Mm you're right about that. But this also causes ResolvedPathname to accept '//' right now. At

kit/packages/kit/src/core/sync/write_non_ambient.js

Line 248 in 9956b0a

'\t\tResolvedPathname(): `${"/" | `/${string}/`}${ReturnType<AppTypes[\'Path\']>}`;',

it's formed by '/' plus the values of Path so we need to adjust that logic instead

[Rich-Harris]

I think that's probably unavoidable, sadly, because of /${string}/. It's already like this today

[Rich-Harris]

(but if there's a fix I'm missing we can always patch later)

[teemingc]

Ah right. I'll just open an issue

[teemingc]

Looks good. Just one outstanding thought

[elliott-with-the-longest-name-on-github]

Is there no good way to share the logic between client and server here? Weird to have this specific pathname handling duplicated

[Rich-Harris]

not really, no. one uses shared global state, one needs AsyncLocalStorage. you could maybe try and tease bits of them apart to share implementations but it would end up looking worse I think

[nickolay]

This change means Pathname is the wrong name, since it wrongly implies a root-relative url.pathname

Yet, resolve() still takes a PathnameWithSearchOrHash, makes multiple references to the pathname in its documentation, and conflicts with the standard definition of resolving relative references on the web.

It is also very weird that if you resolve a Path (without params, e.g. resolve('blog/hello-world');) you omit the leading slash, but if you resolve a RouteId (resolve('/blog/[slug]', {..}) you suddenly need it. Why doesn't the argument for "having them be root-relative (/favicon.png instead of favicon.png) [being] wrong" apply to the second form? (I know the technical answer now, but consider the user's perspective!)

Even worse, for simple routes you can use both, yet the migrator removes the leading / leading to unnecessary churn. For some reason it keeps resolve("/") alone though (update: sveltejs/cli#1268), which fails the type check when you have the root page in routes/(area)/+page.svelte. Trying to read the docs to figure what the heck is going on does not help at all: "Resolve a pathname by prefixing it with the base path, if any, or resolve a route ID by populating dynamic segments with parameters. During server rendering, the base path is relative and depends on the page currently being rendered" (I've highlighted the terms that make zero sense to someone who haven't already read the code.)

I also think that asset('favicon.png') is just nicer to look at than asset('/favicon.png')

Maybe, but what about <a href={resolve('')}> ? What about resolve('posts')?

The SK2 version might have been confusing to those who used the advanced functionality. The SK3 version, while technically superior, is confusing to those who don't know about base URL.

In case you're wondering why am I using resolve() at all — it's because eslint default set includes no-navigation-without-resolve. (That page needs an update explaining how SK2 and 3 are different in what they require the user to do...)

---

Anyway, what's the rush with requiring everyone to remove SK2-style pathnames from their resolve() calls? This PR is 1 month old, there have been no deprecations in SK2, and there's no way to prepare SK2 code so that the 2->3 migration doesn't have to change it all at once, which makes it harder to deal with branches — all for unclear gain.

[elliott-with-the-longest-name-on-github]

@nickolay if you want to discuss this please open an issue! Merged PRs are not a good place to talk 😄