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: build time esbuild deps optimization (using proxies) #8265

Closed
wants to merge 6 commits into from

Conversation

patak-dev
Copy link
Member

Description

At #4921, Evan proposed:

Use esbuild to also pre-bundle dependencies for prod, then alias deps to the pre-bundled chunks during the Rollup build phase. This would

  • allow us to drop @rollup/plugin-commonjs and reduce prod/dev inconsistency due to difference in handling CJS between esbuild & @rollup/plugin-commonjs
  • allow us to cache the pre-bundled deps like the dev deps. Although they would still need to be processed by Rollup, it now needs to process fewer files and no longer need to do CJS transforms (essentially these are done by esbuild instead). Could potentially speed up the build to some extent.

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 workers

We 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:

  • Push users to add missing deps to optimizeDeps.include. Log a message explaining why it is important, or even explore doing it automatically when a missing dep is discovered.
  • We may need a 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)
  • Ask users to commit a part of the cache to the repo. This may add more complexity, I think the config option is better.
  • Is there a better way to re-create the bundle when there is a missing dep? Maybe rollup watch could be used to get some incremental gains?

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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label May 21, 2022
@benmccann
Copy link
Collaborator

nested-deps: the importAnalysisPlugin uses the module graph, we have to do something equivalent (@bluwy maybe you could help me here?)

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

@patak-dev
Copy link
Member Author

@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.

@patak-dev
Copy link
Member Author

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:

  • We don't perform any scanning during build
  • We need to block because of the CJS interop logic in importAnalysisBuild. So we avoid blocking user code processing with one of these:
    • Merging feat: non-blocking needs interop #7568
    • Using this.load in importAnalysisBuild transform to do a trick similar to what we do during dev with eager processing. See the second example here https://rollupjs.org/guide/en/#thisload
    • In importAnalysisBuild we start attaching a ?optimized-interop={importExpressionHash}, and then we can do the interop logic at load time through an intermediate proxy. Since we are building we don't have the dev time issue of exploding the requests the browser needs to perform.
  • We detect somehow that rollup is only awaiting for the optimized deps promises, and then do a single optimization of all deps (if needed, all deps may already be cached)

@patak-dev
Copy link
Member Author

In importAnalysisBuild we start attaching a ?optimized-interop={importExpressionHash}, and then we can do the interop logic at load time through an intermediate proxy. Since we are building we don't have the dev time issue of exploding the requests the browser needs to perform.

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 transform hook, so if there are still user files being processed processing of optimized dependencies will be avoided and only once performed at the end.

@patak-dev
Copy link
Member Author

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 think we should merge #7568, as it will simplify a lot the implementation and we can go back to doing the interop in place as needsInterop is known without the need to wait for the optimization.

@patak-dev patak-dev changed the title feat: build time esbuild based deps optimization feat: build time esbuild deps optimization (using proxies) May 22, 2022
@bluwy
Copy link
Member

bluwy commented May 23, 2022

I haven't checked #8280 but

  • nested-deps: the importAnalysisPlugin uses the module graph, we have to do something equivalent (@bluwy maybe you could help me here?)

It seems like the config.optimizeDeps.include isn't used in build. IIUC this PR continuously registers missing imports as it goes, when the include wasn't taken into account? Perhaps we should also incorporate it

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

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 optimizeDeps.extensions out of experimental stage for it to be relied on in Vite I think.

@patak-dev
Copy link
Member Author

It seems like the config.optimizeDeps.include isn't used in build. IIUC this PR continuously registers missing imports as it goes, when the include wasn't taken into account? Perhaps we should also incorporate it

Oh, yes! We don't do scanning now during build in this PR either, and include was added as part of that phase. I'll check it out, this should fix two of the three skipped tests from the last commit (the third one is the ssr-vue /external issue that I'm having issues reproducing locally)

@patak-dev
Copy link
Member Author

Closing in favor of

@patak-dev patak-dev closed this May 25, 2022
@antfu antfu deleted the feat/esbuild-for-deps-at-build-time branch March 8, 2023 10:52
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
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants