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

Remove dependency on svelte-preprocess #2391

Closed
benmccann opened this issue Jun 8, 2024 · 8 comments
Closed

Remove dependency on svelte-preprocess #2391

benmccann opened this issue Jun 8, 2024 · 8 comments
Labels
bug Something isn't working Fixed Fixed in master branch. Pending production release.

Comments

@benmccann
Copy link
Member

benmccann commented Jun 8, 2024

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 sveltejs/kit#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

"svelte-preprocess": "^5.1.3",

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

@benmccann benmccann added the bug Something isn't working label Jun 8, 2024
@jasonlyu123
Copy link
Member

jasonlyu123 commented Jun 8, 2024

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
Copy link
Member Author

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 😆 @sveltejs/enhanced-img and I think a couple custom user preprocessors might do that, but the fallback isn't going to help there

@dummdidumm
Copy link
Member

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
Copy link
Member Author

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

@dummdidumm
Copy link
Member

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

@benmccann
Copy link
Member Author

I just meant remove it from svelte-check

@jasonlyu123
Copy link
Member

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
Copy link
Member

sveltejs/svelte-preprocess#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.

dummdidumm added a commit that referenced this issue Jul 31, 2024
Removes the need for a dependency on svelte-preprocess in svelte-language-server, since that was all we used it for.

closes:
- #1683 and #1259, because the situation no longer arises (because we no longer have svelte-preprocess inside the VS Code extension)
- #2391
dummdidumm added a commit that referenced this issue Aug 19, 2024
Removes the need for a dependency on svelte-preprocess in svelte-language-server, since that was all we used it for.

closes:
- #1683 and #1259, because the situation no longer arises (because we no longer have svelte-preprocess inside the VS Code extension)
- #2391
@dummdidumm dummdidumm added the Fixed Fixed in master branch. Pending production release. label Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Fixed Fixed in master branch. Pending production release.
Projects
None yet
Development

No branches or pull requests

3 participants