refactor: update to tsconfck3 with lazy cache #14234
Conversation
|
@sheremet-va would be great if you could test this with your windows setup that was slow on findAll. @fi3ework / @sapphi-red what's the best way to benchmark the code in this PR (comparison between current main and this, startup and hmr in a project with many ts files) |
|
|
|
It may be a good idea to do the following cases:
Triggered the benchmark CI: https://github.com/vitejs/vite-benchmark/actions/runs/6032434823 |
|
Hmmm, the benchmark CI is failing for some reason. Maybe the recent breaking changes broke it |
|
that might be a legit fail. for some reason the tsconfck should still return an empty object in that case and vite then just uses defaults, but it seems something is different in v3, even though the testcase in tsconfck passes for this. gotta investigate more. cc @fi3ework for the benchmark thing. not sure if the missing tsconfig is on purpose or an accident |
|
benchmark call should work again now, here are the results of a local run Benchmark for pull request #14234 Meta Info
Benchmark ResultCase 1: vite 2.7 slowDetails
Case 2: 1000 React componentsDetails
note that the results fluctuate a bit, so it's not clear that the new tsconfck is faster in this benchmark, it isn't slower though and does less work up front in huge projects so i'll take it. |
|
The removal of the "root" option means it'll also continue to search for tsconfig files outside of workspace root, which is more in line with typescripts own behavior at the expense of added risk that files outside the workspace root can change the outcome of a build - i would not recommend such a setup and maybe it warrants a warning log. Another thing to consider is that findAll ignored node_modules, with that gone, find will now use the closest tsconfig for files inside node_modules too, so with the files below # some lib containing ts files in src and a tsconfig.json
node_modules/some-lib/tsconfig.json
node_modules/some-lib/src/index.ts
# vite root contains tsconfig.json and src
src/something.ts
tsconfig.json the old implementation with tsconfck v2 and findAll would use root tsconfig.json for Now there shouldn't be published libraries that come with ts instead of js files, but if you had a locally linked package inside node_modules, it could happen 🤔 |
Ah, I see. The reason it was missing tsconfig.json was because the original repo was missing that and I didn't notice that when forking from it.
I think we can have a warning for this.
I think we can skip the tsconfigs in node_modules. esbuild does this as well (there were some discussion about this at evanw/esbuild#3019). For For locally linked packages, Vite will pass the real path to tsconfck (if |
|
It would be fairly easy to add a "jump out of node_modules" here https://github.com/dominikg/tsconfck/blob/version-3/packages/tsconfck/src/find.js#L42 like const i = dir.indexOf('/node_modules/');
if(i>-1) {
dir = dir.slice(0,i);
}however does esbuild pick up tsconfig outside node_modules or outright ignore all tsconfig files and go with defaults? I really hope they do it consistently to avoid having another option 🙈 |
|
of course typescripts own find config does not skip over node_modules. dominikg/tsconfck#123 |
…h lazy cache create and use direct cache access to avoid one promise wrap/unwrap
|
latest tsconfck 3 beta ignores files in node_modules by default, bumped this PR to it got rid of init altogether in favor of lazy cache create. local bench with it: Benchmark for pull request #14234Meta Info
Benchmark ResultCase 1: vite 2.7 slowDetails
Case 2: 1000 React componentsDetails
|
|
the test fail seems to happen due to insufficient error handling inside tsconfck. cache evicts entries that error automatically but somehow it seems to get stuck on a promise that never settles. |
|
this PR in tsconfck dominikg/tsconfck#125 caches errors instead of eviction. After a parse error, a file has to be edited to correct that error and vite clears the tsconfck cache on changes, so it would get parsed once again. Speaking of reparsing, in some situations we might not have to throw away the whole cache or invalidate the complete module graph but just reparse that tsconfig and invalidate files matching the old (and new) config. But determining this safely and then finding out if invalidating some files is faster than reloading everything is hard. |
Ah, it seems esbuild simply ignores and go with defaults. |
|
Can we expose the cache on |
we would have to expose the complete tsconfck options or even better the function that returns the config to prevent differences |
I think having a method on a server to get tsconfig for a specific file might be a good idea for performance reasons. Having the same for package.json would also be phenomenal, but it’s out of the scope of this issue of course. Exposing cache itself seems risky and limits design space I think (need to release a breaking change if interface changes or something) |
It should be available in both dev and build mode |
PR to change tsconfck to this behavior: dominikg/tsconfck#128 Would it be better if this was done inside vite though? This is more specific to esbuild behavior than typescript itself. |
|
went ahead and changed it to ignoreNodeModules and have vite opt in to that behavior. |
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
|
Merging this. We can bump to a stable version of |
Description
tsconfck 3 lazily caches found and parsed tsconfig files to ensure minimal use of fs without having to use the
rootorfindAll + tsconfigPathsoptions.This PR updates to a beta release to gather feedback. vite beta bundling a tsconfck beta should be fine, before releasing vite5.0 tsconfck 3.0 is going to be released and added.
Additional context
The removal of async init and root options means we no longer have to do the song and dance to initialize it, however the way it was tied to root/workspace root could mean it wasn't meant to be shared before. It should be safe now but extra care is needed to ensure we don't break stuff.
It should also be checked how perf is affected. cold start/ready should become slightly faster while page ready could become a little slower. hmr should be unaffected.
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).