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

fix: do not preserve type imports (WIP) #20

Merged
merged 4 commits into from
Apr 5, 2021

Conversation

SomaticIT
Copy link
Contributor

@SomaticIT SomaticIT commented Mar 27, 2021

Hello,

I'm trying this plugin on a real-world project and the first thing to say is, it works very well, what a developer experience!

I noticed something that when I enable this plugin on a vite project (even without any svelte files): it breaks a lot of TypeScript modules because it preserve type imports.

Here is a little reproductible scenario:

// lib.ts
export interface Test {
    arg?: boolean;
}

export function test(): void {
    console.log('la');
}

// index.ts
import { test, Test } from './lib';

main();

export function main({ arg = true }: Test = {}): void {
    if (arg) test();
}

If you compile this app with vite without @sveltejs/vite-plugin-svelte, it works as expected, the result would be:

function test() {
    console.log('la');
}

main();

function main({ arg = true } = {}) {
    if (arg) test();
}

If you enable @sveltejs/vite-plugin-svelte, this will break with one of the following Error:

- runtime (dev): Uncaught SyntaxError: The requested module '/lib.ts' does not provide an export named 'Test'
- compile-time (build): Non-existent export 'Test' is imported from /lib.ts

You can reproduce this error by installing the playground I embedded in this PR


After a first inspection, I noticed that this plugin enforce the importsNotUsedAsValues Typescript compiler option with value preserve. This force TypeScript users to separate type imports from value imports using the strict syntax: import type. It can be a real problem on large TypeScript codebases because it force the developer to import its modules twice.

I'm not sure if there is a good reason about this but when I comment this line, it does not impact my project and it resolves this issue on all .ts files of my project (read below about .svelte files).

Important Note

I also noticed that the same issue occured on .svelte files using this plugin. This is not the case using the rollup plugin. So when I switch a project (with a lot of TypeScript) from rollup-plugin-svelte to vite-plugin-svelte, the project breaks because of type imports.

This fix does not resolve the issue inside .svelte files. I'm not sure if this plugin is responsible of this or if the issue comes from svelte-preprocess.

Any idea?

I will investigate on this and give you more feedbacks.

Thanks again

@dominikg
Copy link
Member

Thanks, this is a great PR. That option should not be set by default.
It is only needed when using esbuild for preprocessing ts to js in .svelte components via the experimental useVitePreprocess option.

@SomaticIT
Copy link
Contributor Author

This was my initial thoughts.

Do you think we could enable this option only during the preprocessing?

@dominikg
Copy link
Member

It is possible if you use a separate preprocessor like @lukeed s https://github.com/lukeed/svelte-preprocess-esbuild the idea behind useVitePreprocess is to reuse existing vite capabilities tough. During dev it may be possible to use transformWithEsbuild of the devserver, but the preprocessor has to be run at build time too.

If you have any ideas for this i'd be happy to hear about it. But until then i'm going to remove that option from defaults.

This is the esbuild release that added the preserve option specifically for sveltes usecase
https://github.com/evanw/esbuild/releases/tag/v0.8.28

@dominikg
Copy link
Member

dominikg commented Apr 4, 2021

added a testcase and enabled the extra esbuild option for useVitePreprocess: true only. Does that work for you @SomaticIT ?

@dominikg dominikg merged commit 8d9ef96 into sveltejs:main Apr 5, 2021
@SomaticIT
Copy link
Contributor Author

I tried on my projects and it seems to work well.

Thanks

@SomaticIT SomaticIT deleted the @fix/dont-preserve-types branch April 5, 2021 16:53
@dominikg
Copy link
Member

dominikg commented Aug 2, 2021

heads up @SomaticIT , a change has landed on vite main branch that will be released with vite 2.5.0 which starts to read/use tsconfig.json and use it for esbuild. PR on vite: vitejs/vite#4279

you can try to use https://github.com/fwouts/rollup-plugin-friendly-type-imports or you'll have to switch to using import type for interfaces. (i suggest the latter).
The testcase you've added here is still going to stay but it'll be using import type.

@github-actions github-actions bot mentioned this pull request Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants