[fix] resolve $lib alias when packaging

[dummdidumm]

Fixes the 90% case of #1950

Shortcomings of this solution:

• Uses a regex+string-replacement approach. Traversing an AST would be more robust
• only resolves the $lib alias for now. Other aliases should be resolved, too

These shortcomings should be adressed later

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 pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: b99df98

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	Patch

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

[ignatiusmb]

I'm a bit concerned about using string replacement for resolving module aliases, I know bundlers like rollup or esbuild handle these stuff differently, there must be a reason a separate plugin for alias and replace exists in rollup, or separate options in esbuild. One of the case I can think of is when a user have a code block with an import path that uses $lib, it will match the pattern and replace the path when it's not supposed to do so.

With that being said, I agree this should be sufficient for now while we try to figure out more ways to tackle this, and possibly in an asynchronous way as the "pause" is getting longer as the lib folder gets bigger. I know @dominikg has a prototype with using esbuild, but I think it's only for transpiling TS files and not Svelte.

[ignatiusmb]

What happens when full_import_path doesn't actually exists, for whatever reasons it may be (most likely a bad copy-paste or refactor)?

path.relative won't throw as long as it's a string, should we throw an error, leave it with the original alias in place, or use the non-existent resolved path (currently the case)?

[dummdidumm]

I think that's on the user then, the also things like svelte-check should be able find out. We don't check other file paths for correctness, so in my mind it would be inconsistent to check the aliases.

[ignatiusmb]

Maybe inline this ternary below to keep resolved as const?

[dummdidumm]

Fully agree that this is a hacky solution and I'd very much would like to leverage vite instead. There's a library mode but it works too different from what we want/need, as it bundles everything and will transform Svelte to JS, which we actively want to avoid.
If we do not want to use string replacements, we could also parse the script parts of the Svelte files - don't know which parsers are available to us though without adding additional dependencies.

[dominikg]

not entirely sure if it's helpful in this case but vite-plugin-svelte has 2 different features that may be combined to solve it.

1. there is experimental code that demonstrates how to use vite plugins as svelte preprocessors, this could be used to try and run vite's alias resolving during preprocess
2. vite plugins can contribute additional preprocessors via api.sveltePreprocess

So in theory you could build a vite plugin that searches for the other vite plugin it wants to run during svelte-preprocess, save that as local state in configResolved hook and then offer api.sveltePreprocess which uses the other vite-plugin and is picked up by vite-plugin-svelte.

I agree that a simple string replace of aliases is too dangerous. For the time being it may be best to recommend not using aliases for libraries build with sveltekit package command

[benmccann]

I think we'd probably want to handle $app as well:

kit/packages/kit/src/core/build/index.js

Line 172 in 6bd21c7

$app: path.resolve(`${build_dir}/runtime/app`),

It probably wouldn't be too hard to refactor out the code to set the config defaults then look at the actual value of kit.vite.resolve.aliases

I agree this is a bit dangerous, but it also seems better than not supporting it, so I'm a bit torn

I wonder if we need to do something as advanced as what @dominikg is suggesting. Could we just have a svelte-preprocessor that does a replacement using the AST? I don't think it would necessarily need to invoke Vite, which would make things quite a bit easier

[bluwy]

If we are able to use Vite to preprocess the components, we could take advantage of useVitePreprocess which removes the need for svelte-preprocess (for most cases). The caveat mentioned would be addressed soon (sveltejs/vite-plugin-svelte#173). Hopefully it could be a default for create-svelte one day.

[benmccann]

Ohhh. I did not understand from Domenik's comment or the API names that these methods were not calling svelte-preprocess. I thought it would be too complex to have both involved.

[dummdidumm]

I'm a little confused by the different solutions outlined in this PR. What we need is something that resolves any alias for .js, d.ts and .svelte files, with the additional difficulty that the generation of d.ts files is separate (vite or svelte-preprocess therefore can't hook into it). So the most robust solution to me sounds like "do alias resolving after having done everything else as a separate step", which means we need some parser that understands both TS and JS. In case of .svelte files, we can use Svelte's parse which includes parsing the JS part. In case of d.ts files, we can hopefully use TypeScript somehow. In case of .js files -> I don't know, since we can't ensure that people have TypeScript installed (they only need it in case of d.ts generation) - that we could change though to always require TypeScript when people want to build libraries.

I think we'd probably want to handle $app as well

$app is very different because it does not resolve to a file we know the location of ahead of time. I therefore would leave that out of this but I very much agree that we need a solution for it. Not being able use $-imports inside the library is limiting.

[bluwy]

I think it's fine to merge the PR as is with the current approach, we could look into other long-term solutions so we don't block others who need this feature. A documentation update noting the caveat would be nice too. Unless we want to ship a more robust experience to the end-users.