feat: improved cold start using deps scanner

[patak-dev]

Fix #8867

Description

See comment #8869 (comment)

Initial exploration of diff strategies

Click to read the original intent of the PR

Adds a new `optimizeDeps.devStrategy` option to allow users to define the cold start strategy.

• 'pre-scan': pre-scan user code with esbuild to find the first batch of dependencies to optimize. This is the v2.9 scheme (this option replaces legacy.devDepsScanner, removed in this PR)
• 'lazy': only static imports are crawled, leading to the fastest cold start experience with the tradeoff of possible full page reload when navigating to dynamic routes. This is the current default.
• 'eager': both static and dynamic imports are processed on cold start completely removing the need for full page reloads at the expense of a slower cold start.

And there is a new strategy that I think should be the default for non MPAs:

• 'dynamic-scan': delay optimization until static imports are crawled, then scan with esbuild dynamic import entries found in the source code.

Edit: after some discussions, I think if we have to use the scanner then the best would be a new mode (to be implemented)

• 'scan': perform scanning in the background, then once the server is idle, use the scanned deps list to complete what was found normally. It would be like 'lazy' + completed with the scanned deps

Data point for a SPA (57 dynamic routes)

- v2.9:            20s
- v3.0: 'lazy':    11s  (adding one dep to `optimizeDeps.include`, if not there is a reload)
- v3.0: 'eager':   27s

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

[netlify]

✅ Deploy Preview for vite-docs-main canceled.

Name	Link
🔨 Latest commit	8861550
🔍 Latest deploy log	https://app.netlify.com/sites/vite-docs-main/deploys/62c28c9c35d7bd0008ef6e94

[patak-dev]

Our conclusions from the last team meeting:

• We should keep the scanner for v3, as it is currently a good way to ensure DX on cold start.
• As we are going to make using the scanner the default, we don't need the devStrategy options at this point.
• Since we are using the scanner, and we now have the infra in place to detect the end of the static imports crawling, we can avoid full page reloads by delaying sending the optimized deps to the browser until the crawling is done and then check if we have all the deps needed to render the page. How it was before:

After this PR:

Same findings:

• Using the scanner should fix:
  • optimizeDeps does not process aliased imports #8867.
    The problem here is that the preAliasPlugin uses the scanned deps for its logic. We should refactor it to resolve each aliased bare import. We may need to inline and modify Rollup plugin alias for this. I'll tackle this in a future PR once I add back the possibility to run dev without the scanner
• For the same reason, we need the scanner in build time until the preAliasPlugin is changed. This PR adds it back.

[patak-dev]

It took me a while to find the race condition that was causing the errors. I think some of the errors we've seen lately may be related to the SSR devs optimizer running before the scanner (or when the scanner was not even run).

We need to find a way later to implement the preAlias plugin without the scanner. Now only if the scanner finds an aliased and optimized dependency, the plugin works correctly. I think it is the only place left where the scanner needs to be awaited. For the moment, the PR implements guards to start the optimizers after the scanner is done. I'll try to modify the preAlias plugin after v3 is out.

[patak-dev]

@sapphi-red @bluwy I'm using this PR to check the CI flakiness, since after enabling the scanner again it becomes more evident
I think I found one of the main issues. We had several playgrounds with two or three .spec.ts files. Each of them would trigger the creating of a default dev server, so we had race conditions with two or even three dev servers scanning, creating caches with the same dir name, etc.
Two different cases:

• For source maps we had the pattern:
  • __tests__/serve.spec.ts
  • __tests__/build.spec.ts
    We could extend vitestSetup to support this, but for the moment I combined them in a single spec.ts.
• In the CSS playground we had two extra .spec.ts that were starting their own servers. But if we don't add a serve.ts in the same folder, they will also start a default server. So for the CSS playground, we had three servers up in CI.

This explains why the CSS tests were one of the most usual to fail, and also the sourcemaps had failures regularly.

I wasn't checking this myself, so it is something to be aware of when we review PRs in the future. This commit in the PR removes these: 6c72736

I'm still fixing other things, so fails are expected

[patak-dev]

@sapphi-red I see why you did the build.spec.ts/serve.spect.ts split. The snapshot takes the name, so they are competing between server and build now that I mixed them into one. Any ideas? If not I'll have to revert the change and give support to the pattern in vitestSetup 🤔
We could check if build.spect.ts is used, then the dev server isn't started, and server.spec.ts, then the build server is not run.

[sapphi-red]

@patak-dev I don't completely grasp the situation but maybe using inline snapshot helps? (It makes the test file long and messy so it is not a good choice though...)

[patak-dev]

@sapphi-red the problem is that vitest is deleting the snapshots if we call pnpm run test-build -u because they are skipped. This may count like a bug in vitest, or maybe a feature request. I think at one point we should replace runIf by onlyIf and show all tests grayed instead of yellow/skipped like now, and these tests could ignore -u for their snaphots.
I think this may not be a new issue though, so we can move on and later find a good solution.
I pushed hints so the cases are easier to find in the snapshot file

[patak-dev]

@bluwy I tried to implement auto-exclude of explicit external deps here:

• fix: SSR deps external/noExternal optimization

You can see the tests in that commit working well.

But VitePress failed to build with it. Vue is setting itself as ssr.external in plugin-vue. But normally it is optimized for the browser. I'm starting to think that we can't safely use noExternal and external to define which dependencies should be optimized during SSR.
I think we should remove these heuristics in v3. I did so in this commit:

• fix: don't collide external/noExternal with optimizeDeps

@bholmesdev this may also affect Astro, as I think you were counting on having noExternal optimized automatically.

So if someone has an issue with a noExternal dep, they need to manually add it to optimizeDeps.include.
We will still be using the same optimizeDeps for SSR and non-SSR deps. During build time we can use ssrBuild to conditionally add diff deps. But during dev it isn't possible.
I'm thinking that we should provide a way to configure optimization for SSR only during dev using a function:

optimizedDeps: ssr => {
  if (ssr) {
    return {
      include: [ ... ]
      exclude: [ ... ]
    }
  } 
}

Or we could have a function like include: string[] | (ssr: boolean) => string[], and also exclude, esbuildOptions. Both look ok to me, leaning towards a general ssr function at the optimizeDeps level that seems to be the most flexible one.

What do you think? I would implement this in another PR, and it is a feature we could add later also.

Did another run of vite-ecosystem-ci and the svelte tests are still failing with the same error: https://github.com/vitejs/vite-ecosystem-ci/runs/7171919769?check_suite_focus=true

Maybe optimizing during SSR shouldn't be shared at all? And if we don't get the functional form in optimizeDeps it means that it only applies for the browser?
This feels safer, first because of how the ecosystem has been using optimizedDeps, and second because both envs are quite different and you may not want to optimize in the same way.

[patak-dev]

The function form could cause issues in the ecosystem, as there are a lot of plugins that are expecting an object only there. I think it would be better to keep things as objects. We could do:

optimizeDeps: {
  ...
},
ssr: {
  optimizeDeps: {
    ...
  },
  ...
}

That I think is better compared to optimizeDeps.ssr because we can start a pattern of centralizing everything related to ssr under the same entry, instead of having several ssr objects around the configuration.

Since we are discussing this API, @DylanPiercey proposed a few days ago having something similar using overrides:

  optimizeDeps: {
    include: ['stuff included in all targets'],
    exclude: ['stuff excluded in all targets'],
    overrides: {
      node: {
        include: ['only included for node']
      },
      webworker: {}.
      browser: {}
    }
  }

I think that if we want something like this, it is already better to use multiple config files. We need the entry in ssr because during dev, we only have one config.

[patak-dev]

We discussed with @bluwy that we should merge to continue to build on top of this PR. There is an issue in vite-plugin-svelte with this but looks like it is on the plugin side. And it may be resolved once we correct the noExternal/optimizeDeps logic in a PR I'll send next

[Veath]

Excuse me, why does the scanner dependency not exist in the crawling discovered dependency

[bluwy]

This line checks that when we crawl for dependencies, those that are in scanDeps (already optimized), shouldn't be added as a missing import since it's already optimized.

[Veath]

There is one point that I don't quite understand.

Dependencies found during the scan phase will be stored in metadata.discovered, and then runOptimizeDeps will generate result metadata.optimized(scanDeps). In the craw phase, crawlDeps should contain all the dependencies of the scan phase (scanDeps). Think this conditional statement will not hit, right?

[bluwy]

Sorry for the late reply. I think it's a bit out of my territory now since I haven't been following up on this part of the code. Maybe @patak-dev knows more about this. Also, are you suggesting of a perf improvement, or a bug?

[patak-dev]

With the current code, it could still hit, but I think you have a point @Veath. We should move

vite/packages/vite/src/node/optimizer/optimizer.ts

Line 572 in 881050d

const crawlDeps = Object.keys(metadata.discovered)

after the await of the scanner

vite/packages/vite/src/node/optimizer/optimizer.ts

Line 579 in 881050d

const result = await postScanOptimizationResult

and then it should never hit. PR welcome if you would like to tackle that

[Veath]

I think this part of the code can be deleted.

vite/packages/vite/src/node/optimizer/optimizer.ts

Lines 608 to 612 in 881050d

	for (const dep of scanDeps) {
	if (!crawlDeps.includes(dep)) {
	addMissingDep(dep, result.metadata.optimized[dep].src!)
	}
	}

I understand it this way. The scan phase found foo, bar dependencies. A new dependency C is found during the crawling phase, then at the end of the crawling, it should be scanDeps = [foo,bar], crawlDeps = [foo,bar,C].

The precondition for executing this part of the code is scanDeps = [foo, bar], crawlDeps = [C], Will there be such a scene? @patak-dev