Skip to content

Comments

feat: inline dynamic imports that imports statically imported modules#7742

Merged
graphite-app[bot] merged 1 commit intomainfrom
12-22-feat_4905
Jan 5, 2026
Merged

feat: inline dynamic imports that imports statically imported modules#7742
graphite-app[bot] merged 1 commit intomainfrom
12-22-feat_4905

Conversation

@IWANABETHATGUY
Copy link
Member

@IWANABETHATGUY IWANABETHATGUY commented Jan 4, 2026 •
edited
Loading

closed #4905

@IWANABETHATGUY Graphite App
Copy link
Member Author

IWANABETHATGUY commented Jan 4, 2026 •
edited
Loading

How to use the Graphite Merge Queue

Add the label graphite: merge-when-ready to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@netlify
Copy link

netlify bot commented Jan 4, 2026 •
edited
Loading

✅ Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 8257a30
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/695bacf3a28c3800080e5907

@IWANABETHATGUY IWANABETHATGUY force-pushed the 12-22-feat_4905 branch 2 times, most recently from 8d2c4c0 to cf2e00b Compare January 4, 2026 13:01
@IWANABETHATGUY IWANABETHATGUY changed the base branch from main to graphite-base/7742 January 4, 2026 13:53
@IWANABETHATGUY IWANABETHATGUY changed the base branch from graphite-base/7742 to 01-04-test_use_assertion_instead_of_console.log January 4, 2026 13:53
@IWANABETHATGUY IWANABETHATGUY mentioned this pull request Jan 4, 2026
Merged
@IWANABETHATGUY IWANABETHATGUY force-pushed the 01-04-test_use_assertion_instead_of_console.log branch from 3418c7c to b3a0351 Compare January 4, 2026 13:55
@graphite-app graphite-app bot changed the base branch from 01-04-test_use_assertion_instead_of_console.log to graphite-base/7742 January 4, 2026 14:19
@graphite-app graphite-app bot force-pushed the graphite-base/7742 branch from b3a0351 to 832cba1 Compare January 4, 2026 14:31
@graphite-app graphite-app bot changed the base branch from graphite-base/7742 to main January 4, 2026 14:31
@IWANABETHATGUY IWANABETHATGUY force-pushed the 12-22-feat_4905 branch 4 times, most recently from 435d62d to 8fd50ff Compare January 5, 2026 08:31
@IWANABETHATGUY IWANABETHATGUY changed the title feat: 4905 refactor: optimize dynamic imports for modules in same chunk Jan 5, 2026
@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review January 5, 2026 08:58
Copilot AI review requested due to automatic review settings January 5, 2026 08:58
@IWANABETHATGUY IWANABETHATGUY changed the title refactor: optimize dynamic imports for modules in same chunk feat: inline dynamic imports that imports statically imported modules Jan 5, 2026
Copilot AI reviewed Jan 5, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR optimizes dynamic imports for modules that end up in the same chunk after code splitting and chunk merging. When a dynamically imported module is merged into the same chunk as the importer (either through chunk merging optimization or user-defined entry points), the PR eliminates the unnecessary dynamic import by converting it to a synchronous reference wrapped in a resolved Promise.

Key changes:

  • Converts import('./module.js') to Promise.resolve().then(() => module_namespace) when both modules are in the same chunk
  • Handles both ESM and CJS modules correctly, applying __toESM wrapper for CJS modules
  • Extends chunk merging optimization to support merging dynamic entries into user-defined entry chunks
  • Adds namespace extraction for cross-chunk imports when dynamic entries are merged into common chunks

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/rolldown_common/src/chunk/mod.rs Adds is_pure_user_defined_entry() helper method to check if a chunk is purely a user-defined entry without other flags
crates/rolldown/src/stages/generate_stage/chunk_optimizer.rs Extends chunk merging logic to handle dynamic entries merged into user-defined entry chunks
crates/rolldown/src/module_finalizers/mod.rs Implements core rewriting logic for dynamic imports when modules are in the same chunk, with proper handling of ESM and CJS modules
crates/rolldown/tests/rolldown/optimization/chunk_merging/issue_4905/* New test case demonstrating ESM dynamic import optimization with both ESM and CJS output formats
crates/rolldown/tests/rolldown/optimization/chunk_merging/dynamic_import_cjs_same_chunk/* New test case demonstrating CJS module dynamic import optimization
crates/rolldown/tests/snapshots/* Updated snapshots reflecting optimized output with fewer chunks and rewritten dynamic imports
crates/rolldown/tests/*/artifacts.snap Updated test snapshots showing the optimization in action across various existing tests

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@IWANABETHATGUY IWANABETHATGUY mentioned this pull request Jan 5, 2026
Merged
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2026 •
edited
Loading

Benchmarks Rust

group                                                        pr                                     target
-----                                                        --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol              1.00     65.0±1.94ms        ? ?/sec    1.03     66.8±2.13ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap    1.00     70.8±1.92ms        ? ?/sec    1.07     75.6±3.40ms        ? ?/sec
bundle/bundle@rome_ts                                        1.00    107.7±3.24ms        ? ?/sec    1.02    110.2±2.62ms        ? ?/sec
bundle/bundle@rome_ts-sourcemap                              1.00    120.1±2.42ms        ? ?/sec    1.01    121.1±2.12ms        ? ?/sec
bundle/bundle@threejs                                        1.00     39.9±2.17ms        ? ?/sec    1.00     40.0±2.39ms        ? ?/sec
bundle/bundle@threejs-sourcemap                              1.01     44.2±0.76ms        ? ?/sec    1.00     43.8±1.13ms        ? ?/sec
bundle/bundle@threejs10x                                     1.00    396.3±7.30ms        ? ?/sec    1.01    400.5±5.16ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                           1.00    460.3±6.16ms        ? ?/sec    1.01    462.8±6.88ms        ? ?/sec
scan/scan@rome_ts                                            1.00     85.4±1.70ms        ? ?/sec    1.00     85.3±2.64ms        ? ?/sec
scan/scan@threejs                                            1.00     28.9±0.44ms        ? ?/sec    1.00     29.0±1.70ms        ? ?/sec
scan/scan@threejs10x                                         1.00    297.9±4.23ms        ? ?/sec    1.00    298.4±4.76ms        ? ?/sec

@IWANABETHATGUY IWANABETHATGUY mentioned this pull request Jan 5, 2026
Merged
@IWANABETHATGUY
Copy link
Member Author

IWANABETHATGUY commented Jan 5, 2026

@hyf0 @sapphi-red I am going to merge this pr, feel free to comment if you found any issues.

@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 5, 2026 •
edited
Loading

Merge activity

@graphite-app graphite-app bot merged commit 8257a30 into main Jan 5, 2026
34 checks passed
@graphite-app graphite-app bot deleted the 12-22-feat_4905 branch January 5, 2026 12:41
This was referenced Jan 7, 2026
Closed
Merged
shulaoda added a commit that referenced this pull request Jan 7, 2026
@github-actions @shulaoda
release: v1.0.0-beta.59 (#7786)
25ffe2a 
## [1.0.0-beta.59] - 2026-01-07

⚡ Inline Dynamic Imports for Statically Imported Modules

- When a module is already statically imported, dynamic imports to that same module are now inlined instead of creating a separate chunk

### 🚀 Features

- plugin_timings: add 3s threshold and doc link to warning message (#7741) by @shulaoda
- improve treeshaking logic to handle empty parameter list in dynamic import .then() callbacks (#7781) by @Copilot
- dev/lazy: don't include already executed modules (#7745) by @hyf0
- dev/lazy: support dynamic `import(..)` (#7726) by @hyf0
- inline dynamic imports that imports statically imported modules (#7742) by @IWANABETHATGUY
- option: add experimental option to control chunk optimization (#7738) by @IWANABETHATGUY

### 🐛 Bug Fixes

- inline dynamic entry to user defined entry with esm wrap kind (#7783) by @IWANABETHATGUY
- use canonical namespace reference for property access (#7777) by @IWANABETHATGUY
- dynamic entry merged into common chunk with cjs and esm wrap kind (#7771) by @IWANABETHATGUY
- tla: should not await non-tla-related modules (#7768) by @hyf0
- dynamic entry captured by common chunk with CJS format (#7757) by @IWANABETHATGUY
- module_loader: mark emitted chunks as user-defined entry when already loaded (#7765) by @shulaoda
- normalize preserveModulesRoot path (#7737) by @IWANABETHATGUY
- linker: resolve race condition in side effects computation for export-star (#7728) by @camc314

### 🚜 Refactor

- plugin_timings: filter out plugins with duration < 1s from timing warnings (#7785) by @shulaoda
- module_loader: remove unnecessary collect before extend (#7769) by @shulaoda
- rename _id suffixes to _idx for oxc_index types (#7767) by @IWANABETHATGUY
- remove duplicate `preserve_entry_signatures` from `AddEntryModuleMsg` (#7762) by @shulaoda
- module_loader: pass `user_defined_entries` by reference (#7756) by @shulaoda
- dev/lazy: get proxy entry's `ResolvedId` correctly (#7746) by @hyf0
- simplify try_rewrite_import_expression control flow (#7753) by @IWANABETHATGUY
- module_loader: remove unnecessary dynamic import handling for runtime module (#7754) by @shulaoda
- inline __toDynamicImportESM  (#7747) by @IWANABETHATGUY
- use From impl for ModuleLoaderOutput conversion (#7732) by @shulaoda
- remove duplicate fields from `ModuleLoader` (#7731) by @shulaoda
- tweak `resolve_user_defined_entries` (#7727) by @shulaoda

### 📚 Documentation

- add rolldown-string reference to native MagicString compatibility section (#7778) by @Copilot
- improve comments for export star side effects handling (#7730) by @IWANABETHATGUY

### 🧪 Testing

- use assertion instead of console.log for some testcase (#7744) by @IWANABETHATGUY

### ⚙️ Miscellaneous Tasks

- tweak some `output.dynamicImportInCjs` related rollup test results (#7776) by @sapphi-red
- mark esbuild/dce/dce_of_symbol_ctor_call as passed (#7775) by @sapphi-red
- deps: update oxc apps (#7772) by @renovate[bot]
- vite-tests: allow running on PRs with `test: vite-tests` label (#7770) by @shulaoda
- deps: update oxc apps (#7760) by @renovate[bot]
- deps: update rollup submodule for tests to v4.55.1 (#7763) by @sapphi-red
- deps: update test262 submodule for tests (#7764) by @sapphi-red
- deps: update oxc to v0.107.0 (#7758) by @camc314
- deps: update taiki-e/install-action action to v2.65.13 (#7751) by @renovate[bot]
- deps: update rust crates (#7750) by @renovate[bot]
- deps: update npm packages (#7749) by @renovate[bot]
- deps: update github-actions (#7748) by @renovate[bot]
- deps: update dependency oxlint-tsgolint to v0.10.1 (#7729) by @renovate[bot]
- deps: update crate-ci/typos action to v1.41.0 (#7725) by @renovate[bot]

Co-authored-by: shulaoda <[email protected]>
@ArnaudBarre ArnaudBarre mentioned this pull request Jan 10, 2026
Closed
@lazka
Copy link

lazka commented Jan 22, 2026 •
edited
Loading

Is there a way to disable this optimization? it produces broken code for me I think, and I'd like to confirm that.

After bundling I get:

// dbp-playground-activity.js:
var file_sink_exports = /* @__PURE__ */ __exportAll({ FileSink: () => FileSink });

And on the importing end I get this, so the import isn't adjusted somehow (the filename is, the rest isn't):

import("../dbp-playground-activity.js").then(({ FileSink }) => {
	this.defineScopedElement("dbp-file-sink", FileSink);
});

One reason I have this mix of static and dynamic imports is to avoid a cyclic dependency during import, so maybe this optimization isn't handling import cycles correctly?

I'll try to reduce the problem.

@lazka lazka mentioned this pull request Jan 23, 2026
Closed
@lazka
Copy link

lazka commented Jan 23, 2026 •
edited
Loading

I've found a reproducer: #8027 The trigger seems to be multiple entry points, and not import cycles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@shulaoda shulaoda shulaoda approved these changes

@hyf0 hyf0 Awaiting requested review from hyf0

@sapphi-red sapphi-red Awaiting requested review from sapphi-red

Assignees

@IWANABETHATGUY IWANABETHATGUY

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Feature Request]: inline dynamic imports that imports statically imported modules

3 participants

@IWANABETHATGUY @lazka @shulaoda