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

fix(create-vite): simplify TSConfig setup #17732

Closed
wants to merge 5 commits into from
Closed

Conversation

ArnaudBarre
Copy link
Member

I think that #15913 was not the good solution for this setup:

  • It forces the creation of DTS build artefact, which was added for performance optimization when projects depend on others or are split in multiple parts, but this not the case here (and create issue like New tsconfig of create-vite gives type errors if .d.ts files can not be generated #17638)
  • It breaks people use to run a simple tsc or tsc -w to check the main project
  • Typechecking the config can be done simply by having "build": "tsc && tsc -p tsconfig.node.json && vite build"

First commit is the revert, the second one suggest a new setup on the React project, I will do the changes on toher later if we go in this direction.

The config change does multiple thing:

  • Avoid the usage of references and composite that make things complex for starter project IMO
  • Remove resolveJsonModule which default to true in bundler more
  • Make the node config on par with the src config:
    • Linting with noUnusedLocals and noUnusedParameters
    • Enable bundler more flags: allowImportingTsExtensions, isolatedModules, noEmit
  • For the node config, update target/lib to follow TS recommendation for node 18

Copy link

stackblitz bot commented Jul 21, 2024

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

@ArnaudBarre ArnaudBarre changed the title fix(create-vite): Simplify TSConfig setup fix(create-vite): simplify TSConfig setup Jul 21, 2024
"skipLibCheck": true,

/* Bundler mode */
"moduleResolution": "bundler",
Copy link

@armano2 armano2 Jul 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are missing "moduleDetection": "force", this is important for any user that decides to use eslint within template, as it forces all files to be recognized as module's,

eg, vue files in setup mode will not be treated as such without this.

ref #17468

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I've only updated back the React one

"build": "tsc -b && vite build",
"build": "tsc && tsc -p tsconfig.node.json && vite build",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we needed references so that the IDE picks up the right config when typechecking the Vite config? Perhaps we need both this script change and the references to properly support the usecase, and should be good enough?

@dominikg
Copy link
Contributor

do we really want to have a file called tsconfig.node.json that includes just one file vite.config.ts ? This feels like a misnomer as for vite.config.ts "node" isn't really relevant or descriptive.

+1 to removing -b from the tsc call.

Still on the fence if we should even typecheck vite config as part of the build script. It is a bit opinionated, and if the main purpose is to ensure vite receives a valid config, maybe it shouldn't just be added to the barebones templates but to vite core itself (throw human readable error)

@ArnaudBarre
Copy link
Member Author

ArnaudBarre commented Jul 23, 2024

Oh yeah ok so references are needed for IDE. I think the features is great but it's a bit sad they merge this is requirement for dts and a new --build flag 😫 I'll update the PR for that, which will be mostly a revert

I don't think we should run TS as part of vite core configuration validation, it's too slow (500ms the react template config on M1)

@ArnaudBarre
Copy link
Member Author

Updated, the current state of the PR is similar to before #15913 but with few improvements that should fix the initial needs of this PR:

  • -b in the build script now uses caching and check both (debatable for @dominikg)
  • configure tsbuildinfo output so -b is faster and doesn't add unwanted to level file
  • the node config is stricter and in line with node18 (even if @types/node are not installed)

@dominikg
Copy link
Contributor

i don't understand why you need incremental, references should work without it.

@ArnaudBarre
Copy link
Member Author

it's not required, but it enable caching when running tsc -b

@dominikg
Copy link
Contributor

this should be a users descision to enable then. If you leave it out you can also leave out the extra config for cache location?

@bluwy
Copy link
Member

bluwy commented Jul 23, 2024

Do we need -b? It has caused confusion to people in the past: #17585. I don't completely understand #17638, but would using tsBuildInfo bring back its issue?

@ArnaudBarre
Copy link
Member Author

@bluwy Here -b is not required, it's just a way when you have reference to do sc && tsc -p tsconfig.node.json in a faster way. the good part about the current setup is that people use to just run tsc or tsc --noEmit will get back the previous behaviour because the main tsConfig is not just references

What caused #17638 is the fact that the "app" config was a composite tsConfig, which forces emitDeclaration which caused the issue. By putting the "app" config into the main tsconfig, we avoid that

@dominikg Yes this can be enabled by the user, but I don't think it hurts to be there by default IMO. It's clearly marked as a performance optimization and can be easily removed by the user.

@dominikg
Copy link
Contributor

dominikg commented Jul 25, 2024

I believe we should not put optional performance optimizations in here unless we want to change the semantics of the projects generated from create-vite away from minimal starters to something more opinionated.
For now, create-vite provides the bare essentials to get it running, everything else is up to the user.

More opinionated setups are available via delegated framework clis, eg create-solid, create-svelte, nuxi etc. Maybe there is one we can add to react? (vike, rakkas?)

@ArnaudBarre
Copy link
Member Author

ArnaudBarre commented Jul 27, 2024

For info we may need to add back at least the tsbuildInfoFile option in TS 5.6: https://devblogs.microsoft.com/typescript/announcing-typescript-5-6-beta/#tsbuildinfo-is-always-written

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

Successfully merging this pull request may close these issues.

4 participants