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

fix: avoid optimized deps during build in worker bundles #13275

Closed
wants to merge 1 commit into from

Conversation

patak-dev
Copy link
Member

Description

After #11218, we introduced a regression when deps optimization is enabled during build. Since v3, the way deps optimization works is that dependencies imported in workers are optimized together with the dependencies of the main app. For this to work, given that each worker is bundled by a different rollup process, we were awaiting for all the workers to be bundled before doing the optimization run with esbuild (as we need to know during build every dep that should be optimized before, we don't have reloads at build time). We had then a registerWorkerSource scheme so we avoid waiting for the worker id, and instead wait for the registered ids inside the worker. For this scheme to work, all the workers need to be processed in parallel and #11218 serialized the workers. This means that there could potentially be a deadlock if there are deps imported by several workers.

Unrelated to #11218, we have an issue with maxParallelFileOps. Rollup is willing to give us better tools to avoid this problem (see rollup/rollup#4985). Using delayLoading would be very clean, but if we need to also use it to await for the worker bundles, we are back to registering worker sources and adding extra complexity. It should be doable though, but we also need to revert #11218.

This PR explores not using deps optimization when bundling workers. I still don't know if this is acceptable, but I don't know how we can work out optimizing deps for them if they aren't bundled in parallel. Even if we force the user to include all the deps in the worker by hand, it won't work. Sending the PR in case others would like to also check this problem, I don't think we should merge it yet.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented May 19, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev patak-dev mentioned this pull request May 31, 2023
9 tasks
@patak-dev
Copy link
Member Author

Closing this as we have merged #14685, and we are back at doing parallel builds for workers. Optimizing dependencies in workers is possible now, but I no longer think that this is what we'll be recommending in the future. With rolldown, there shouldn't be the need for pre-bundling during build, or it will be tackled directly by its own caching.

@patak-dev patak-dev closed this Nov 14, 2023
@patak-dev patak-dev deleted the fix/use-commonjs-plugin-for-worker-bundles branch March 22, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant