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

feat: non-blocking pre bundling of dependencies #6758

Merged
merged 29 commits into from Mar 3, 2022

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Feb 4, 2022

Close #6508, #6543
Related #6654

Description edited to reflect latest state of the PR

Description

This PR refactors Vite's pre-bundling to achieve:

  • Non-blocking cold server start: Until now, when starting a project for the first time, the Vite dev server needed to pre-bundle all discovered dependencies (scan phase + optimizeDeps.include). We only need to wait for the scan phase now, and pre-bundling is done in the background.
  • No need to full-reload the page if the pre-bundled dependencies are stable (the prev optimized chunks are the same).
  • Non-blocking requests when pre-bundling: When processing, Vite was blocking every request. We now allow requests to flow through the pipeline improving initial cold start load speed and increasing the chances of discovering new missing dependencies when re-processing and letting Vite populate the module graph and the browser to process files (in case of stable optimization they will be used as-is)

Related PRs

Allowing PRs to flow through the pipeline exposed race conditions related to the in-fly requests when invalidating a module. These issues were present before but it was harder to hit them. The race conditions were fixed by:

Avoid corrupt cache after moving a Vite project folder:

Fixes:

New cache structure

We discovered edge cases because of emptying the cacheDir while pre-bundling. fix: sync swap of new processed deps in disk 7f51389 reworks the optimizer to pre-bundle to a temporal folder, swapping the result in sync once it is done. This should resolve other issues reported before this PR, where a source map was requested but was deleted as part of a reprocessing.

The commit also fixed an unrelated bug. We were deleting the whole cacheDir, and with it the https certificate. Now the optimized dependencies will be at cacheDir/deps, so we can safely delete that without touching other caches. For compat, the cacheDir is emptied if the old structure is found. While processing, the deps live at cacheDir/processing. This should also ensure that the cache doesn't end up in a corrupted state if there is an error while processing.

Changes to the Transform Middleware

We no longer block requests while processing, and that was done before in this middleware. We now need to block as deep inside the pipeline as possible:

  • When an optimized dependency entry point is loaded from the disk (see optmizeDepsPlugin)
  • When we find a URL of an optimized dependency entry point when during import analysis (as we need to know if es interop is needed for it). After this PR is merged, we should check if we can avoid waiting here, as this is now blocking the browser from processing files and discovering other missing deps. See feat: non-blocking pre bundling of dependencies #6758 (comment)

Handling of optimized deps source maps has been also added. To avoid warnings in the browser, we return empty sourcemaps for outdated requests. We confirmed in a large app that this works correctly.

And a 1 second timeout was removed and it isn't performed when waiting for loading in case re-processing takes more time.

New optimizedDepsPlugin

A new serve only plugin is introduced to load optimized files after waiting for its processing promise to be fulfilled. Files from the cache were loaded through the regular fallback before, but we now block at loading time.

Shortcircuit the resolve id for optimized deps URLs

This wasn't needed before in the resolve plugin, but we need to resolve these Ids when the files for the processed deps don't yet exist (something that later checks in the resolve plugin will check for).

Refactor tryNodeResolve

server._registerMissingImport now returns the resolved id. Before it was only registering the dep as missing and the regular node_modules id was returned. The path to where the optimized dep will be in the cacheDir is known, we pre-create a browserHash for it, and we no longer add the &es-interop suffix. So this is still a sync call.

No longer provided: optimizeDeps.holdBackServerStart

We decided that we aren't going to expose a new option, and wait until there is a bug report or feat request to add this.

There is a new config option optimizeDeps.holdBackServerStart. Hold back server start when doing the initial pre-bundling. Default is false. Set to true for testing purposes, or in situations where the previous strategy is required. I don't know if this is really needed, but it feels safer to let users opt-in to the old behavior if there are cases where initial pre-bundling is so long that the browser could timeout the requests.

OptimizedDepInfo.browserHash

We have now a browserHash per dependency info instead of the global one in optimizeDepsMetadata. This is needed as missing deps are assigned a browserHash when discovered. If files are stable after re-processing, these hashes are kept avoiding a full-reload

OptimizedDepInfo.fileHash

Each optimized entry point gets now a fileHash that takes into account its imports to generate a hash that lets us know if an optimized bundle has changed after re-processing.

OptimizedDepInfo.processing

Each dependency info gets its own processing promise so we can block the load of the file until it is available and avoid blocking other requests. _isRunningOptimizer and _pendingReload global server states are no longer needed

DepOptimizationMetadata.discovered

Discovered dependencies are now exposed to the pipeline so the new optimizeDepsPlugin can await until they are processed (see also currentMissing -> metadata.discovered)

Removal of &es-interop suffix

We don't know when resolving node dependencies if ES interop is needed as esbuild may still be processing it. Instead of using a suffix in the URL, we now find out if ES interop is needed by looking at the OptimizedDepInfo in the importAnalysisPlugin where is needed (after awaiting the processing promise for it)

Note: hash is used instead of browserHash for files excluded from optimization. This is unrelated to the PR, but as we need to re-test quite a bit to be able to merge this one, I think we could also check it in. hash only depends on the lock file, but should be enough here AFAICS


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@Shinigami92 Shinigami92 added p2-nice-to-have Not breaking anything but nice to have (priority) YAO Yet another option... labels Feb 4, 2022
@patak-dev patak-dev added this to the 2.9 milestone Feb 4, 2022
@patak-dev
Copy link
Member Author

Writing down some ideas for further improvement related to pre-bundling:

ES Interop

We need to await the processing of dependencies at import analysis to add the ES interop code when rewriting imports. It would be great if we could only await at loading time, and let import analysis finish so the browser can keep processing and requesting other files. If there is a way to add the ES interop by patching the entry point generated by esbuild or by creating a virtual proxy to it, we should explore it.

Relative paths in _metadata.json

We currently use absolute paths in _metadata.json, this means that if the user moves or copy a Vite app folder, the cache is invalid. I think users may be getting hard to report bugs because of this. We could make the paths relative to the root to avoid this issue. @JessicaSachs proposed that users may commit their cache for some use cases, which may be interesting for small playgrounds that are used a lot to speed up things. Relative paths are also needed for this use case.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

The flow LGTM! I've tested this branch on a fairly big production app and pre-bundling works as usual. Have a few nitpicks below, but otherwise it's an approval from me.

packages/vite/src/node/optimizer/index.ts Outdated Show resolved Hide resolved
packages/vite/src/node/optimizer/index.ts Outdated Show resolved Hide resolved
packages/vite/src/node/optimizer/index.ts Outdated Show resolved Hide resolved
packages/vite/src/node/optimizer/index.ts Outdated Show resolved Hide resolved
packages/vite/src/node/optimizer/registerMissing.ts Outdated Show resolved Hide resolved
bluwy
bluwy previously approved these changes Feb 9, 2022
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Awesome 🚀

@patak-dev patak-dev mentioned this pull request Feb 10, 2022
9 tasks
@ElMassimo
Copy link
Contributor

Tested on a few îles sites and it's working nicely!

For example, when preact is used in async components, this patch prevents a full page reload when that new dependency is detected and optimized 👍

@patak-dev
Copy link
Member Author

patak-dev commented Feb 10, 2022

Thanks a lot for testing the PR @ElMassimo!

We discussed today with @bluwy and @dominikg, some points we got about the PR:

  • We should expose optimizeDeps.holdBackServerStart as @experimental, or avoid providing the option until we have a real use case.
  • It would be good to find a way to remove the es-interop logic from import analysis and push it to the dep somehow (to be done in a future PR, it is out of the scope for this one)

I pushed a fix for source maps that @vursen tested successfully in their app. He still saw some source maps warnings so there still is something missing there.

bluwy
bluwy previously approved these changes Feb 11, 2022
@patak-dev
Copy link
Member Author

@vursen confirmed that a637809 worked as expected, properly avoiding concurrent runs of optimizeDeps

I just pushed a new commit refactor: createOptimizeDepsRun, remove new option 44eca4e to continue the work from 3f9ec98 and clean up the PR after the latest changes:

  • We talked with @antfu and we think we shouldn't expose the optimizeDeps.holdBackServerStart to get the old behavior until it is requested by users, so it has been removed.
  • There is a new createOptimizeDepsRun function that should make the code easier to follow.
  • optimizeDeps has been now returned to the previous signature as it was exposed to the CLI.
  • In the rerun we are now waiting until the end of the optimizeDeps run to reassign server._optimizeDepsMetadata. This allows us to properly handle errors, and it may be the cause of the issue that @vursen was experiencing (still to be determined, we need to also check if returning 504 for staled dep request is proper here)

@vursen
Copy link
Contributor

vursen commented Feb 21, 2022

In the rerun we are now waiting until the end of the optimizeDeps run to reassign server._optimizeDepsMetadata. This allows us to properly handle errors, and it may be the cause of the issue that @vursen was experiencing (still to be determined, we need to also check if returning 504 for staled dep request is proper here)

Confirmed that d587774 fixes the issue where 504 errors were sometimes returned for some dependencies because of an outdated ?v=... parameter in the import statement paths. It is no longer reproducible in our project. Everything works flawlessly now. 🎉

@IanVS
Copy link
Contributor

IanVS commented Feb 28, 2022

I tested this out in my project which uses storybook-builder-vite. Previously, if I did not carefully include all of my dependencies in optimizeDeps.include (or via optimizeDeps.entry), I would get output that looked like this:

10:59:15 AM [vite] new dependencies found: chromatic/isChromatic, storybook-dark-mode, updating...
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/lodash_pickBy.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/lodash_isPlainObject.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/lodash_mapValues.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/memoizerific.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/synchronous-promise.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/lodash_pick.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/stable.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/fast-deep-equal.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/lodash_startCase.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/slash.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/doctrine.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/chunk-XYEMJMCR.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/html-tags.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/uuid-browser_v4.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/chunk-P2TIWM3V.js?v=568ff62e.
10:59:18 AM [vite] Failed to load source map for /node_modules/.cache/storybook-vite/escodegen.js?v=568ff62e.
Sourcemap for "/Users/ianvs/code/defined/webclient/node_modules/@base2/pretty-print-object/dist/index.js" points to missing source files
Sourcemap for "/Users/ianvs/code/defined/webclient/node_modules/@tippyjs/react/dist/tippy-react.esm.js" points to missing source files
10:59:42 AM [vite] ✨ dependencies updated, reloading page...

And in the browser, I would see logs:

[vite] connecting...
VM28 client:211 [vite] connected.
chunk-4N3UMKTO.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-3FZATABW.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-ZR4UDYUI.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-77C4GO2N.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-TMHQWTX7.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-4OGZZ5AR.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-BEJSN3AA.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-JQXBAWD4.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-CSKXKA4J.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-2RANQSEU.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-LIQEJE3L.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-VKFTJZB7.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-WUF4TTWO.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-QDP3QWDZ.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-FDN3XCKL.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-5ZRQL7D2.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-NGEFCRKM.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-XMG3TUVU.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-YA3IKP7Y.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-Z7WTX5YJ.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-SLELS7LP.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-J4BOMYHS.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-ZX4FRMCP.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-FUY4GZGX.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-AIYPGAYC.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-GRB5ERZS.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-WXRWA2OT.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-EQTRZVUY.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-UMMWF256.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-55S2WK4J.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-XDS4OTGE.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-IQ7ARXV5.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-3AYB2QZN.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-MJEAHNT3.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-PKK46IQF.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-WN2BKCIU.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-B6DWLJNF.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-SKK4DGKX.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-U3XFOMJ6.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-VIP2OIIR.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-5MMHKA33.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-P5B7VUU4.js:1          Failed to load resource: the server responded with a status of 408 (Request Timeout)
chunk-NLD4S4XA.js:1          Failed to load resource: the server responded with a status of 404 (Not Found)
chunk-37KLHXX3.js:1          Failed to load resource: the server responded with a status of 404 (Not Found)
DevTools failed to load source map: Could not load content for http://localhost:6006/index.js.map: HTTP error: status code 404, net::ERR_HTTP_RESPONSE_CODE_FAILURE
DevTools failed to load source map: Could not load content for http://localhost:6006/react-popper-tooltip.js.map: HTTP error: status code 404, net::ERR_HTTP_RESPONSE_CODE_FAILURE
client.ts:22 [vite] connecting...
client.ts:58 [vite] connected.

Now, with this PR, I see the following in the terminal:

10:50:40 AM [vite] new dependencies found: chromatic/isChromatic, storybook-dark-mode, updating...
10:50:53 AM [vite] ✨ dependencies updated, reloading page...

And in the browser, just:

VM27 client:184 [vite] connecting...
VM27 client:211 [vite] connected.
DevTools failed to load source map: Could not load content for http://localhost:6006/index.js.map: HTTP error: status code 404, net::ERR_HTTP_RESPONSE_CODE_FAILURE
preview.tsx:16          GET http://localhost:6006/node_modules/.cache/storybook-vite/deps/chromatic_isChromatic.js?v=5182ef10 net::ERR_ABORTED 504 (Gateway Timeout)
preview.tsx:18          GET http://localhost:6006/node_modules/.cache/storybook-vite/deps/storybook-dark-mode.js?v=022294b7 net::ERR_ABORTED 504 (Gateway Timeout)
client.ts:22 [vite] connecting...
client.ts:58 [vite] connected.

😍

I also confirmed that I had no problems when switching to this PR and running storybook without clearing my previous cache. Awesome work here! This has definitely been a source of confusion and problems for our users in the past, so I'm very glad to see that this is working much better now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Status: Todo
Development

Successfully merging this pull request may close these issues.

"Failed to load source map" warnings appear because of a race condition
8 participants