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

Feature: type-checking #205

Closed
sharifzadesina opened this issue Jul 23, 2020 · 13 comments
Closed

Feature: type-checking #205

sharifzadesina opened this issue Jul 23, 2020 · 13 comments
Labels
question Further information is requested

Comments

@sharifzadesina
Copy link

sharifzadesina commented Jul 23, 2020

Hi,
Unlike ts-loader, svelte-preprocess doesn't respect noEmitOnError compiler option, it seems right now we only report syntax errors and we ignore all other errors.
like if you do this:

let myvar: string = "hi";
myvar = [];

You will get no error.
But for normal Typescript files ts-loader, will report the error and will prevent files to be compiled.

@kaisermann
Copy link
Member

kaisermann commented Jul 23, 2020

svelte-preprocess doesn't do any kind of type-checking, it just transpiles your ts into js (see it here). If you want to fail your build when a type error is found, you can use svelte-check. You can use the VSCode implementation while developing and use the CLI in CI.

@sharifzadesina
Copy link
Author

sharifzadesina commented Jul 25, 2020

@kaisermann Why you just closed the issue?
Of course, I am aware of svelte-check, but it is not possible to use it alongside preprocess, you need to run it manually each time, or use a language-server and an editor that supports that language server.
but ts-loader does the type checking itself and prevents code from compiling. (You know what noEmitOnError does? and why it even exists?)
I don't understand why you guys don't follow the standard behavior of typescript tools. what's the point of using typescript if I am going to type check in CLI?

@kaisermann
Copy link
Member

kaisermann commented Jul 25, 2020

@sharifzadesina From a user perspective, I understand your nuisance. However, type-checking all of a Svelte component's code is not as trivial as it is with React components or with simple TypeScript files. We consciously decided to not type-check from v4 onwards. Previously, we indeed did some sort of type-checking but it was not only very slow but also had a lot of false-positive errors. The current type-checking workflow would be type-checking in the IDE while developing via svelte-vscode and in a CI environment via svelte-check.

However, there have been a significative amount of folks asking why we don't throw an error if a type error is found. Following the rest of the optional features of this library, maybe we can use the svelte-language-server via a typeCheck: boolean or even the noEmitOnError: boolean prop. The LSP would be an optional/peer dependency, of course.

I'll reopen this as this could be easily done, but I should note that, performance-wise, this is not an optimal solution, as we'd be going over a component's code twice: svelte-preprocess while transpiling and svelte-check while type-checking.

@kaisermann kaisermann reopened this Jul 25, 2020
@kaisermann kaisermann added enhancement New feature or request question Further information is requested labels Jul 25, 2020
@kaisermann kaisermann changed the title Respecting noEmitOnError compiler option Feature: type-checking Jul 25, 2020
@sharifzadesina
Copy link
Author

sharifzadesina commented Jul 26, 2020

@kaisermann bro I understand what you're saying, but while all standard TS tools do it, and they don't have any problem, why we shouldn't do it? some people use Notepad for developing, why we have to force them to use VSCode? or to run a CLI command each time? (I know they can use some kind of hook, but it is still a dirty solution).
and regardless of that, when someone wants type checking and secure code, he can forget about compilation speed.
Writing TypeScript without type checking is like writing Java code without type checking, imagine what it would be like.

@dummdidumm
Copy link
Member

Another approach would be to just add a watch-mode to svelte-check, which would be run in parallel to the normal dev compilation. Wouldn't this suffice? sveltejs/language-tools#287

@dummdidumm
Copy link
Member

dummdidumm commented Jul 26, 2020

If we do this inside svelte-preprocess, how would we make sure that

  1. Diagnostics are run on all files, not just the ones which were changed? -> can be done inside language server ("trigger diagnostics for all known files")
  2. Diagnostics are only invoked once? We need to prevent to trigger diagnostics for all files each time a single file is preprocessed. Debounce? But how would we stop the emit then?

@dummdidumm
Copy link
Member

dummdidumm commented Jul 30, 2020

The more I think about it the more I feel like svelte-preprocess is not the right place to throw type check errors.

  • svelte-check now supports watch mode
  • This article shows how to integrate svelte-check with rollup. Maybe that's something to add to the official rollup-plugin-svelte / svelte-loader (off by default)?

@sharifzadesina
Copy link
Author

sharifzadesina commented Aug 7, 2020

@dummdidumm Hi, I just checked Nuxt.js (It is for Vue.js)
It also checks types and reports them to the user when you run the app.
We should do the same. or maybe just run svelte-check in watch mode alongside the svelte app.

@kaisermann kaisermann removed the enhancement New feature or request label Aug 18, 2020
@brunnerh
Copy link
Member

TypeScript is all about finding errors at build time so i expect a TypeScript preprocessor to also throw these errors at build time.

@dummdidumm
Copy link
Member

dummdidumm commented Aug 24, 2020

A preprocessor only gets the content of the script tag, which is only part of a svelte file. This will produce false errors. Think of this situation:

<script lang="ts">
    let a: number = 1;
</script>
{a}

svelte-preprocess only gets let a: number = 1; and the diagnostics would output that a is never used, which is wrong.

So there simply is no good way to make this part of the preprocessors.

To get TS errors at build time, just use svelte-check and run it as a pre-build-step.

@brunnerh
Copy link
Member

brunnerh commented Aug 24, 2020

Maybe this could be integrated as a kind of post-processing step then? Right now this breaks expectations and complicates things. I already have a webpack build on watch (actually 2, server & client) and ideally that would just show me the errors without the need to run another watcher in parallel.

@dummdidumm
Copy link
Member

I agree that there are ways to make this easier / feel more built-in, but svelte-preprocess is the wrong place for this. A better place might be svelte-loader / rollup-plugin-svelte.

@kaisermann
Copy link
Member

I'm on @dummdidumm's side here. Will close this in favor of issues in svelte-loader and rollup-plugin-svelte.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants