fix: fix chunk assignment for deoptimized module with dynamic import#6306
fix: fix chunk assignment for deoptimized module with dynamic import#6306lukastaegert merged 2 commits intorollup:masterfrom
Conversation
|
@JoaoBrlt is attempting to deploy a commit to the rollup-js Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Fixes a crash in Rollup’s chunk-assignment analysis when a dynamically importing module is present in a dynamic entry’s importer list but is absent from dependentEntriesByModule (e.g. when execution is triggered via the de-optimization path), addressing #6305.
Changes:
- Make
getDynamicallyDependentEntriesByDynamicEntry()tolerate missingdependentEntriesByModuleentries by skipping those importers. - Add a new
chunking-formregression sample covering the deoptimized + dynamic import scenario. - Add expected outputs for the new sample across
es,cjs,amd, andsystemformats.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/chunkAssignment.ts |
Avoids iterating undefined by skipping importers not present in dependentEntriesByModule. |
test/chunking-form/samples/deoptimized-module-with-dynamic-import/_config.js |
Adds a chunking-form test config that reproduces the deoptimization-path scenario. |
test/chunking-form/samples/deoptimized-module-with-dynamic-import/main.js |
Test entry for the new regression sample. |
test/chunking-form/samples/deoptimized-module-with-dynamic-import/a.js |
Sample module that sets up the conditions (including a dynamic import) relevant to the bug. |
test/chunking-form/samples/deoptimized-module-with-dynamic-import/lazy-loader.js |
Sample module used to trigger the problematic importer state. |
test/chunking-form/samples/deoptimized-module-with-dynamic-import/cjs.js |
Dynamically imported module used by the regression sample. |
test/chunking-form/samples/deoptimized-module-with-dynamic-import/_expected/es/main.js |
Expected ES output for the new regression sample. |
test/chunking-form/samples/deoptimized-module-with-dynamic-import/_expected/cjs/main.js |
Expected CJS output for the new regression sample. |
test/chunking-form/samples/deoptimized-module-with-dynamic-import/_expected/amd/main.js |
Expected AMD output for the new regression sample. |
test/chunking-form/samples/deoptimized-module-with-dynamic-import/_expected/system/main.js |
Expected System output for the new regression sample. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6306 +/- ##
=======================================
Coverage 98.77% 98.77%
=======================================
Files 273 273
Lines 10725 10728 +3
Branches 2859 2860 +1
=======================================
+ Hits 10594 10597 +3
Misses 89 89
Partials 42 42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lukastaegert
left a comment
lukastaegert
left a comment
There was a problem hiding this comment.
Thanks a lot! This is really a tricky bug, and I spent quite some time contemplating if there is a deeper problem, but I convinced myself that your fix is essentially correct and it should not be possible to trigger actual broken states here.
Great work on the reproduction and analysis! Will release this once the pipeline agrees.
|
Hey! 👋 Thanks a lot for reviewing and merging the PR so quickly. Really appreciate you taking the time! |
|
This PR has been released as part of [email protected]. You can test it via |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
Minimal reproducible example
https://github.com/JoaoBrlt/rollup-dynamic-import-bug
Problem
The
getDynamicallyDependentEntriesByDynamicEntry()function assumed that every module in theincludedDynamicImporterslist was also present in thedependentEntriesByModulemap. That assumption could be violated when a module was marked as executed via the de-optimization path (using themarkModuleAndImpureDependenciesAsExecuted()function) rather than the normal static-dependency traversal.Proposed solution
Skip any importer that is not present in
dependentEntriesByModuleinstead of asserting its presence with!.Test
Added
test/chunking-form/samples/deoptimized-module-with-dynamic-import/reproducing the issue. The test fails on the unfixed code withTypeError: dependentEntriesByModule.get is not a function or its return value is not iterableand passes after the fix. I designed the test to reproduce more or less the situation that I faced in my current project and in the minimal reproducible example.