Skip to content

feat: add ineffective dynamic import warning#7600

Closed
luke358 wants to merge 4 commits intorolldown:mainfrom
luke358:feat/ineffective-dynamic-import-warning
Closed

feat: add ineffective dynamic import warning#7600
luke358 wants to merge 4 commits intorolldown:mainfrom
luke358:feat/ineffective-dynamic-import-warning

Conversation

@luke358
Copy link
Contributor

@luke358 luke358 commented Dec 19, 2025 •
edited
Loading

This PR implements the changes described in #7541

  • Implement warning for dynamic imports that won't create separate chunks when a module is both statically and dynamically imported.
  • Detect modules with both static and dynamic importers in same chunk and filter out HMR hot.accept to avoid false positives
  • Add some cases covering various scenarios

IWANABETHATGUY
IWANABETHATGUY reviewed Dec 20, 2025
View reviewed changes
crates/rolldown/tests/rolldown/warnings/ineffective_dynamic_import/basic/main.js Outdated
Copy link
Member

@IWANABETHATGUY IWANABETHATGUY Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case is redundant, as there already exists a similar test case in our codebase.

IWANABETHATGUY
IWANABETHATGUY reviewed Dec 20, 2025
View reviewed changes
crates/rolldown/tests/rolldown/warnings/ineffective_dynamic_import/common_chunk/entry1.js Outdated
Copy link
Member

@IWANABETHATGUY IWANABETHATGUY Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there also exists a similar testcase for this one as well.

@IWANABETHATGUY
Copy link
Member

IWANABETHATGUY commented Dec 20, 2025

Could you review your test cases again and remove those that are redundant or already covered by existing tests in our codebase?

sapphi-red
sapphi-red reviewed Dec 20, 2025
View reviewed changes
crates/rolldown/src/stages/generate_stage/code_splitting.rs Outdated
return;
}

for chunk in chunk_graph.chunk_table.iter() {
Copy link
Member

@sapphi-red sapphi-red Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this has a perf impact because we are adding a new loop here.

Copy link
Contributor Author

@luke358 luke358 Dec 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that there are duplicated loops at

These loops look very similar and might be merged. Since the chunks have already been processed at this point, it seems reasonable to collect warnings here.

@luke358
Copy link
Contributor Author

luke358 commented Dec 20, 2025

Could you review your test cases again and remove those that are redundant or already covered by existing tests in our codebase?

Thanks for the feedback!
I’m reviewing the test cases now and will remove the redundant ones shortly.

@sapphi-red sapphi-red mentioned this pull request Feb 12, 2026
Merged
graphite-app bot pushed a commit that referenced this pull request Feb 12, 2026
@shulaoda @luke358 @AliceLanniste
feat: add INEFFECTIVE_DYNAMIC_IMPORT warning in core (#8284)
80fa52b 
closes #7541, #7600, #7971

---

Thanks to @luke358 for the effort in PR #7600.
Thanks to @AliceLanniste for the effort in PR #7971.

Co-authored-by: luke358 <qq1494135711@gmail.com>
Co-authored-by: AliceLanniste <1399789151@qq.com>
@shulaoda
Copy link
Member

shulaoda commented Feb 12, 2026

Closed by #8284

@shulaoda shulaoda closed this Feb 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@IWANABETHATGUY IWANABETHATGUY IWANABETHATGUY left review comments

@sapphi-red sapphi-red sapphi-red left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@luke358 @IWANABETHATGUY @shulaoda @sapphi-red