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

Svelte language server forces too many tsconfig options #1976

Closed
dummdidumm opened this issue Apr 5, 2023 · 1 comment
Closed

Svelte language server forces too many tsconfig options #1976

dummdidumm opened this issue Apr 5, 2023 · 1 comment
Labels
breaking change bug Something isn't working

Comments

@dummdidumm
Copy link
Member

This came up in a conversation with @TehShrike - assume you have a project which you slowly want to convert from JS to TS. You have "allowJs": false and you use svelte-check - now you have a mismatch in reported errors, because the Svelte language server forces allowJs to true, which means JS file imports are followed, its types are picked up and used etc.

This got me thinking about the current list of things we force, and if we should no longer force some of them in the next major version of svelte-check (I feel like this should be seen as a breaking change).

These are the compiler options we currently force:

const forcedCompilerOptions: ts.CompilerOptions = {
    allowNonTsExtensions: true,
    target: ts.ScriptTarget.Latest,
    allowJs: true
    noEmit: true,
    declaration: false,
    skipLibCheck: true
}

allowNonTsExtensions

This is some internal compiler option - AFAIK we need this for basic functionality, but I'm not sure anymore.

target

The idea is to not allow CJS etc, but I'm no longer sure we should just flat out force it to latest. You might use globals in your .svelte files which do not exist in the version yet that you set in your tsconfig.json - target influences the default value of lib. Then again, something like Vite might auto-transpile it so it's fine. Not sure what's better.

allowJs

We need this so we can go to .svelte files from within TS files, get some typings etc. There may be one situation where we shouldn't set it though in my opinion: When running svelte-check with a referenced tsconfig.json. In that case, we're not in the context of an editor, so go-to etc is unimportant. We also should not blank svelte/types/runtime/ambient.d.ts in that case so the fallback ambient *.svelte module definition is picked up.
In the other cases it should be fine since the language server is only used within Svelte files, and the Typescript plugin doesn't override it so that described situation doesn't appear there.

noEmit

We need this because we're not building anything, just checking, and this avoids tsconfig warnings

declaration

Same as noEmit

skipLibCheck

I'm honestly not sure why we force this to true - should we just remove that?

@jasonlyu123 would love your thoughts on this.

@dummdidumm dummdidumm added the bug Something isn't working label Apr 5, 2023
@jasonlyu123
Copy link
Member

I think we can only add these when there isn't a tsconfig/jsconfig.json. And have a migration guide that points out some potential problems we can think of.

allowNonTsExtensions. I tested. If we don't have it, TypeScript will complain that it can't find the source file.

target Yeah. I don't think we should force this too. Maybe we might need some kind of diagnostic to tell people not to set it to ES5 or something. But we should only add it if we're certain there is a problem with it.

I think the cjs problem is with the "module"? But we currently don't force it to not be cjs.

skipLibCheck Yeah, I think we can remove it.

noEmit and declaration. We currently don't show the diagnostic of the config files. But maybe we should do it in svelte-check. But I think removing it makes sense. You'll see it in your editor if you open any ts files anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants