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-kit build swallows typescript errors #1536

Closed
thislooksfun opened this issue May 24, 2021 · 11 comments · Fixed by #1556
Closed

svelte-kit build swallows typescript errors #1536

thislooksfun opened this issue May 24, 2021 · 11 comments · Fixed by #1556

Comments

@thislooksfun
Copy link
Contributor

Describe the bug
Errors in .ts files are silently ignored when running svelte-kit build.

Logs
N/A

To Reproduce

  1. Enable some feature in tsconfig.json, such as strictNullChecks or noUnusedLocals, then write some code that breaks that rule. For example, setting noUnusedLocals: true then writing let unusedVar = 42;.
  2. Run svelte-kit build

Expected behavior
I expect that svelte-kit build would fail (exit != 0) and give a helpful error message. Instead it just silently succeeds, even though compiling the exact same code with tsc correctly errors.

Stacktraces
N/A

Information about your SvelteKit Installation:

N/A

Severity
Very annoying, means that I can't rely on the build process to catch errors so I have to spend extra time checking for them manually and also worry about an avoidable bug slipping through the cracks.

Additional context
N/A

@ignatiusmb
Copy link
Member

This should probably be done in Vite's side, I remember Sapper can catch TS errors because it uses Rollup plugins. In the meantime, you can add a pre-build step to check for errors first

"scripts": {
    ...
    "build": "tsc --noEmit && svelte-kit build",
    ...
},

@dummdidumm
Copy link
Member

dummdidumm commented May 24, 2021

The build process itself will not run any type checking. You need to use svelte-check for that. Running tsc --noEmit will not be enough because that one doesn't know how to deal with Svelte files / doesn't check them. We probably should add that to the configuration when the user selects TypeScript from in the setup commands.

@ignatiusmb
Copy link
Member

As @dummdidumm says, but if you only want to catch errors in .ts files like you said in the description, then tsc should be enough.

@thislooksfun
Copy link
Contributor Author

Right, forgot that you need a separate step to do the main thing that TypeScript was made for. That still really feels like an anti-pattern to me.

@thislooksfun
Copy link
Contributor Author

I went back and re-read the reasoning behind not having preprocess do the type checking (sveltejs/svelte-preprocess#205), which is an unfortunate but ultimately sensible approach since the preprocessors work on isolated chunks so checks like unused variables wouldn't work. However, at the end of that issue it's mentioned that while the preprocessor is the wrong place for this type of check putting it in the loader/rollup plugin would make sense. I think that svelte-kit's build command (and also the dev command, hopefully) is similar enough to warrant also running svelte-check automatically.

The job of svelte-preprocess is to take snippets of typescript code (be that an entire .ts file or just one line from a lang="ts" script) and convert them into standard JS. By the very nature of it being snippets it therefore can't check for full type correctness within .svelte files. This is understandable, but svelte-kit build (and svelte-kit dev) compile the entire project and it would make sense for it to also therefore be responsible for checking for logical errors before trying to compile the project.

Besides, think how amazingly useful it would be for us TS devs if having a type error (or unused var, or any of the other errors tsc can check for) would pop up a Vite panel on the live site, like it does if you have an html error. That level of integration of tooling would be absolutely invaluable for rapid development, regardless of your choice of editor, and it comes with peace-of-mind as you'd no longer have to ask "did I remember to run svelte-check before pushing?"

@thislooksfun
Copy link
Contributor Author

Additionally I now realize that svelte-check only checks .svelte files, which means that in order to fully type-check the default generated SvelteKit project I have to use both svelte-check and tsc --noEmit.

@dummdidumm
Copy link
Member

svelte-check will also check TS/JS files, soon. Running it as part of the build step inside kit itself feels wrong, but I think that the starter template should invoke them in order to get the same result.

@thislooksfun
Copy link
Contributor Author

@dummdidumm Could you elaborate on why it would be wrong to do type checking in the build process? I (and many other people) feel it's wrong to not do it in the build step. I tolerated it when using svelte-preprocess via webpack/rollup due to the aforementioned technical limitations, but now that you have your own start-to-finish build command I see no reason to not have type checking built into the build step like every single other compiler does.

@dummdidumm
Copy link
Member

Because doing "build": "svelte-check && svelte-kit build" is more explicit and more flexible to me (and if it was built in, this would be what would happen under the hood anyway).

@thislooksfun
Copy link
Contributor Author

Sure it's more explicit, but there's still a reason why tooling like tsc has it built in. Besides, that still doesn't address having type checking as part of svelte-kit dev which would be insanely useful for detecting errors as we go, rather than having to manually check afterwards.

@dummdidumm
Copy link
Member

dummdidumm commented May 25, 2021

svelte-kit dev uses Vite under the hood, which does not do any type checking. So if it was "built-in", it would again mean that svelte-check --watch is called, which I'd rather have explicit in my package.json. My development workflow for example relies on IDE checks during dev and then svelte-check prior to the build.
It works for other CLIs like Angular or Vue because they use webpack, where they can integrate the typechecking into the process.

dummdidumm added a commit that referenced this issue May 28, 2021
This add svelte-check along with two validate scripts to the package.json if the user selects TS. This uncovered missing type definitions for the cookie package in the default template, which are now added as well if the user selects TS.
Closes #1536
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants