Skip to content

perf(vite): narrow test for changed files in esbuild plugin#22327

Closed
danielroe wants to merge 2 commits into
vitejs:mainfrom
danielroe:fix/esbuild-tsconfig
Closed

perf(vite): narrow test for changed files in esbuild plugin#22327
danielroe wants to merge 2 commits into
vitejs:mainfrom
danielroe:fix/esbuild-tsconfig

Conversation

@danielroe
Copy link
Copy Markdown
Contributor

was browsing the codebase and noticed this pattern:

  if (changedFile.endsWith('.json')) {
    const cache = getTSConfigResolutionCache(server.config)
    if (changedFile.endsWith('/tsconfig.json')) {
      // ...
    }
  }

as getTSConfigResolutionCache doesn't seem to have side effects, and since #21622 the cache is only used in the second if block, I think we can slightly refactor. I've made an early return but could leave the if and simply reduce if checks by one.

it's a tiny, tiny improvement but thought it was worth it 🤷

@danielroe
Copy link
Copy Markdown
Contributor Author

danielroe commented Apr 25, 2026

I don't think the failing test is related.

// any json file in the tsconfig cache could have been used to compile ts
if (changedFile.endsWith('.json')) {
const cache = getTSConfigResolutionCache(server.config)
if (changedFile.endsWith('/tsconfig.json')) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line actually should have || cache.hasParseResult(changedFile) like the condition in v7:

if (
changedFile.endsWith('/tsconfig.json') ||
cache.hasParseResult(changedFile)
) {

The fix is blocked by oxc-project/oxc-resolver#1011, but after we have that, the condition should be added.

So if the performance difference isn't significant, I'll prefer to keep the code as-is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ahh, that makes sense!

@danielroe danielroe closed this Apr 27, 2026
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.

2 participants