Extract the static dependencies imported by manual chunks into separate chunks#6374
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Thank you for your contribution! ❤️You can try out this pull request locally by installing Rollup via npm install rollup/rollup#fix/manual-chunk-optimizeNotice: Ensure you have installed the latest nightly Rust toolchain. If you haven't installed it yet, please see https://www.rust-lang.org/tools/install to learn how to download Rustup and install Rust. or load it into the REPL: |
Performance report
|
eff1ea6 to
14d635e
Compare
There was a problem hiding this comment.
Pull request overview
Updates Rollup’s chunk assignment logic for the function form of output.manualChunks when output.onlyExplicitManualChunks is enabled, so that static dependencies imported by manual chunks are extracted into their own chunks instead of being kept in entry-chunk atoms—avoiding certain circular chunk graphs and adjusting related warning output.
Changes:
- Adjust chunk assignment to extract static dependencies of manual chunks into separate chunks under
onlyExplicitManualChunks(function form). - Update the
CIRCULAR_CHUNKwarning wording for theonlyExplicitManualChunksscenario. - Add/refresh chunking-form fixtures and expected outputs to cover the non-circular and circular-warning cases.
Reviewed changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/chunkAssignment.ts | Implements dependency extraction for manual-chunk static deps under onlyExplicitManualChunks (function form). |
| src/utils/logs.ts | Updates the circular-chunk warning message text for the onlyExplicitManualChunks case. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_config.js | Adds a regression fixture config for the non-circular scenario. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/main.js | New fixture entry using dynamic imports. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/a.js | New fixture module. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/b.js | New fixture module. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/c.js | New fixture module. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/c2.js | New fixture module. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/c3.js | New fixture module. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/c4.js | New fixture module. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/c5.js | New fixture module. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/e.js | New fixture module. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/system/main.js | Adds expected SystemJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/system/generated-a.js | Adds expected SystemJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/system/generated-b.js | Adds expected SystemJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/system/generated-c.js | Adds expected SystemJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/system/generated-c2.js | Adds expected SystemJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/system/generated-c3.js | Adds expected SystemJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/system/generated-c4.js | Adds expected SystemJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/system/generated-e.js | Adds expected SystemJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/es/main.js | Adds expected ES output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/es/generated-a.js | Adds expected ES output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/es/generated-b.js | Adds expected ES output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/es/generated-c.js | Adds expected ES output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/es/generated-c2.js | Adds expected ES output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/es/generated-c3.js | Adds expected ES output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/es/generated-c4.js | Adds expected ES output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/es/generated-e.js | Adds expected ES output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/cjs/main.js | Adds expected CJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/cjs/generated-a.js | Adds expected CJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/cjs/generated-b.js | Adds expected CJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/cjs/generated-c.js | Adds expected CJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/cjs/generated-c2.js | Adds expected CJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/cjs/generated-c3.js | Adds expected CJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/cjs/generated-c4.js | Adds expected CJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/cjs/generated-e.js | Adds expected CJS output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/amd/main.js | Adds expected AMD output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/amd/generated-a.js | Adds expected AMD output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/amd/generated-b.js | Adds expected AMD output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/amd/generated-c.js | Adds expected AMD output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/amd/generated-c2.js | Adds expected AMD output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/amd/generated-c3.js | Adds expected AMD output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/amd/generated-c4.js | Adds expected AMD output for new fixture. |
| test/chunking-form/samples/non-circular-chunk-with-only-explicit-manual-chunks/_expected/amd/generated-e.js | Adds expected AMD output for new fixture. |
| test/chunking-form/samples/circular-chunks-warning-caused-by-only-explicit-manual-chunks/_config.js | Updates expected warning ids/message for the revised chunk graph. |
| test/chunking-form/samples/circular-chunks-warning-caused-by-only-explicit-manual-chunks/_expected/system/main.js | Updates expected SystemJS output for the circular-warning fixture. |
| test/chunking-form/samples/circular-chunks-warning-caused-by-only-explicit-manual-chunks/_expected/system/generated-b.js | Adds expected SystemJS extracted chunk output. |
| test/chunking-form/samples/circular-chunks-warning-caused-by-only-explicit-manual-chunks/_expected/system/generated-ac.js | Updates expected SystemJS output to reference extracted chunk. |
| test/chunking-form/samples/circular-chunks-warning-caused-by-only-explicit-manual-chunks/_expected/es/main.js | Updates expected ES output for the circular-warning fixture. |
| test/chunking-form/samples/circular-chunks-warning-caused-by-only-explicit-manual-chunks/_expected/es/generated-b.js | Adds expected ES extracted chunk output. |
| test/chunking-form/samples/circular-chunks-warning-caused-by-only-explicit-manual-chunks/_expected/es/generated-ac.js | Updates expected ES output to reference extracted chunk. |
| test/chunking-form/samples/circular-chunks-warning-caused-by-only-explicit-manual-chunks/_expected/cjs/main.js | Updates expected CJS output for the circular-warning fixture. |
| test/chunking-form/samples/circular-chunks-warning-caused-by-only-explicit-manual-chunks/_expected/cjs/generated-b.js | Adds expected CJS extracted chunk output. |
| test/chunking-form/samples/circular-chunks-warning-caused-by-only-explicit-manual-chunks/_expected/cjs/generated-ac.js | Updates expected CJS output to reference extracted chunk. |
| test/chunking-form/samples/circular-chunks-warning-caused-by-only-explicit-manual-chunks/_expected/amd/main.js | Updates expected AMD output for the circular-warning fixture. |
| test/chunking-form/samples/circular-chunks-warning-caused-by-only-explicit-manual-chunks/_expected/amd/generated-b.js | Adds expected AMD extracted chunk output. |
| test/chunking-form/samples/circular-chunks-warning-caused-by-only-explicit-manual-chunks/_expected/amd/generated-ac.js | Updates expected AMD output to reference extracted chunk. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6374 +/- ##
=======================================
Coverage 98.78% 98.78%
=======================================
Files 274 274
Lines 10789 10806 +17
Branches 2882 2882
=======================================
+ Hits 10658 10675 +17
Misses 89 89
Partials 42 42 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1922ca1 to
305c45c
Compare
c577a98 to
bd72a42
Compare
lukastaegert
left a comment
There was a problem hiding this comment.
Sorry for the long review on this one. I currently only have short times I can use for working on Rollup, and wrapping my head around this one took a little more time than that 😬
I pushed a change that inlined the entry atom detection to avoid the second iteration. But I think in general this is more like a fix for a symptom rather than the root cause, and I wonder if this problem could not re-appear on a higher level if you have multiple entries with the exact same dependencies that are also shared by a manual chunk.
I think the solution should go more in the direction of treating manual chunks like "dependent entries" with regard to the atom creation, even though they are not single modules.
So I wonder if it is possible to somewhat generalize the concept of dependent entries in general. Do you understand what I mean? Do you think it makes sense? I can also have a go, but it could take a moment.
|
Thanks a lot for taking the time to look into this so deeply, especially with your limited Rollup time! Yeah, I agree with you. When working on this PR, I also thought about a similar approach: refactoring the chunk atom creation logic and treating modules in manual chunks more like dependent entries. But that felt like a larger change and would take more time, so I went with the current symptom fix for now.
For this specific case, I do not think the issue should reappear, but I may be missing something 😅 I can try to move this towards the long-term solution, since I currently have quite a bit of spare time 😄 Of course, if you have already started looking into it, I’m happy to leave it to you to avoid duplicating the work. |
|
I want to give it a shot tomorrow morning, I will let you know how to go on then |
|
I pushed a solution similar to what I had in mind. So basically what it does is that for the purpose of generating chunks, it partially treats manual chunks like entry points: If both an entry and a manual chunk import a module, then it will always end up in a third chunk. What is different from entry points is that static dependencies that only the manual chunk has will not be merged into the manual chunk automatically. This is currently only relevant for explicit manual chunks, though, where this behavior is not desired anyway. I am not 100% sure if we want to have this in the way I implemented it, but it is quite efficient and should resolve many circular dependency scenarios. |
|
Wow, that was really fast. Looks good to me — nice approach! |
3d4ad0e to
eb0c641
Compare
Initially, we are only collection single modules with their dependent entries.
This will effectively avoid cycles where manual chunks import back from entry chunks. The price, though, is that it might create more chunks. For the longer term, it would be nice to replace the current logic where non-explicit manual chunks try to include as many static dependencies as possible with a logic where they behave as any entry points and just include all dependencies not shared with another entry.
…dencies Basically for the purpose of analysis, all explicit members of a manual chunk are treated as the same module, meaning dependencies are shared in the chunk. This mirrors what will happen in the output and reduces generated chunks.
eb0c641 to
594042c
Compare
594042c to
d5d16a9
Compare
|
I think I am happy with the implementation now. I added an improvement to actually treat all modules in the manual chunk as one module with regard to finding static dependencies. That avoids creating too many other chunks, see the added test where this change reduces the chunk count by one. For Rollup 5, I think I would actually extend this idea to rework how manual chunks are created. Instead of just collecting static dependencies, I would like to treat manual chunk definitions really just like entry points and assign the alias to the chunk that contains all the modules that defined the chunk. This will lead to slightly smaller manual chunks if the array form is used, but I think will much more closely follow expectations. |
I think this logic needs one more condition: it should only apply when all modules in the manual chunk have the same dependent entries. For example, in the test, if we change the imports of
Nice, looking forward to it!
Sounds good to me. |
Hi @lukastaegert, what do you think? Does that make sense? |
|
I thought about this but I am pretty sure that we do not need this condition. Even in my test, there is no guarantee that both entry points are loaded. So you have this graph with entry points E1 and E2 graph LR
E1 --> M1
M1 --> D1
E2 --> M2
M2 --> D2
subgraph manual
M1
M2
end
As I understand you, you would put D1 and D2 into separate chunks. However after bundling, you get graph LR
E1 --> manual
manual --> D1
E2 --> manual
manual --> D2
where manual has D1 and D2 as static dependencies. So whenever |
|
Oh, sorry about that. I should have been looking at |
|
This PR has been released as part of rollup@4.62.0. You can test it via |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
This PR changes the
onlyExplicitManualChunksbehavior for the function form ofoutput.manualChunks.Static dependencies imported by manual chunks are now extracted into separate chunks instead of remaining in entry chunk atoms. This avoids an unnecessary circular chunk case.