sveltejs / language-tools

The Svelte Language Server, and official extensions which use it
MIT License
1.22k stars 195 forks source link

Remove dependency on `svelte-preprocess` #2391

Closed benmccann closed 4 weeks ago

benmccann commented 3 months ago

Describe the bug

This dependency is unnecessary. If the user doesn't have it installed in their project, then they aren't using it and there's no reason for svelte-check to add it. This is one of multiple issues contributing to https://github.com/sveltejs/kit/issues/12258. But even without that issue, the dependency is just adding extra weight we don't need in most projects. Even most TypeScript-enabled SvelteKit projects don't use it as they generally use vite preprocess from v-p-s

Reproduction

https://github.com/sveltejs/language-tools/blob/c18615292110d7a945547a71609c4af03f971484/packages/svelte-check/package.json#L32

Expected behaviour

Try to dynamically import it instead

System Info

N/A

Which package is the issue about?

svelte-check

Additional Information, eg. Screenshots

No response

jasonlyu123 commented 3 months ago

svelte-preprocess is used as a fallback when there isn't a preprocess config or config can't be loaded so I think this would be a breaking change. One example is that the preprocess config is directly passed into the svelte plugin in the vite.config.js/rollup.config.js instead of in the svelte.config.js

benmccann commented 3 months ago

svelte-preprocess is used as a fallback when there isn't a preprocess config or config can't be loaded so I think this would be a breaking change

Yeah, potentially. Might not be bad to do with the next major whenever that is

One example is that the preprocess config is directly passed into the svelte plugin in the vite.config.js/rollup.config.js instead of in the svelte.config.js

I've never seen anyone do that for TypeScript :laughing: @sveltejs/enhanced-img and I think a couple custom user preprocessors might do that, but the fallback isn't going to help there

dummdidumm commented 3 months ago

I'm wondering why we're having this dependency at all and are not bundling with the other packages. Was it because of some transitive dependency not playing ball? Can't remember.

benmccann commented 3 months ago

But do we need to bundle it or can we just remove it?

dummdidumm commented 3 months ago

We can't really remove it because it's a dependency of svelte-language-server, which is also powering the VS Code extension.

benmccann commented 3 months ago

I just meant remove it from svelte-check

jasonlyu123 commented 3 months ago

I'm wondering why we're having this dependency at all and are not bundling with the other packages.

It might be because of the dynamic import in svelte-preprocess.

svelte-language-sever is bundled by rollup so we should be able to remove it but we'll need to adjust the useFallbackPreprocessor to handle the module not found error. It might also make sense to add a step to try loading the vite preprocess as the fallback.

dummdidumm commented 2 months ago

https://github.com/sveltejs/svelte-preprocess/issues/643 showed that the removal of the import preprocessor has affected more people than I imagined. I'm wondering what that means for our fallback preprocessor with regards to intellisense etc., and whether or not it should become a optional peer dep in svelte-check instead of a dependency.