Skip to content
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

Resolve aliases while running svelte-kit package #1950

Closed
kwangure opened this issue Jul 19, 2021 · 21 comments
Closed

Resolve aliases while running svelte-kit package #1950

kwangure opened this issue Jul 19, 2021 · 21 comments
Labels
feature / enhancement New feature or request pkg:svelte-package Issues related to svelte-package
Milestone

Comments

@kwangure
Copy link
Contributor

Describe the problem

Aliases are currently not resolved when packaging since components are only preprocessed.

For example, importing $lib/css/shared.css from $lib/components/Component/SubComponent involves a lot of ../ without aliases. This is very brittle especially considering something like $lib/css/shared.css is something that might be shared across many components at different nested levels.

Describe the proposed solution

It would be helpful if svelte-kit resolved these aliases.

My proposal around Svelte files would be to create a built-in preprocessor that uses plugin-alias since this is already svelte-kit dependency.

It's not immediately clear what the approach to resolving aliases in js files should be because svelte-kit currently avoids any bundling and just copies them. I haven't used Typescript, so I don't know that tsc aliases are compatible with plugin-alias.

Alternatives considered

Not using aliases in any files that will be packaged (my current approach).

Importance

would make my life easier

Additional Information

No response

@ignatiusmb
Copy link
Member

Would this apply to only Svelte files or TS and JS too?

@dummdidumm
Copy link
Member

I think the desired behavior is resolve the alias in Svelte/TS/JS files.

@jonathangreenemeier-vizio

This would be very helpful when using svelte-kit to publish components and component libraries. It's good to know we can remove the aliases in the meantime to get it working.

@dummdidumm
Copy link
Member

dummdidumm commented Sep 16, 2021

Since TS does not do the resolving itself, we have to implement/add it ourselves somehow.
Options:

  • use https://www.npmjs.com/package/tsconfig-paths somehow. This would read the paths from the tsconfig and resolve them. This would add another dependency required to install (~90kb). This would also be somewhat inconsistent because it would deal with aliases by reading the tsconfig, not by reading the Vite config.
  • access the resolve.alias config of Vite to get the aliases. When preprocessing TS, it might be possible to write a transformer which will resolve these - but I'm not sure how to pass this into svelte-preprocess. When not preprocessing Svelte, I'm not sure how to do this other than either also running it through TS. The quick fix would be to use a hacky regexp. Or parse the Svelte code result into a Svelte AST which will use Acorn on the JS part which will give us the correct import locations.

I prefer the latter.

Things are further complicated by the fact that the alias is resolved relative to the root (src/..) but the output files need to reference each other relative to the lib root (src/lib by default).

@jonathangreenemeier-vizio

I wasn't super happy with the potential workaround of removing all the $lib/ imports so wrote a little script to rewrite these imports in the packaged code. I thought I would share this in case anyone else finds it useful while waiting for svelte-kit package to become less experimental/new: https://github.com/6eDesign/svelte-calendar/blob/master/scripts/postpackage.js

I'm running this as a postpackage npm script (where a package script simply invokes svelte-kit package). If you are using a non-default output directory (something other than package/) you can specify that folder when invoking the script, eg: PACKAGE_FOLDER=my-package-folder node scripts/postpackage

I will note that I ran into other issues beyond transforming the $lib imports. My approach to exporting components is to create an index.js file which package.main points to that exports an object containing all components I want to expose (ex: https://github.com/6eDesign/svelte-calendar/blob/master/index.js)

The problem I ran into is that this file gets transformed into an index.d.ts file and updating package.main to point to this new ts file does not seem to work correctly (components won't load in REPL/elsewhere). I didn't dig too much into this and instead opted to rename the index.d.ts file back to index.js which is working well for me right now.

@dummdidumm
Copy link
Member

This sounds unrelated. Anyway: the d.ts file is a type definition file which has no runtime equivalent. The index.js should live next to it (it should get copied across automatically) and main should point to that js file.

@jonathangreenemeier-vizio
Copy link

jonathangreenemeier-vizio commented Sep 17, 2021

I understand that may have been the intention of the d.ts file but when I run svelte-kit package the d.ts file is a verbatim copy of the index.js file and the index.js file does not live alongside it in the package/ directory. You can try this for yourself in the linked repository above.

Sorry @dummdidumm - I may have misunderstood: what are you saying sounds unrelated?

dummdidumm pushed a commit that referenced this issue Sep 17, 2021
@dummdidumm
Copy link
Member

The index.js issue is unrelated. I looked at the repository, and the index.js is defined at the root of the repo, but it should be at src/lib, the root of your library. If you put it there (and update the imports accordingly), the index.js and the generated index.d.ts should end up in the packaging build output folder.

dummdidumm added a commit that referenced this issue Sep 21, 2021
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
@dummdidumm
Copy link
Member

dummdidumm commented Sep 24, 2021

The $lib alias (no others currently) is now resolved with the caveat that it's a regexp-based string-replacement, so there might be false positives.
Keeping this issue open as this is obviously not the final solution, but it should help for the majority of cases.

@josdejong
Copy link

Thanks @dummdidumm, that works very well!

@Rich-Harris Rich-Harris added the bug Something isn't working label Mar 6, 2022
@Rich-Harris Rich-Harris added this to the post-1.0 milestone Apr 26, 2022
@bertybot
Copy link
Contributor

I would very much like to work on this problem, but I think it exposes a complication with this package approach in Svelte. Basically the heart of this problem is how can we compile the svelte without actually compiling the svelte.

So far we have a few approaches.

  1. Svelte Preprocess: I do think this is a very good suggestion since svelte-preprocess seems to address this very problem. However, the problem with this is that ts and js files aren't captured by this, only svelte files. (maybe a rollup config that only resolves ts and js files???)
  2. Really just roll our own like @dummdidumm did. Like, I get the concerns with it not being fool proof, but I really can't see another way than this without creating our own weird svelte compiler or rollup plugin. (which would probably just do the same thing)

Both of these are alright approaches but, there are downsides with both that keep them from being bullet proof.

Here are a few approaches I have considered.

  1. Making package an adapter. Like really if you think of it isn't a package a deployment target? Idk seems like a logical way to go about it in the whole philosophy of Sveltekit. Really problematic though since package is really more of a separate deployment in general, instead of a target, and also all the Svelte is already compiled JS. Idk just thought this was a fun thought experiment that leads into my second.
  2. Big thing we are missing is just a build step, a rollup step where we compile things. Right now we are kind of slowly writing our own version of rollup for this use case. Maybe the solution is to finally add rollup to the package build step. However, the problem with this is what I said at the beginning we need to compile svelte without running the svelte compiler. So we'd have to write our own rollup plugin for this weird use case which brings us right back to step 1.

Idk these are just a few of the possible solutions I came up with. Any other suggestions? What do you guys think is the best approach? I think adding a rollup build step to this might be the way to go. Then we could also do like a dev server setup too which would be nice.

@stanf0rd
Copy link

When I try to import something from src/lib/index.ts using just $lib:

import { foo } from '$lib';

path does not resolve. I need to explicitly write:

import { foo } from '$lib/index';

So, such thing

    import { foo } from '$lib/index';
    import { bar } from '$lib';

results to that:

    import { foo } from '../../index'; // correct
    import { bar } from '$lib';        // incorrect

@dummdidumm
Copy link
Member

Since we have kit.alias now this might become a little easier.

@BeeMargarida
Copy link
Contributor

Is it working for no $lib aliases, for example with something like $clib? I'm currently trying with this, but when I run the package command, all $clib mentions are not being replaced

@dummdidumm
Copy link
Member

Not currently which is why this issue is still open

@BeeMargarida
Copy link
Contributor

Not currently which is why this issue is still open

Thank you! Do you know if there is any hack to allow this for now?

@dummdidumm
Copy link
Member

Thanks to @BeeMargarida this now works with all aliases set through kit.alias . Since this is now its own package, the question is whether or not we regard this as completely solved, or if we should also try to read the vite config and resolve aliases from there, if a svelte.config.js is not present, or (because that's likely very cumbersome) we should make our lives easier by introducing a config option similar to kit.alias inside the package options.
Either way, this has low priority since in the vast majority of cases you probably use it together with SvelteKit.

@dummdidumm dummdidumm added p3-edge-case SvelteKit cannot be used in an uncommon way and removed p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. bug Something isn't working labels Aug 29, 2022
@BeeMargarida
Copy link
Contributor

we should make our lives easier by introducing a config option similar to kit.alias inside the package options. Either way, this has low priority since in the vast majority of cases you probably use it together with SvelteKit.

Hi! I could implement this as well. Regarding the kit.alias inside the package options, these ones always take priority over the ones in svelte.config, if both are present?

@dummdidumm
Copy link
Member

dummdidumm commented Aug 30, 2022

In general the package options take priority over the kit options.

I'd be awesome if you implement this, but I fear it either brings some complexity with it or people are confused that they have to configure aliases in multiple places. Right now @sveltejs/package is independent of Vite, so we

  • either need to connect it to Vite somehow, which brings complexity into the package
  • or keep it indepent, so package.alias would only be concerned with resolving the aliases while bundling, and not make them available as tsconfig paths and/or Vite aliases. This would be easy to implement but might fall short of expectations

I'm not sure what's the best path forward, so I would like to gather feedback first how people really use @sveltejs/package and if it's even worth it to add such a feature, given that probably 90% of users will use it in conjunction with SvelteKit, where everything works as expected thanks to you already.

@BeeMargarida
Copy link
Contributor

I'm not sure what's the best path forward, so I would like to gather feedback first how people really use @sveltejs/package and if it's even worth it to add such a feature, given that probably 90% of users will use it in conjunction with SvelteKit, where everything works as expected thanks to you already.

Makes total sense, better to wait for feedback and user request than develop prematurely 👍 Thank you!

@benmccann
Copy link
Member

Closing this since it was implemented in #6350. If folks want to specify aliases in the config elsewhere such as package.alias, vite.alias, or alias because they're using the package command without using SvelteKit then that's something we could consider as a separate feature request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature / enhancement New feature or request pkg:svelte-package Issues related to svelte-package
Projects
None yet
Development

No branches or pull requests

10 participants