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

perf(esbuild): make tsconfck non-blocking #12548

Merged
merged 2 commits into from Mar 23, 2023
Merged

perf(esbuild): make tsconfck non-blocking #12548

merged 2 commits into from Mar 23, 2023

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Mar 23, 2023

Description

ref #12519

Move tsconfck out of configResolved 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.jsons 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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bluwy bluwy added the performance Performance related enhancement label Mar 23, 2023
@stackblitz
Copy link

stackblitz bot commented Mar 23, 2023

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

Comment on lines -496 to 528
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: '*',
})
}
}
Copy link
Member Author

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.

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Mar 23, 2023

📝 Ran ecosystem CI: Open

suite result
astro ❌ failure
histoire ❌ failure
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ✅ success
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success
windicss ✅ success

Comment on lines +436 to +438
let tsconfckRoot: string | undefined
let tsconfckParseOptions: TSConfckParseOptions | Promise<TSConfckParseOptions> =
{ resolveWithEmptyIfConfigNotFound: true }
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@patak-dev patak-dev left a 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.

@patak-dev patak-dev merged commit e5cdff7 into main Mar 23, 2023
12 checks passed
@patak-dev patak-dev deleted the tsconfck-no-block branch March 23, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants