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

feat(create-vite): use "solution" tsconfig so that vite.config.ts is type checked #15913

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

philbates35
Copy link
Contributor

@philbates35 philbates35 commented Feb 14, 2024

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 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

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:

"check": "svelte-check --tsconfig ./tsconfig.json"

I checked the docs of the svelte-check project but I couldn't see anyway to pass options through to tsc, 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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Copy link

stackblitz bot commented Feb 14, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@philbates35 philbates35 force-pushed the tsc-build branch 2 times, most recently from ec14e59 to 2be3660 Compare February 23, 2024 19:42
philbates35 added a commit to philbates35/laravel-starter that referenced this pull request Feb 24, 2024
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
philbates35 added a commit to philbates35/laravel-starter that referenced this pull request Feb 24, 2024
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
philbates35 added a commit to philbates35/laravel-starter that referenced this pull request Feb 24, 2024
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
philbates35 added a commit to philbates35/laravel-starter that referenced this pull request Feb 24, 2024
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
philbates35 added a commit to philbates35/laravel-starter that referenced this pull request Feb 24, 2024
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
@philbates35 philbates35 changed the title feat(create-vite): pass -b to tsc so that vite.config.ts is checked feat(create-vite): use "solution" tsconfig so that vite.config.ts is type checked Feb 26, 2024
@sapphi-red sapphi-red added p2-nice-to-have Not breaking anything but nice to have (priority) feat: create-vite create-vite package p2-to-be-discussed Enhancement under consideration (priority) and removed p2-nice-to-have Not breaking anything but nice to have (priority) labels Feb 28, 2024
@sapphi-red
Copy link
Member

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
@patak-dev
Copy link
Member

Let's wait for a tsconfck update before merging this one.

@philbates35
Copy link
Contributor Author

@patak-dev sounds good - just out of interest, which update are we waiting for and what's the reason?

@patak-dev
Copy link
Member

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.

@dominikg
Copy link
Contributor

dominikg commented Apr 3, 2024

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

@dominikg
Copy link
Contributor

dominikg commented Apr 3, 2024

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?

@sapphi-red
Copy link
Member

why is this turning on composite

It is to run type check on vite.config.ts too. Another way to do this is to call tsc && tsc -p tsconfig.node.json instead of tsc -b.

storing a file in node_modules/.tmp

AFAIK when using project references, TypeScript outputs tsbuildinfo files and defaults to the same directory with tsconfig.json. There's no way to disable the output.

Is there a way to make this work that does not require additional files?

We can merge tsconfig.app.json and tsconfig.node.json, but that allows node-related types to be used in source files and browser-related types to be used in config files (e.g. process.env in src/index.ts, document.querySelector in vite.config.ts).

@dummdidumm
Copy link
Contributor

dummdidumm commented Apr 3, 2024

@dominikg chaining looks fine, assuming the vite config does not import any Svelte code. It's fine because the tsconfigs have separate includes, so svelte-check doesn't check the vite config, which is what the tsc command then does.

@dominikg
Copy link
Contributor

dominikg commented Apr 3, 2024

AFAIK when using project references, TypeScript outputs tsbuildinfo files and defaults to the same directory with tsconfig.json. There's no way to disable the output.

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.

@dominikg
Copy link
Contributor

dominikg commented Apr 3, 2024

to expand on the separate command part, build may not be the right command to typecheck the vite config. The vite configs included in create-vite don't have errors, if and how these are checked as part of build should be decided by the user.

@dominikg
Copy link
Contributor

dominikg commented Apr 3, 2024

enabling composite/incremental can also slow down CI build times and it's not clear if the node_modules directory always exists as a sibling of the tsconfig file. I think this needs to be actively done by the user instead of being default in create-vite.

@sapphi-red
Copy link
Member

you can use references without turning on composite (composite makes incremental true by default https://www.typescriptlang.org/tsconfig#incremental , which generates these files)

enabling composite/incremental can also slow down CI build times and it's not clear if the node_modules directory always exists as a sibling of the tsconfig file. I think this needs to be actively done by the user instead of being default in create-vite.

I didn't know about that. Then, I think we should remove composite and tsBuildInfoFile if we'd go this way.

calling it tsconfig.node.json is also slightly controversial for users that use other environments such as deno or bun.

tsconfig.node.json was there for more than two years. So I want to split that discussion in a different PR.


Maybe removing tsconfig.node.json and adding vite.config.ts to include of tsconfig.json is better? 🤔 It doesn't have much benefit of having it in the current state (#11396 (comment)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: create-vite create-vite package p2-to-be-discussed Enhancement under consideration (priority)
Projects
Status: Discussing
Development

Successfully merging this pull request may close these issues.

tsconfig.node.json should not be included in tsconfig.json references
5 participants