feat: non-blocking scanning of dependencies

[patak-dev]

Description

For context, first read:

• feat: non-blocking pre bundling of dependencies #6758

#6758 moved the pre-bundling of dependencies after the server start. But the initial scan phase was still blocked. The scanning is fast thanks to esbuild, but the time it takes will grow with the user code. Depending on what dependencies the project uses, the scanning phase could take longer than the pre-bundling itself.

This PR completes the work started by #6758 by moving scanning after the server start, and letting the requests flow and be processed in parallel.

A refactoring was needed to implement this feature. I think how the different pieces interact are more clear after this PR, and even if the scanning will be blocking, I would advocate merging the refactoring.

Main changes:

• Moving from server._optimizeDepMetadata and server._registerMissingImport to server._optimizedDeps which is an object where we can have the metadata and related functions. I think we could move some of the free functions that takes the metadata to this object (like we do with moduleGraph), but I wanted to keep the changes focused on the feat in this PR. We can do other refactorings in future PRs.
• Before this PR, we used _registerMissingImport as a flag to know if we were scanning (if it was null, then we were resolving for the scan phase). We can no longer use this scheme as we are processing requests in parallel and we will have some resolve calls from the scan process intermixed with the regular requests. So, there is a new scan option passed to resolveId (as we do with ssr) and we have a server._optimizedDeps.scanProcessing promise which lets us await until scanning is over when resolving bare imports.
• The PR also Updates the log messages for the initial scan phase and pre-bundling. The old messages were more verbose, and it wasn’t a problem because they appeared before the server start message and the console was cleared. The new messages occupy only three lines to avoid hiding the server URLs.

Note: Since we are processing requests in parallel to the scan phase we could allow their resolution to progress (or some of them) and register them as missing imports. Then we can aggregate the scan phase dependencies with the discovered deps potentially saving a page reload. I found this problematic though, as we can't safely cache in the browser the resulting deps. See related explanation at #7378. Maybe this could be further explored.

Tested with Vaadin Start and with Ladle.

More testing is still required, but we may want to merge this one during the 2.9 beta so we only test the changes to pre-bundling in this minor and we don't need to repeat these again for 3.0.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

[patak-dev]

✅ all green vite-ecosystem-ci run against this PR

• full run, only fail was vite-plugin-ssr, doesn't look like a Vite error
• retry vite-plugin-ssr, so seems like a flaky test

We could make the new scan option to resolveId as @experimental or @internal so people don't start relying on it if we would like to merge this one to test. I think the option makes sense though in the same way as we currently have ssr.

[antfu]

This might break https://github.com/antfu/vite-plugin-optimize-persist since it's relying on some internals. But what is great is that we might be able to completely deprecate it in favor of these improvements! 🚀

[patak-dev]

This might break antfu/vite-plugin-optimize-persist since it's relying on some internals. But what is great is that we might be able to completely deprecate it in favor of these improvements! 🚀

Oh, interesting. IMO it may not be needed anymore as you said. FWIW, I checked the code of vite-plugin-optimize-persist, and looks like it will not fail with a hard error, it will just not update since _optimizeDepsMetatada never gets assigned. We could avoid the change but I think it is good that the metadata starts to be controlled by _optimizedDeps so we can avoid leaking this abstraction into the server with time (like we do with the module graph). A new version of optimize persist could check for both _optimizeDepsMetadata and _optimizedDeps.metadata if the plugin isn't going to be deprecated.

[antfu]

Not at all. I mean the good side, it's much better not needing it :) No need to change anything

[patak-dev]

This PR is now ready to be merged.

All green vite-ecosystem-ci run here:
https://github.com/vitejs/vite-ecosystem-ci/actions/runs/2019124256

Also tested it in Laddle and Vaddin Start.

I ended up removing alteredFiles as it can now be calculated directly as needsReload in optimizer/registerMissing.ts. The coupling between this file and optimizer/index.ts has been reduced. I hope that the flow is more clear now, probably better to look at the end result than the changeset for this one.

[patak-dev]

Since the scanning is performed after the server start, this PR needs to update the way we log dependencies, here is an example:

[sapphi-red]

Thank you for the improvement!
It was also working well with my project.

[bluwy]

Left some comments/questions below, but the rest looks great! It looks a lot cleaner than before.

[Shinigami92]

I'm not enough deep into this as I could make a real review, but from a code review I can accept this ^^

[patak-dev]

This is how it looked before for icones (beta.1):

And how it looks now for icones:

Interesting that a page reload isn't needed, but it makes sense as that two dependencies are unrelated to Vue. In beta.1, the check for altered files was broken due to paths mismatch

I wonder if it would make sense to add a delay before logging the optimized deps so we avoid the multiple lines in case there are missing deps

[patak-dev]

I wonder if it would make sense to add a delay before logging the optimized deps so we avoid the multiple lines in case there are missing deps

Done at: e55d8a9

This is how icones looks in 2.8.6. There were 2 page reloads needed on cold start

It is interesting that for icones, the server start isn't faster than in 2.8.6, I wonder if the used plugins may affect this. With only the Vue or React plugin I'm seeing 200ms start times.

@antfu 💚

[patak-dev]

Looks like some tests are failing because of timeouts, maybe 30sec isn't enough. I will try to raise it in another PR.

CI was working better though lately compared to the fails we are getting in this PR, so we may still need to squash a bug here. I think we should merge it though and get people to test it to have more time during the beta

[bluwy]

LGTM 🚀

I wonder if it would make sense to add a delay before logging the optimized deps so we avoid the multiple lines in case there are missing deps

Done at: e55d8a9

I think it's nice to have for DX as long as in debug mode, we can see the separation of deps pre-bundled, as it may help with debugging some issues/support in the future.

[patak-dev]

I think it's nice to have for DX as long as in debug mode, we can see the separation of deps pre-bundled, as it may help with debugging some issues/support in the future.

Indeed, when debugging with vite:deps, the timeout isn't there and we get all the verbose logs from before, including separated logs for each discovered batch

[patak-dev]

Example of new logs running Laddle and opening a few stories (each story will discover a new set of dependencies). I still think that we should only show the last message, but maybe it is too much of a change for 2.9 and we can discuss it for 3.0. I don't think that it is useful to a regular user to know when their dependencies were optimized, it is an implementation detail for Vite.

Merging this now so we can do a new beta patch and get some more testing.

[zheeeng]

Thx for your great work and I could close my issues #6677

[patak-dev]

Thanks for the feedback @zheeeng! If you have a screenshot before and after, please share. And if you are interested we are discussing hiding the logs in the normal case #7419

[zhangyuang]

LGTM, 2.9.0 test succeed in my ssr framework without full reload when find new dependencies