revert: `feat(on_demand_wrapping): don't wrap modules that don't rely on others and have side effect #4670` by hyf0 · Pull Request #4686 · rolldown/rolldown

hyf0 · 2025-05-26T10:57:17Z

Description

No side effect is not the full condition to safely remove the wrapper. A module might not have side effects, but it might rely on the order that side-effect module get executed first. See the case at #4684.

netlify · 2025-05-26T10:57:33Z

✅ Deploy Preview for rolldown-rs ready!

Name	Link
🔨 Latest commit	`221794c`
🔍 Latest deploy log	https://app.netlify.com/projects/rolldown-rs/deploys/68344f7edfd28000080d63c8
😎 Deploy Preview	https://deploy-preview-4686--rolldown-rs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

hyf0 · 2025-05-26T10:57:34Z

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

Copilot

Pull Request Overview

Revert the optimization that skipped wrapping modules without imports and only side effects, ensuring all modules are correctly wrapped again.

Restore wrapping behavior for modules that have side effects even if they lack imports.
Update expected snapshots across tests to reflect the restored __esm initialization calls.

Reviewed Changes

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

Show a summary per file

File	Description
crates/rolldown/src/stages/link_stage/wrapping.rs	Removed early exit that skipped wrapping for modules without imports and side effects.
crates/rolldown/tests/snapshots/integration_rolldown__filename_with_hash.snap	Updated hashed filenames in snapshots due to restored wrapping.
crates/rolldown/tests/rolldown/topics/bundler_esm_cjs_tests/25/artifacts.snap	Added `init_bar`/`init_foo` calls in CJS artifacts.
crates/rolldown/tests/rolldown/topics/bundler_esm_cjs_tests/24/artifacts.snap	Added `init_bar`/`init_foo` calls in ESM artifacts.
crates/rolldown/tests/rolldown/issues/3437/artifacts.snap	Imported `__esm` where needed for wrapping side-effect modules.
crates/rolldown/tests/rolldown/function/experimental/strict_execution_order/top_level_await_syntax_minify/artifacts.snap	Adjusted helper invocation order for strict execution.
crates/rolldown/tests/rolldown/function/experimental/strict_execution_order/remove_unused_no_side_effect_module/artifacts.snap	Added initialization call for no-side-effect module.
crates/rolldown/tests/rolldown/function/advanced_chunks/issue_2617/artifacts.snap	Inserted `init_shaked` call before `lib.js` execution.
crates/rolldown/tests/rolldown/function/advanced_chunks/basic/artifacts.snap	Exported `init_a`/`init_b` and invoked them in `a.js`/`b.js`.
crates/rolldown/tests/esbuild/default/top_level_await_forbidden_require/artifacts.snap	Added `init_something` invocation before top-level await.
crates/rolldown/tests/esbuild/default/export_forms_common_js/artifacts.snap	Initialized `abc` and `xyz` via `__esm` calls and invoked them.
crates/rolldown/tests/esbuild/dce/tree_shaking_in_esm_wrapper/artifacts.snap	Ensured `init_lib()` is called before tree-shaken functions.

Files not reviewed (1)

crates/rolldown/tests/rolldown/function/advanced_chunks/split_node_modules/artifacts.snap: Language not supported

...lldown/tests/rolldown/function/experimental/strict_execution_order/issue_4684/artifacts.snap

… on others and have side effect #4670`

github-actions · 2025-05-26T11:40:10Z

Benchmarks Rust

target: main(db25788)
pr: 05-26-revert_feat_on_demand_wrapping_don_t_wrap_modules_that_don_t_rely_on_others_and_have_side_effect_4670_(221794c)

group                                                               pr                                     target
-----                                                               --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol                     1.05     79.1±1.34ms        ? ?/sec    1.00     75.7±1.01ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify              1.03     98.3±1.65ms        ? ?/sec    1.00     95.6±4.66ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-minify-sourcemap    1.03    111.7±0.62ms        ? ?/sec    1.00    108.0±1.07ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap           1.03     88.7±1.05ms        ? ?/sec    1.00     86.1±1.79ms        ? ?/sec
bundle/bundle@rome-ts                                               1.04    120.7±1.39ms        ? ?/sec    1.00    115.9±1.66ms        ? ?/sec
bundle/bundle@rome-ts-minify                                        1.05    193.6±2.08ms        ? ?/sec    1.00    184.8±1.26ms        ? ?/sec
bundle/bundle@rome-ts-minify-sourcemap                              1.02    234.5±2.29ms        ? ?/sec    1.00    229.1±1.35ms        ? ?/sec
bundle/bundle@rome-ts-sourcemap                                     1.03    130.5±1.46ms        ? ?/sec    1.00    127.3±0.97ms        ? ?/sec
bundle/bundle@threejs                                               1.03     41.1±1.28ms        ? ?/sec    1.00     39.9±2.02ms        ? ?/sec
bundle/bundle@threejs-minify                                        1.02     82.1±1.01ms        ? ?/sec    1.00     80.5±0.40ms        ? ?/sec
bundle/bundle@threejs-minify-sourcemap                              1.02     98.0±1.25ms        ? ?/sec    1.00     95.7±0.49ms        ? ?/sec
bundle/bundle@threejs-sourcemap                                     1.03     47.5±0.85ms        ? ?/sec    1.00     46.2±0.44ms        ? ?/sec
bundle/bundle@threejs10x                                            1.01    420.3±3.73ms        ? ?/sec    1.00    418.1±3.31ms        ? ?/sec
bundle/bundle@threejs10x-minify                                     1.01   1006.9±9.04ms        ? ?/sec    1.00    997.5±3.58ms        ? ?/sec
bundle/bundle@threejs10x-minify-sourcemap                           1.01   1191.8±9.96ms        ? ?/sec    1.00   1176.9±3.38ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                                  1.00    485.1±2.62ms        ? ?/sec    1.00    486.7±9.27ms        ? ?/sec
remapping/remapping                                                 1.00     26.6±0.71ms        ? ?/sec    1.02     27.1±0.61ms        ? ?/sec
remapping/render-chunk-remapping                                    1.01     73.9±5.55ms        ? ?/sec    1.00     73.4±5.40ms        ? ?/sec
scan/scan@rome-ts                                                   1.05     95.1±1.15ms        ? ?/sec    1.00     90.5±1.43ms        ? ?/sec
scan/scan@threejs                                                   1.02     31.6±0.41ms        ? ?/sec    1.00     30.8±0.30ms        ? ?/sec
scan/scan@threejs10x                                                1.02    315.9±3.42ms        ? ?/sec    1.00    310.1±2.01ms        ? ?/sec

github-actions bot added the needs-triage label May 26, 2025

hyf0 marked this pull request as ready for review May 26, 2025 10:58

Copilot AI review requested due to automatic review settings May 26, 2025 10:58

Copilot AI reviewed May 26, 2025

View reviewed changes

hyf0 force-pushed the 05-26-revert_feat_on_demand_wrapping_don_t_wrap_modules_that_don_t_rely_on_others_and_have_side_effect_4670_ branch 2 times, most recently from 9f1f3e9 to 1626cb5 Compare May 26, 2025 11:14

hyf0 commented May 26, 2025

View reviewed changes

...lldown/tests/rolldown/function/experimental/strict_execution_order/issue_4684/artifacts.snap Show resolved Hide resolved

revert: `feat(on_demand_wrapping): don't wrap modules that don't rely…

221794c

… on others and have side effect #4670`

hyf0 force-pushed the 05-26-revert_feat_on_demand_wrapping_don_t_wrap_modules_that_don_t_rely_on_others_and_have_side_effect_4670_ branch from 1626cb5 to 221794c Compare May 26, 2025 11:24

hyf0 requested a review from shulaoda May 26, 2025 11:29

shulaoda approved these changes May 26, 2025

View reviewed changes

hyf0 added this pull request to the merge queue May 26, 2025

Merged via the queue into main with commit 8a77ce9 May 26, 2025
27 checks passed

hyf0 deleted the 05-26-revert_feat_on_demand_wrapping_don_t_wrap_modules_that_don_t_rely_on_others_and_have_side_effect_4670_ branch May 26, 2025 11:51

hyf0 mentioned this pull request Jul 10, 2025

perf: don't wrap module if it is leaf module and side effects free when enable strictExecutionOrder #5240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

revert: `feat(on_demand_wrapping): don't wrap modules that don't rely on others and have side effect #4670`#4686

revert: `feat(on_demand_wrapping): don't wrap modules that don't rely on others and have side effect #4670`#4686
hyf0 merged 1 commit intomainfrom
05-26-revert_feat_on_demand_wrapping_don_t_wrap_modules_that_don_t_rely_on_others_and_have_side_effect_4670_

hyf0 commented May 26, 2025 •

edited

Loading

Uh oh!

netlify bot commented May 26, 2025 •

edited

Loading

Uh oh!

hyf0 commented May 26, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

github-actions bot commented May 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

hyf0 commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

netlify bot commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for rolldown-rs ready!

Uh oh!

hyf0 commented May 26, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

github-actions bot commented May 26, 2025

Benchmarks Rust

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hyf0 commented May 26, 2025 •

edited

Loading

netlify bot commented May 26, 2025 •

edited

Loading