fix(esm_library): extract TLA shared modules to break circular dependency#13606
fix(esm_library): extract TLA shared modules to break circular dependency#13606
Conversation
github-actions
Bot
added
team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75e0197b0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR addresses a circular dependency that can occur in ESM library output when top-level await (TLA) causes an async chunk to be awaited by an ancestor chunk while also importing shared modules back from that ancestor. It adds a chunk-graph optimization pass to extract such shared modules into split chunks (breaking the cycle) and emits a warning when TLA is detected in ESM library output.
Changes:
- Add
extract_tla_shared_modules()to scan async chunks and extract ancestor-resident dependencies into split chunks. - Invoke the new extraction pass during
CompilationOptimizeChunksand emit a TLA warning diagnostic. - Extend optimize-chunks utilities with dependency-graph traversal for identifying problematic cross-chunk dependencies.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/rspack_plugin_esm_library/src/plugin.rs | Calls the new extraction pass during optimize-chunks and emits a TLA warning diagnostic. |
| crates/rspack_plugin_esm_library/src/optimize_chunks.rs | Implements extract_tla_shared_modules() to detect and extract shared modules from ancestor chunks to break TLA-induced cycles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merging this PR will degrade performance by 1.29%
Performance Changes
Comparing 
|
Rsdoctor Bundle Diff AnalysisFound 6 projects in monorepo, 0 projects with changes. 📊 Quick Summary
Generated by Rsdoctor GitHub Action |
📦 Binary Size-limit
❌ Size increased by 16.16KB from 49.39MB to 49.41MB (⬆️0.03%) |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0f9d9dbec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
JSerFeng
force-pushed
the
fy/cranky-chaplygin
branch
from
cc11129 to
9f1b14a
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41336613cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ency When using ESM library output with top-level await (TLA), a circular dependency occurs: the main chunk awaits the async chunk (because it has TLA), but the async chunk imports shared modules from the main chunk. This adds `extract_tla_shared_modules()` which scans async chunks' dependency graphs and extracts modules that reside in ancestor chunks into separate chunks to break the cycle. Also emits a warning when TLA is used with ESM library output, as the bundled execution order may differ from the original source.
- Reconnect entry modules to new chunk with entrypoint metadata after split, preventing entrypoints from losing their startup module - Group extracted modules by their source chunk set to avoid duplicating a module into multiple new chunks when it appears in multiple ancestors - Filter inactive connections via is_target_active() during BFS to avoid over-approximating the dependency graph - Fix doc comment: clarify that early exit skips chunk-graph scanning, not all computation
- Fix cargo fmt: merge std imports, fix line break - Change extract_tla_shared_modules to return true only when modules were actually extracted, not just when TLA exists - Only emit the warning diagnostic when a circular dependency is found and shared modules are extracted, avoiding false warnings in tests that legitimately use TLA
- Remove redundant has_tla early exit since async_chunks.is_empty() already covers the no-TLA case - Use is_only_initial instead of can_be_initial to also catch chunks that belong to both initial and non-initial groups
Replace per-async-chunk independent BFS with a single BFS pass sharing one visited set across all async chunks. Pre-compute a reverse map (chunk → which async chunks consider it an ancestor) to enable O(1) ancestor lookups during traversal. Complexity: O(N + E + A × G) down from O(A × (N + E + G)) where N=modules, E=edges, A=async chunks, G=chunk groups.
Remove test.filter.js from split-sync-modules-{1,2,3} tests that were
skipped with "cycle caused by available modules". These tests exercise
the exact circular dependency scenario that extract_tla_shared_modules
now fixes.
Add warnings.js to expect the TLA circular dependency warning that is
emitted when shared modules are extracted.
- Fix clippy collapsible_if lint by merging nested if-let + if - Restore test.filter.js for split-sync-modules tests: the chunk extraction alone is not sufficient to break the cycle in ESM rendering — additional render-side changes are needed - Add side_effects_state_artifact param for is_target_active API change
Remove test.filter.js so the tests actually execute, and add warnings.js to expect the TLA circular dependency warning.
Previously we detected "async chunks" as non-initial chunks containing a module marked async. This missed the actual cycle scenario: the async chunk's modules themselves are sync; what matters is that the importing module has TLA and uses await-import to load the chunk. New detection: scan all modules with has_top_level_await, follow their DynamicImport dependencies, and mark the non-initial target chunks as potentially problematic. This correctly covers the split-sync-modules test cases where the imported chunk re-exports from a lib that sits in the parent chunk due to available-modules dedup.
Previously the extraction reconnected ALL moved modules as entry modules to every initial entrypoint group of the new chunk. This was wrong — only modules that were ORIGINALLY entry modules in their source chunks should be reconnected, and only to their original entrypoint group. Wrongly promoting shared library modules (like lib.js) to entrypoint status corrupted the chunk graph and caused runtime deadlocks. Key changes: - Only reconnect modules as entry modules if they WERE entry modules in some source chunk (tracked per-source-chunk via get_chunk_entry_modules_with_chunk_group_iterable lookup before disconnecting) - Use the ORIGINAL entrypoint group when reconnecting, not "all initial groups of the new chunk" - Skip DynamicImport dependencies during BFS — they produce promises and don't force the target chunk to be evaluated synchronously, so they can't cause the TLA deadlock - Set chunk_reason for debuggability This mirrors the pattern used by RemoveDuplicateModulesPlugin.
Run with UPDATE_SNAPSHOT=true locally confirms all 3 tests pass: lib.js is extracted into a separate chunk, main.mjs and async.mjs both import from it — no circular dependency. All 198 ESM output tests pass, no regressions.
Previously the BFS used a shared visited set with single-origin tracking per module. This was buggy when a module was reachable from multiple async chunks whose ancestors differ: - Module M lives in chunks [P1, P2] - P1 is ancestor of async chunk A1, P2 is ancestor of A2 - A1's BFS reaches M first, records extract from P1 - A2's BFS skips M via visited, never records extract from P2 - Result: cycle between A2 and P2 remains Fix: run an independent BFS per async chunk with its own visited set. Each module is analyzed in the context of the async chunk actually traversing it, so extractions for different ancestor→async edges don't collide. Complexity goes from O(N+E+A·G) back to O(A·(N+E+G)), but correctness trumps the marginal speedup — A is usually small. Also add a note about the Phase 1 over-inclusion of function-scoped import()s; accepted as safe because extraction only fires when real cross-chunk static cycles exist.
JSerFeng
force-pushed
the
fy/cranky-chaplygin
branch
from
04ed988 to
a4bf205
Compare
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4bf205133
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
When an ancestor module (sharedA) itself imports another module (sharedB) also in an ancestor chunk, the BFS must continue into sharedA to discover sharedB. Previously the BFS stopped at ancestor boundaries, only extracting direct dependencies but leaving their transitive deps in the parent chunk — preserving the cycle. Now the BFS continues into both in-async-chunk targets AND ancestor-resident targets, ensuring the full transitive closure of static dependencies is extracted.
Summary
await import()s the async chunk (because it has TLA), but the async chunk imports shared modules back from the main chunk via the "available modules" optimization.extract_tla_shared_modules()in theoptimize_chunkshook that scans async chunks' dependency graphs and extracts modules residing in ancestor chunks into separate chunks to break the cycle.Link
web-infra-dev/rslib#1573
Test plan
cargo check -p rspack_plugin_esm_librarypassescargo test -p rspack_plugin_esm_library— all 19 tests pass