-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
feat(create-vite): use "solution" tsconfig so that vite.config.ts is type checked #15913
Conversation
Run & review this pull request in StackBlitz Codeflow. |
75a6af5
to
ed63247
Compare
ec14e59
to
2be3660
Compare
The change in 2aa6462 is resulting in a vite.config.js and vite.config.d.ts being generated which we don't want. The work around, to be able to have type checking of both src files and vite.config.ts during build is to create a dedicated tsconfig.app.json and tsconfig.node.json. See: * vitejs/vite#15913 * https://github.com/vuejs/create-vue/blob/f75fd9813a624b8e44b012608335721901aba00b/template/tsconfig/base/tsconfig.json
The change in 2aa6462 is resulting in a vite.config.js and vite.config.d.ts being generated which we don't want. The work around, to be able to have type checking of both src files and vite.config.ts during build is to create a dedicated tsconfig.app.json and tsconfig.node.json. See: * vitejs/vite#15913 * https://github.com/vuejs/create-vue/blob/f75fd9813a624b8e44b012608335721901aba00b/template/tsconfig/base/tsconfig.json
2be3660
to
27988cb
Compare
The change in 2aa6462 is resulting in a vite.config.js and vite.config.d.ts being generated which we don't want. The work around, to be able to have type checking of both src files and vite.config.ts during build is to create a dedicated tsconfig.app.json and tsconfig.node.json. See: * vitejs/vite#15913 * https://github.com/vuejs/create-vue/blob/f75fd9813a624b8e44b012608335721901aba00b/template/tsconfig/base/tsconfig.json
The change in 2aa6462 is resulting in a vite.config.js and vite.config.d.ts being generated which we don't want. The work-around. To fix this, lets do the "solution" tsconfig approach as used by the create-vue project. This results in things like the following, all of which aren't enforced currently: * tsc allows "process.cwd()" in vite.config.ts and sum.test.ts but not in any src files such as app.ts * tsc allows document.createElement() in src files such as app.ts and in sum.test.ts (because jsdom exists there) but not in vite.config.ts which is just pure node environment. * Can't import test files into src files * Allows using new features such as "".replaceAll() in vite.config.ts which should be allowed because we're using node 20. See: * vitejs/vite#15913 * https://github.com/vuejs/create-vue/blob/12bf2889b9ca981bcfed894a7c24fa9db5e7bad5/template/tsconfig/base/tsconfig.app.json * https://github.com/vuejs/create-vue/blob/12bf2889b9ca981bcfed894a7c24fa9db5e7bad5/template/tsconfig/base/tsconfig.node.json * https://github.com/vuejs/create-vue/blob/12bf2889b9ca981bcfed894a7c24fa9db5e7bad5/template/tsconfig/vitest/tsconfig.vitest.json
The change in 2aa6462 is resulting in a vite.config.js and vite.config.d.ts being generated which we don't want. To fix this, lets do the "solution" tsconfig approach as used by the create-vue project. This results in things like the following, all of which aren't enforced currently: * tsc allows "process.cwd()" in vite.config.ts and sum.test.ts but not in any src files such as app.ts * tsc allows document.createElement() in src files such as app.ts and in sum.test.ts (because jsdom exists there) but not in vite.config.ts which is just pure node environment. * Can't import test files into src files * Allows using new features such as "".replaceAll() in vite.config.ts which should be allowed because we're using node 20. See: * vitejs/vite#15913 (comment) * https://github.com/vuejs/create-vue/blob/12bf2889b9ca981bcfed894a7c24fa9db5e7bad5/template/tsconfig/base/tsconfig.app.json * https://github.com/vuejs/create-vue/blob/12bf2889b9ca981bcfed894a7c24fa9db5e7bad5/template/tsconfig/base/tsconfig.node.json * https://github.com/vuejs/create-vue/blob/12bf2889b9ca981bcfed894a7c24fa9db5e7bad5/template/tsconfig/vitest/tsconfig.vitest.json
27988cb
to
7576086
Compare
One thing we might need to consider is that this might make more users to encounter #15870. |
Currently when running "pnpm run build" on a fresh Vite project using any of these templates, tsc doesn't fail when there are type errors in vite.config.ts, such as something obvious like: const foo: string = 123; Passing -b makes it run for all project references, meaning that vite.config.ts starts to be type checked too during build. In order to not generate a vite.config.js and vite.config.d.ts we need to make the main tsconfig.json have nothing but references to tsconfig.node.json and a new tsconfig.app.json, as is done in the create-vue project, see: https://github.com/vuejs/create-vue/blob/f75fd9813a624b8e44b012608335721901aba00b/template/tsconfig/base/tsconfig.json
7576086
to
07358e4
Compare
Let's wait for a tsconfck update before merging this one. |
@patak-dev sounds good - just out of interest, which update are we waiting for and what's the reason? |
I think we can now move forward with this one. @dominikg I'll let it open a few more days in case you'd like to check it out too. |
why is this turning on composite and storing a file in node_modules/.tmp ? This feels opinionated cc @dummdidumm regarding svelte-check, iirc it does check all files, not just .svelte. It should not be needed to chain tsc after it in svelte templates |
in general I'd like to challenge the use of solution style tsconfig in create-vite. My understanding is that these are minimalist setups. Is there a way to make this work that does not require additional files? |
It is to run type check on
AFAIK when using project references, TypeScript outputs tsbuildinfo files and defaults to the same directory with
We can merge |
@dominikg chaining looks fine, assuming the vite config does not import any Svelte code. It's fine because the tsconfigs have separate includes, so |
you can use references without turning on composite (composite makes incremental true by default https://www.typescriptlang.org/tsconfig#incremental , which generates these files) calling it tsconfig.node.json is also slightly controversial for users that use other environments such as deno or bun. If you have to add a second config file, i'd use a more neutral name and not use composite. Maybe not even use references but instead add a separate script that just typechecks vite.config.ts with a tsconfig.viteconfig.json. |
to expand on the separate command part, |
enabling composite/incremental can also slow down CI build times and it's not clear if the |
I didn't know about that. Then, I think we should remove
tsconfig.node.json was there for more than two years. So I want to split that discussion in a different PR. Maybe removing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's give this a try. I'm also concern that this makes a simple Vite project complicated with having 3 tsconfigs ootb, but we can gauge the perception of this later, and if many others feel the same way, we can simplify as a single tsconfig and document how to typecheck the vite config separately.
IMO, making things complex. this shouldn't be included in a basic template. |
Thought it worth mentioning that this does seem to cause errors out-of-the-box in every JSX lib on StackBlitz -- it can't seem to utilize the I imagine this is something StackBlitz has to figure out rather than an issue here, but with StackBlitz being a recommended option for reproductions... something worth considering for the moment, anyways. |
This also gives issues in Storybook, as In storybook, In a fresh storybook vite react project, users now get the following TS error:
|
There is a fix for StackBlitz that should be deployed soon, so that won't be an issue for people using this scheme. But we are getting other issues as the one that @kasperpeulen has brought up, so I think we should review this change. Would you open an issue so we can better track this @kasperpeulen? |
@patak-dev here is the issue: |
…g.ts is type checked (#15913)"
Type checking is currently not evaluated for referenced files. In order to make it evaluate, we have to use `--build`. However, this cannot be used with `--noEmit` and so will emit compiled files unless we use the solution tsconfig pattern. See - microsoft/TypeScript#53979 - vitejs/vite#15913 See also - https://www.typescriptlang.org/docs/handbook/project-references.html#overall-structure - vitejs/vite#17774
Type checking is currently not evaluated for referenced files. In order to make it evaluate, we have to use `--build`. However, this cannot be used with `--noEmit` and so will emit compiled files unless we use the solution tsconfig pattern. See - microsoft/TypeScript#53979 - vitejs/vite#15913 See also - https://www.typescriptlang.org/docs/handbook/project-references.html#overall-structure - vitejs/vite#17774
Description
Currently when running
pnpm run build
on a fresh Vite project using any of these templates,tsc
doesn't fail when there are type errors invite.config.ts
, such as something obvious like:Passing
-b
makes it run for all project references, meaning thatvite.config.ts
starts to be type checked too during build.In order to not generate a
vite.config.js
andvite.config.d.ts
we need to make the maintsconfig.json
have nothing but references totsconfig.node.json
and a newtsconfig.app.json
, as is done in the create-vue project, see:https://github.com/vuejs/create-vue/blob/f75fd9813a624b8e44b012608335721901aba00b/template/tsconfig/base/tsconfig.json
Note: I am unfamiliar with Svelte which is the only template that I wasn't sure how to apply this to, its script looks like this:
I checked the docs of the
svelte-check
project but I couldn't see anyway to pass options through totsc
, if you know of a way to do that then let me know and I'll update the PR.close #12590
close #11396
What is the purpose of this pull request?