-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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: build time esbuild deps optimization (using proxies) #8265
Conversation
We could maybe get rid of nested deps in v3 if @bluwy's prebundling of Svelte libraries is able to be enabled by default? I'm not sure if that's the long-term plan or what's left to do there |
@benmccann I think there isn't an issue with supporting nested deps, so I think at least in the scope of this PR we should try to support them. |
About how to deal with missing dependencies, maybe there is a way to avoid the issue by waiting until every user code has been processed. So:
|
This is now implemented by: fbd4c0f. If we merge #7568, is possible that the implementation could be simplified and proxies aren't needed. What I'm doing is issuing a delay in a |
The error is due to dynamic import interop. I tried to port it from importAnalysis but we need to do it in place, I don't think it is possible to do the interop in a proxy. But this means waiting for the needsInterop flag so this may be a dead end. |
I haven't checked #8280 but
It seems like the
There's one more thing dominik raised, that's a blessed way to trigger re-optimization on server restart. Currently it's using a small hack to trigger that. I haven't got to it yet. We'd also need |
Oh, yes! We don't do scanning now during build in this PR either, and |
Closing in favor of |
Description
At #4921, Evan proposed:
This PR is an initial exploration of the idea. Code isn't abstracted at this point (especially in importAnalysisBuild) to be able to play with the POC more easily.
POC
There is a new option
config.build.optimizeDeps
that is true by default. If it is enabled, the@rollup/plugin-commonjs
isn't included in the pipeline. And CJS compat is achieved with esbuild + es interop in the same way as we have now during dev. If we end up implementing this feature, I think we should have an option to let people keep using the commonjs plugin, maybe until v4?The dev deps cache can't be used during build as env variables could be different, so we now have a new
depsBuild
folder that caches the deps during build time. At this point dev and build caches are completely separated but we can use the dev metadata during build to avoid a scan for example (or the other way around).I had to modify the react plugin because it is currently doing some tricks to optimize bundle size that aren't compatible with the optimize cache. But something similar could probably be added back. See discussion here for more context.
CI
All tests are using esbuild optimization during build, except for:
nested-deps
: the importAnalysisPlugin uses the module graph, we have to do something equivalent (@bluwy maybe you could help me here?)worker
: we need to check how to properly handle workers bundling, I didn't spent much time but is esbuild optimization currently disabled for workersWe also need to check
build --watch
. Right now deps optimization is disabled for watch mode. It may be a matter of watching the buildDeps folder for changes.SSR builds are also avoiding the use of esbuild optimized deps. We should also explore this.
Possible issues
One of the biggest issues with this scheme is how to deal with missing dependencies. In the PR, what I end up doing for the moment is to call rollup again if a missing dependency is discovered. It doesn't look that bad because the generation is only performed once, but this means that if scanning doesn't catch every dep, build time will be increased (+75%?) the first time is run (then the cache kicks in). But for CI, we will never have the cache. We talked a bit with Evan in the last meeting about this problem. Some initial ideas:
optimizeDeps.include
. Log a message explaining why it is important, or even explore doing it automatically when a missing dep is discovered.optimizeDeps.lazyInclude
? At least for laddle, they preferred having missing deps because optimizing all the deps of a design system was too slow for them (it may also be that they were using flow instead of typescript, so maybe this is a non-issue)Another possible problem is increased bundle size. The way we handle es interop for CJS isn't as compact as the one used in rollup. I think that when minified and gziped, the bundles shouldn't be that different but we need to review this. A possible improvement is to avoid checking for
__esModule
when we know the dep is CJS, we have this information and we aren't using it in the interop code. Another possibility is to use the name of the default if it exists instead of creating a new__vite__cjsExportDefault_xxx
safe name. I tried both and they help, but I didn't include them in the PR as they aren't important enough right now.What is the purpose of this pull request?