Skip to content

refactor(rolldown): skip CJS namespace merging under strict execution order#10203

Merged
graphite-app[bot] merged 1 commit into
mainfrom
strict-link-groundwork
Jul 11, 2026
Merged

refactor(rolldown): skip CJS namespace merging under strict execution order#10203
graphite-app[bot] merged 1 commit into
mainfrom
strict-link-groundwork

Conversation

@hyfdev

@hyfdev hyfdev commented Jul 9, 2026

Copy link
Copy Markdown
Member

What this does (and doesn't). Under output.strictExecutionOrder, skip the "safely merge CJS namespace" optimization: determine_safely_merge_cjs_ns now early-returns when strict execution order is enabled. This is the one piece of strict-gated link-stage groundwork that stands on its own. It does NOT change flag-off behavior, and it deliberately excludes the feature's other strict bookkeeping (star_export_record_by_name, execution_dependencies, star-reexport path recording) because none of it has a reader in this PR — that lands with the analysis that consumes it.

Why. The optimization lets several ESM importers of the same CommonJS module share one namespace binding, which relocates the surviving require() to whichever importer statement stays included. That reordering can move a CommonJS module's evaluation past statements that should run after it — an execution-order break that no amount of wrapping can repair. Under strict execution order, correctness must win over the small byte saving, so we keep each importer's own call site (the CJS wrapper memoizes, so the module still evaluates exactly once). The added cjs_ns_merge_skipped fixture shows the contrast directly: the default variant shares one import_cjs, while the strict variant gives lib.js and main.js their own bindings materialized at their own use sites.

Validation. cargo test -p rolldown --test integration: passing (same 5 pre-existing environment-only failures as the rest of the stack). cargo clippy -p rolldown --all-targets -- -D warnings is clean and cargo fmt --all produces no diff. Flag-off output is byte-identical — no non-strict snapshot changed. Exactly one existing strict-only snapshot changed, react-like: the shared import_commonjs = __toESM(require_commonjs()) is now split so lib.js does a bare require_commonjs() and main.js materializes the namespace at its own use site. That is the intended strict-only correctness improvement.

Stack. Top of a three-layer stack slicing the output.strictExecutionOrder work; builds on (1) wrap-kind / chunk-graph cleanup and (2) the interop init-target view. The strict analysis and code generation that consume the deferred bookkeeping come in later layers.

hyfdev commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

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.

@hyfdev
hyfdev marked this pull request as ready for review July 9, 2026 12:02
Copilot AI review requested due to automatic review settings July 9, 2026 12:02

hyfdev commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

Merge activity

  • Jul 9, 12:02 PM UTC: The merge label 'graphite: merge-when-ready' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jul 10, 5:33 AM UTC: hyf0 added this pull request to the Graphite merge queue.
  • Jul 10, 6:10 AM UTC: This pull request was removed from the Graphite merge queue because the batch it was included in timed out. Please re-enqueue the PR to retry merge.
  • Jul 11, 8:24 AM UTC: The merge label 'graphite: merge-when-ready' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jul 11, 8:24 AM UTC: hyf0 added this pull request to the Graphite merge queue.
  • Jul 11, 8:25 AM UTC: Merged by the Graphite merge queue.

@hyfdev hyfdev changed the title fix(rolldown): skip CJS namespace merging under strict execution order chore(rolldown): skip CJS namespace merging under strict execution order Jul 9, 2026
@hyfdev hyfdev changed the title chore(rolldown): skip CJS namespace merging under strict execution order refactor(rolldown): skip CJS namespace merging under strict execution order Jul 9, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Disables the “safely merge CJS namespace” size optimization when output.strictExecutionOrder is enabled, ensuring CommonJS require() evaluation stays anchored to each importer’s original call site to preserve observable execution order in strict mode.

Changes:

  • Early-return from determine_safely_merge_cjs_ns under strict execution order, preventing cross-importer namespace merging.
  • Update the existing strict-only react-like snapshot to reflect the non-merged CJS namespace behavior.
  • Add a new strict fixture (cjs_ns_merge_skipped) that demonstrates the non-strict merged output vs strict non-merged output in one snapshot.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
crates/rolldown/src/stages/link_stage/determine_module_exports_kind.rs Skips building safely_merge_cjs_ns_map under strict execution order to prevent require() relocation.
crates/rolldown/tests/rolldown/function/experimental/strict_execution_order/react-like/artifacts.snap Updates strict snapshot to show the split require/__toESM behavior.
crates/rolldown/tests/rolldown/function/experimental/strict_execution_order/cjs_ns_merge_skipped/_config.json Adds a variant to snapshot both default and strict outputs for the new fixture.
crates/rolldown/tests/rolldown/function/experimental/strict_execution_order/cjs_ns_merge_skipped/main.js New test entry that imports the same CJS module as another ESM module.
crates/rolldown/tests/rolldown/function/experimental/strict_execution_order/cjs_ns_merge_skipped/lib.js New test module that also imports the same CJS module, enabling the merge-vs-skip contrast.
crates/rolldown/tests/rolldown/function/experimental/strict_execution_order/cjs_ns_merge_skipped/cjs.cjs New CJS module with an observable side effect (console.log) to validate evaluation ordering.
crates/rolldown/tests/rolldown/function/experimental/strict_execution_order/cjs_ns_merge_skipped/artifacts.snap New snapshot demonstrating merged namespace in default mode and per-importer namespaces in strict mode.

@codspeed-hq

codspeed-hq Bot commented Jul 9, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 7 untouched benchmarks
⏩ 10 skipped benchmarks1


Comparing strict-link-groundwork (025ebaf) with main (b982305)2

Open in CodSpeed

Footnotes

  1. 10 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on main (025ebaf) during the generation of this report, so b982305 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@hyfdev
hyfdev changed the base branch from strict-prep-init-target-view to graphite-base/10203 July 9, 2026 12:44
graphite-app Bot pushed a commit that referenced this pull request Jul 10, 2026
… order (#10203)

**What this does (and doesn't).** Under `output.strictExecutionOrder`, skip the "safely merge CJS namespace" optimization: `determine_safely_merge_cjs_ns` now early-returns when strict execution order is enabled. This is the one piece of strict-gated link-stage groundwork that stands on its own. It does NOT change flag-off behavior, and it deliberately excludes the feature's other strict bookkeeping (`star_export_record_by_name`, `execution_dependencies`, star-reexport path recording) because none of it has a reader in this PR — that lands with the analysis that consumes it.

**Why.** The optimization lets several ESM importers of the same CommonJS module share one namespace binding, which relocates the surviving `require()` to whichever importer statement stays included. That reordering can move a CommonJS module's evaluation past statements that should run after it — an execution-order break that no amount of wrapping can repair. Under strict execution order, correctness must win over the small byte saving, so we keep each importer's own call site (the CJS wrapper memoizes, so the module still evaluates exactly once). The added `cjs_ns_merge_skipped` fixture shows the contrast directly: the default variant shares one `import_cjs`, while the strict variant gives `lib.js` and `main.js` their own bindings materialized at their own use sites.

**Validation.** `cargo test -p rolldown --test integration`: passing (same 5 pre-existing environment-only failures as the rest of the stack). `cargo clippy -p rolldown --all-targets -- -D warnings` is clean and `cargo fmt --all` produces no diff. Flag-off output is byte-identical — no non-strict snapshot changed. Exactly one existing strict-only snapshot changed, `react-like`: the shared `import_commonjs = __toESM(require_commonjs())` is now split so `lib.js` does a bare `require_commonjs()` and `main.js` materializes the namespace at its own use site. That is the intended strict-only correctness improvement.

**Stack.** Top of a three-layer stack slicing the `output.strictExecutionOrder` work; builds on (1) wrap-kind / chunk-graph cleanup and (2) the interop init-target view. The strict analysis and code generation that consume the deferred bookkeeping come in later layers.
@graphite-app
graphite-app Bot force-pushed the strict-link-groundwork branch from 6a26fd9 to 7776b18 Compare July 10, 2026 05:35
@graphite-app
graphite-app Bot requested a review from sapphi-red as a code owner July 10, 2026 05:35
@graphite-app
graphite-app Bot changed the base branch from graphite-base/10203 to strict-prep-init-target-view July 10, 2026 05:35
Base automatically changed from strict-prep-init-target-view to main July 10, 2026 05:39
… order (#10203)

**What this does (and doesn't).** Under `output.strictExecutionOrder`, skip the "safely merge CJS namespace" optimization: `determine_safely_merge_cjs_ns` now early-returns when strict execution order is enabled. This is the one piece of strict-gated link-stage groundwork that stands on its own. It does NOT change flag-off behavior, and it deliberately excludes the feature's other strict bookkeeping (`star_export_record_by_name`, `execution_dependencies`, star-reexport path recording) because none of it has a reader in this PR — that lands with the analysis that consumes it.

**Why.** The optimization lets several ESM importers of the same CommonJS module share one namespace binding, which relocates the surviving `require()` to whichever importer statement stays included. That reordering can move a CommonJS module's evaluation past statements that should run after it — an execution-order break that no amount of wrapping can repair. Under strict execution order, correctness must win over the small byte saving, so we keep each importer's own call site (the CJS wrapper memoizes, so the module still evaluates exactly once). The added `cjs_ns_merge_skipped` fixture shows the contrast directly: the default variant shares one `import_cjs`, while the strict variant gives `lib.js` and `main.js` their own bindings materialized at their own use sites.

**Validation.** `cargo test -p rolldown --test integration`: passing (same 5 pre-existing environment-only failures as the rest of the stack). `cargo clippy -p rolldown --all-targets -- -D warnings` is clean and `cargo fmt --all` produces no diff. Flag-off output is byte-identical — no non-strict snapshot changed. Exactly one existing strict-only snapshot changed, `react-like`: the shared `import_commonjs = __toESM(require_commonjs())` is now split so `lib.js` does a bare `require_commonjs()` and `main.js` materializes the namespace at its own use site. That is the intended strict-only correctness improvement.

**Stack.** Top of a three-layer stack slicing the `output.strictExecutionOrder` work; builds on (1) wrap-kind / chunk-graph cleanup and (2) the interop init-target view. The strict analysis and code generation that consume the deferred bookkeeping come in later layers.
@hyfdev
hyfdev force-pushed the strict-link-groundwork branch from 7776b18 to 025ebaf Compare July 11, 2026 08:18
@netlify

netlify Bot commented Jul 11, 2026

Copy link
Copy Markdown

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 025ebaf
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/6a51fc5247007f0008e47c29

@graphite-app
graphite-app Bot merged commit 025ebaf into main Jul 11, 2026
39 checks passed
@graphite-app
graphite-app Bot deleted the strict-link-groundwork branch July 11, 2026 08:25
@rolldown-guard rolldown-guard Bot mentioned this pull request Jul 15, 2026
shulaoda added a commit that referenced this pull request Jul 15, 2026
## [1.2.0] - 2026-07-15

### 🚀 Features

- dev: skip shipping factories for newly imported top-level modules (#10223) by @h-a-n-a
- dev: per-client ship map for HMR patch sizing (#10208) by @h-a-n-a
- dev: client-side HMR (#10164) by @h-a-n-a
- dev: send a full-reload update to clients when a tsconfig changes (#10262) by @shulaoda
- treat `import.meta['url']` and `import.meta['ROLLUP_FILE_URL_*']` as side-effect free (#10267) by @sapphi-red
- rewrite `import.meta['url']` (#10251) by @sapphi-red
- add `FILE_NOT_FOUND` error (#10220) by @sapphi-red
- treat `import.meta.ROLLUP_FILE_URL_*` as side-effect free (#10217) by @sapphi-red

### 🐛 Bug Fixes

- sourcemap: preserve unmapped boundaries during composition (#10254) by @hyfdev
- `[format]` in `*FileNames` option for ESM format should be `es` instead of `esm` (#10214) by @sapphi-red
- sourcemap: preserve coarse mappings during composition (#10249) by @hyfdev
- rolldown_plugin_vite_import_glob: support tsconfig paths with `import.meta.glob` (#10167) by @sapphi-red
- dev: clear tsconfig caches for bare full builds (#10276) by @shulaoda
- dev: force a full rebuild when a tsconfig changes (#10261) by @shulaoda
- treat rooted drive-less module ids as absolute in preserveModules naming (#10235) by @IWANABETHATGUY
- watch: rebuild when tsconfig files change (#10258) by @shulaoda
- watch: drop tsconfig-merged transform options on each rebuild (#10257) by @shulaoda
- incorrect `EMPTY_IMPORT_META` warning for `import.meta.ROLLUP_FILE_URL_*` for CJS output (#10221) by @sapphi-red
- deconflict: rename CJS locals shadowing wrapped-ESM namespace objects (#9970) by @IWANABETHATGUY
- rolldown: drop the unused runtime module after entry-level external flattening (#10237) by @IWANABETHATGUY
- rolldown: re-propagate has_dynamic_exports to transitive star importers (#10239) by @IWANABETHATGUY
- tree-shaking: tree-shake destructured dynamic import namespace bindings (#10213) by @logaretm
- s390x: use json-escape-simd 3.1.1 for big-endian JSON escaping fix (#10211) by @satyamg1620

### 🚜 Refactor

- dev: move full-reload to client side (#10207) by @h-a-n-a
- readability follow-ups to the ReplaceWith migration (#10286) by @IWANABETHATGUY
- replace take_in-then-write-back with ReplaceWith and by-value moves (#10285) by @Boshen
- share the main resolver's cache with the transformer's tsconfig lookups (#10205) by @shulaoda
- rolldown: extract the ns star-external __reExport emission rule into LinkingMetadata (#10238) by @IWANABETHATGUY
- rolldown: unify link/generate diagnostics into a Diagnostics accumulator (#10234) by @IWANABETHATGUY
- sourcemap_filenames: drop dead sourcemap-filename plumbing (#10189) by @IWANABETHATGUY
- extract external import symbol merging into a method (#10224) by @IWANABETHATGUY
- rolldown: skip CJS namespace merging under strict execution order (#10203) by @hyfdev
- resolve the manual tsconfig per file instead of once at startup (#10200) by @shulaoda
- rolldown: route interop ESM init emission through a shared init-target view (#10202) by @hyfdev
- rolldown: collapse vestigial wrap-kind state and share chunk sort helper (#10201) by @hyfdev

### 📚 Documentation

- show plugin kinds in JSDoc and each hook's description (#10218) by @sapphi-red
- add an explanation about removing imports from external modules without any messages (#10215) by @sapphi-red

### ⚡ Performance

- sourcemap: owned merge in SourceJoiner::join (4005->5 allocs/chunk) (#10250) by @Boshen
- avoid redundant sourcemap string copies in collapse and minify paths (#10093) by @Boshen

### 🧪 Testing

- code-splitting: establish strict-order review baselines (#10287) by @hyfdev
- dev: add hot API test cases (#10181) by @h-a-n-a
- code-splitting: normalize strict execution order variants (#10277) by @hyfdev
- code-splitting: harden strict execution order coverage (#10252) by @hyfdev
- code-splitting: add strict execution order regressions (#10253) by @hyfdev

### ⚙️ Miscellaneous Tasks

- deps: update github actions (#10241) by @renovate[bot]
- deps: update oxc to 0.140.0 (#10274) by @shulaoda
- update Yunfei's GitHub username (#10275) by @hyfdev
- deps: update napi (#10260) by @renovate[bot]
- deps: update test262 submodule for tests (#10266) by @rolldown-guard[bot]
- deps: update dependency vite-plus to v0.2.4 (#10256) by @renovate[bot]
- deps: update napi (#10240) by @renovate[bot]
- deps: update oxc resolver to v11.24.2 (#10245) by @renovate[bot]
- deps: update rust crates (#10244) by @renovate[bot]
- disable Renovate updates for idna_adapter (#10248) by @shulaoda
- deps: update oxc resolver to v11.24.1 (#10232) by @renovate[bot]
- deps: update rust crate oxc_sourcemap to v8.1.1 (#10233) by @renovate[bot]
- deps: update dependency rolldown-plugin-dts to ^0.27.0 (#10206) by @renovate[bot]
- deps: upgrade sugar_path to v3 (#10230) by @hyfdev
- add `dist-*` to `.gitignore` in sourcemap-filenames/hash-final-content fixture (#10216) by @sapphi-red
- deps: update dependency rust to v1.97.0 (#10209) by @renovate[bot]

### ❤️ New Contributors

* @satyamg1620 made their first contribution in [#10211](#10211)

Co-authored-by: shulaoda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants