-
-
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
perf(esbuild): make tsconfck non-blocking #12548
Conversation
Run & review this pull request in StackBlitz Codeflow. |
initTSConfck(server.config).finally(() => { | ||
// server may not be available if vite config is updated at the same time | ||
if (server) { | ||
// force full reload | ||
server.ws.send({ | ||
type: 'full-reload', | ||
path: '*', | ||
}) | ||
} | ||
}) | ||
initTSConfck(server.config.root, true) | ||
|
||
// server may not be available if vite config is updated at the same time | ||
if (server) { | ||
// force full reload | ||
server.ws.send({ | ||
type: 'full-reload', | ||
path: '*', | ||
}) | ||
} | ||
} |
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.
Since tsconfck is non-blocking now, we can reload the page right away, those that rely on tsconfig will wait for the promise before continuing.
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
let tsconfckRoot: string | undefined | ||
let tsconfckParseOptions: TSConfckParseOptions | Promise<TSConfckParseOptions> = | ||
{ resolveWithEmptyIfConfigNotFound: true } |
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.
Shouldn't we have these cache as WeakMap<ResolvedConfig,...>
? It would be good to make it safe to create two esbuild plugins programmatically.
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.
transformWithEsbuild
mess it all up 😄 It doesn't have a reference to ResolvedConfig
, and we can't get the "first value" of the weak map because the API doesn't support that.
I refactored this part a few times and decided to keep most of it as is at the end. Maybe we can hae transformWithEsbuild
accept a ResolvedConfig
too in the future.
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.
This will help for example when the user has --open
because the server can kick in faster and there is time until the browser opens to finish processing.
Let's merge it as this PR is also better at handling the caching, even when there is a single global cache for root
.
What isn't clear to me is how the transformWithEsbuild
exported function from Vite works. As it depends on whether the esbuild plugin is being created or not. I think a possible option is to pass a new optional param to transformWithEsbuild
that is the tsconfckParseOptions or better a callback to pass a loadTsconfigJsonForFile
for file function. Then in the esbuild plugins we can implement a proper cache against ResolvedConfig, and external users can also do the same if the have a ResolvedConfig or implement their own caching. I think right now it is broken.
Description
ref #12519
Move
tsconfck
out ofconfigResolved
to not block server startup.From local testing, the time it took to init tsconfck went from 10ms to 26ms with this change, but I suspect it's because more things are running at the same time when tsconfck scans for
tsconfig.json
s now.The server startup time stays the same locally too, so I'm not sure how beneficial this PR is. Maybe it helps for projects with huge directories / slower machines?
Additional context
Interested to hear thoughts on this. @stafyniaksacha maybe you can help test this?
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).