Skip to content

Conversation

@IWANABETHATGUY
Copy link
Member

@IWANABETHATGUY IWANABETHATGUY commented Nov 14, 2025

Isuses

  • Before, when hmr is enabled, every module_namespace​ export object is generated but some of them are not included in used_symbol_refs​.
  • Before, some module namespace object is not generated but included in used_symbol_refs. (e.g. indirect external module reexport)export * from "@sentry/node"

This pr syncs module_namespace usage before code generation(both module level and chunk level), so that we could use used_symbol_ref to determine if a module namespace ref is used or not.

Closed #6992

Copy link
Member Author


How to use the Graphite Merge Queue

Add the label graphite: merge 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 Nov 14, 2025

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 549e1b7
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/6916caef8729dc0008d23aa2

@IWANABETHATGUY IWANABETHATGUY marked this pull request as ready for review November 14, 2025 05:12
Copilot AI review requested due to automatic review settings November 14, 2025 05:12
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 fixes issue #4992 related to module namespace reference handling in the bundling process. The fix ensures that unused module namespace references are properly eliminated before chunk symbol deconfliction, particularly for scenarios involving re-exported external modules with the preserveModules option.

Key changes:

  • Introduces a new finalized_module_namespace_ref_usage() method to clean up unused module namespace references before deconflicting chunk symbols
  • Adds filtering logic to skip unused symbols when building cross-chunk export relationships
  • Simplifies module namespace declaration logic by moving the reference check earlier in the pipeline

Reviewed Changes

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

Show a summary per file
File Description
crates/rolldown/src/stages/generate_stage/code_splitting.rs Adds new finalized_module_namespace_ref_usage() function to eliminate unused namespace references
crates/rolldown/src/stages/generate_stage/mod.rs Calls the new finalization method and removes parallel iteration in AST processing
crates/rolldown/src/stages/generate_stage/compute_cross_chunk_links.rs Adds filtering to skip unused symbol references in cross-chunk linking
crates/rolldown/src/module_finalizers/mod.rs Simplifies namespace declaration logic by using pre-computed reference flags
crates/rolldown/src/utils/chunk/render_chunk_exports.rs Contains debug statements for export rendering (should be removed)
crates/rolldown/src/ecmascript/format/esm.rs Contains debug statement for exports rendering (should be removed)
crates/rolldown/tests/rolldown/issues/4992/* New test case files for reproducing and validating the fix for issue #4992
crates/rolldown/tests/snapshots/integration_rolldown__filename_with_hash.snap Updated snapshot with new test case and hash changes from code modifications
crates/rolldown/tests/rolldown/topics/hmr/issue_4818/artifacts.snap Updated snapshot reflecting changes in generated code structure

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

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

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


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

@IWANABETHATGUY IWANABETHATGUY changed the title fix: 4992 fix: optimize module namespace object generation and eliminate unused refs Nov 14, 2025
@IWANABETHATGUY IWANABETHATGUY changed the title fix: optimize module namespace object generation and eliminate unused refs fix: remove unused module namespace object exporting Nov 14, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 14, 2025

Benchmarks Rust

group                                                        pr                                     target
-----                                                        --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol              1.00     61.0±2.02ms        ? ?/sec    1.00     61.1±1.07ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap    1.00     66.8±0.87ms        ? ?/sec    1.00     67.1±1.00ms        ? ?/sec
bundle/bundle@rome_ts                                        1.00    104.4±1.42ms        ? ?/sec    1.01    104.9±1.41ms        ? ?/sec
bundle/bundle@rome_ts-sourcemap                              1.00    117.4±1.53ms        ? ?/sec    1.00    117.3±2.60ms        ? ?/sec
bundle/bundle@threejs                                        1.00     37.7±0.98ms        ? ?/sec    1.02     38.5±2.27ms        ? ?/sec
bundle/bundle@threejs-sourcemap                              1.00     41.4±0.48ms        ? ?/sec    1.00     41.5±0.43ms        ? ?/sec
bundle/bundle@threejs10x                                     1.00    378.5±3.78ms        ? ?/sec    1.01    382.8±4.07ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                           1.00    437.8±3.96ms        ? ?/sec    1.01    441.1±6.41ms        ? ?/sec
scan/scan@rome_ts                                            1.01     84.1±1.33ms        ? ?/sec    1.00     83.6±1.42ms        ? ?/sec
scan/scan@threejs                                            1.00     28.6±1.80ms        ? ?/sec    1.00     28.5±1.89ms        ? ?/sec
scan/scan@threejs10x                                         1.00    292.7±4.29ms        ? ?/sec    1.00    291.3±3.88ms        ? ?/sec

@IWANABETHATGUY IWANABETHATGUY force-pushed the 11-14-fix_4992 branch 2 times, most recently from 24ae0c9 to 69bed1e Compare November 14, 2025 05:57
@hyf0
Copy link
Member

hyf0 commented Nov 14, 2025

Feel like we should should remove used_symbol_refs and use statementInfo to check if a symbol is included. I also encountered several bugs that a symbol showed in used_symbol_refs and the corresponding statementInfo isn't get included.

@hyf0
Copy link
Member

hyf0 commented Nov 14, 2025

Closed #6922

is not an issue? It it a correct link?

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 14, 2025

Merge activity

  • Nov 14, 6:23 AM UTC: IWANABETHATGUY added this pull request to the Graphite merge queue.
  • Nov 14, 6:29 AM UTC: The Graphite merge queue couldn't merge this PR because it was not satisfying all requirements (Failed CI: 'node-test (ubuntu-latest) / Node Test', 'node-test (macos-latest) / Node Test', 'node-test (windows-latest) / Node Test').

**Isuses**
- Before, when hmr is enabled, every `module_namespace`​ export object is generated but some of them are not included in `used_symbol_refs`​.
- Before, some module namespace object is not generated but included in `used_symbol_refs`. (e.g. indirect external module reexport)export * from "@sentry/node"


This pr syncs `module_namespace` usage before code generation(both module level and chunk level), so that we could use `used_symbol_ref` to determine if a module namespace ref is used or not.

Closed #6922
@IWANABETHATGUY
Copy link
Member Author

Closed #6922

is not an issue? It it a correct link?

Fixed

@IWANABETHATGUY IWANABETHATGUY merged commit f262cea into main Nov 14, 2025
43 of 46 checks passed
@IWANABETHATGUY IWANABETHATGUY deleted the 11-14-fix_4992 branch November 14, 2025 07:00
This was referenced Nov 19, 2025
shulaoda added a commit that referenced this pull request Nov 19, 2025
## [1.0.0-beta.51] - 2025-11-19

### 💥 BREAKING CHANGES

- rolldown_plugin_vite_react_refresh_wrapper: add vite prefix (#7086) by
@shulaoda
- rolldown_plugin_vite_web_worker_post: add vite prefix (#7085) by
@shulaoda
- rolldown_plugin_vite_wasm_helper: add vite prefix (#7084) by @shulaoda
- rolldown_plugin_vite_wasm_fallback: add vite prefix (#7083) by
@shulaoda
- rolldown_plugin_vite_transform: add vite prefix (#7082) by @shulaoda
- rolldown_plugin_vite_reporter: add vite prefix (#7081) by @shulaoda
- rolldown_plugin_vite_module_preload_polyfill: add vite prefix (#7080)
by @shulaoda
- rolldown_plugin_vite_manifest: add vite prefix (#7079) by @shulaoda
- rolldown_plugin_vite_load_fallback: add vite prefix (#7072) by
@shulaoda
- rolldown_plugin_vite_json: add vite prefix (#7071) by @shulaoda
- rolldown_plugin_vite_import_glob: add vite prefix (#7070) by @shulaoda
- rolldown_plugin_vite_html_inline_proxy: add vite prefix (#7069) by
@shulaoda
- rolldown_plugin_vite_dynamic_import_vars: add vite prefix (#7068) by
@shulaoda
- rolldown_plugin_vite_build_import_analysis: add vite prefix (#7067) by
@shulaoda
- rolldown_plugin_vite_asset_import_meta_url: add vite prefix (#7066) by
@shulaoda
- rolldown_plugin_vite_alias: add vite prefix (#7065) by @shulaoda
- rolldown_plugin_vite_asset_plugin: add vite prefix (#7064) by
@shulaoda

### 🚀 Features

- export sync APIs to experimental (#7122) by @shulaoda
- rolldown_plugin_vite_asset_import_meta_url: implement template literal
support for dynamic URLs (#7118) by @shulaoda
- rolldown_plugin_vite_asset_import_meta_url: implement AST-based URL
detection (#7113) by @shulaoda
- add isPathFragment validation for filename patterns (rollup compat)
(#7101) by @IWANABETHATGUY
- rolldown_plugin_vite_asset_import_meta_url: align filter logic (#7103)
by @shulaoda
- rolldown: oxc v0.98.0 (#6961) by @camc314
- show error contexts for unhandleable errors (#7095) by @sapphi-red
- rolldown_plugin_utils: extract `get_hash` utility function (#7059) by
@shulaoda
- rolldown_plugin_asset: initialize `CSSEntriesCache` (#7015) by
@shulaoda
- rolldown_plugin_vite_html: align `transformIndexHtml` logic (#7010) by
@shulaoda
- builtin-plugin: support `bindingifyViteHtmlPlugin` (#7008) by
@shulaoda
- impl `generatedCode.symbols` for reexport dynamic modules. (#6993) by
@IWANABETHATGUY
- rolldown_plugin_manifest: support v2 logic (#6979) by @shulaoda
- support Node.js `module.exports` ESM export (#6967) by @Copilot
- change "could not clean directory" from error to warning (#6955) by
@Copilot
- rolldown_binding: add context to errors thrown by plugin hooks (#6964)
by @sapphi-red

### 🐛 Bug Fixes

- content hash should be affected by the minify behavior (#7102) by
@hyf0
- `canonical name not found for "__toESM"` error when only named imports
are used from a CJS module (#7094) by @sapphi-red
- preserve directory structure in chunk names with preserveModules
(#6872) by @IWANABETHATGUY
- rolldown_plugin_asset: correct bundle deletion index calculation
(#7063) by @shulaoda
- rolldown_plugin_utils: correct string slicing in
`render_asset_url_in_js` (#7061) by @shulaoda
- rolldown_plugin_vite_html: use transformed result in asset URL
handling (#7060) by @shulaoda
- rolldown_plugin_vite_html: skip redundant path resolution for
processed URLs (#7058) by @shulaoda
- rolldown_plugin_vite_css_post: data race in CSS URL processing (#7055)
by @shulaoda
- rolldown_plugin_vite_css_post: always compute css asset dirname in
build command (#7054) by @shulaoda
- rolldown_plugin_vite_css: ensure consistent url in import and export
(#7053) by @shulaoda
- rolldown_plugin_vite_css_post: use `get_or_insert_default` for
`HTMLProxyResult` (#7052) by @shulaoda
- rolldown_plugin_vite_css: skip `commonjs-proxy` CSS requests (#7050)
by @shulaoda
- rolldown_plugin_utils: correct `is_css_module` (#7049) by @shulaoda
- rolldown_plugin_utils: correct `is_css_request` (#7048) by @shulaoda
- rolldown_plugin_vite_html: use correct inline module index (#7046) by
@shulaoda
- rolldown_plugin_vite_html: correct scripts url update logic (#7045) by
@shulaoda
- rolldown_plugin_vite_html: fallback to original url on NotFound error
(#7043) by @shulaoda
- rolldown_plugin_vite_html: move src_tasks to correct branch (#7040) by
@shulaoda
- rolldown_plugin_vite_html: correct `handle_style_tag_or_attribute`
(#7038) by @shulaoda
- builtin-plugin: add `config` to `htmlInlineProxyPlugin` (#7036) by
@shulaoda
- missing CJS default export when SafelyMergeCjsNs optimization is
enabled (#7006) by @Copilot
- reserve global names before deconflicting external symbols (#7022) by
@IWANABETHATGUY
- rolldown_plugin_build_import_analysis: process all bundle outputs
correctly (#7020) by @shulaoda
- rolldown_plugin_vite_css_post: process all bundle outputs correctly
(#7019) by @shulaoda
- rolldown_plugin_vite_css_post: remove `/*$vite$:1*/` correctly (#7018)
by @shulaoda
- rolldown_plugin_vite_html: use correct span for `style_urls` (#7017)
by @shulaoda
- rolldown_plugin_vite_html: track full element span from start to end
tag (#7016) by @shulaoda
- builtin-plugin: correct `viteHtmlPlugin` related logic (#7013) by
@shulaoda
- remove unused module namespace object exporting (#7002) by
@IWANABETHATGUY
- rust/dev: allow to recover from hmr rebuild failure (#6991) by @hyf0
- rust/dev: `ensure_latest_bundle_output` shouldn't loop infinitely
(#6974) by @hyf0
- rust/dev: `DevEngine#ensure_latest_bundle_output` should schedule a
rebuild task if there're no queued tasks (#6968) by @hyf0
- add Symbol.toStringTag to module facades when generatedCode.symbols is
enabled (#6784) by @Copilot

### 🚜 Refactor

- extension checking to use constant array (#7057) by @Copilot
- rust/devtools: tweak namings and introduction comments (#7028) by
@hyf0
- rolldown_plugin_vite_html: use `root` instead of `cwd` (#7035) by
@shulaoda
- rolldown_plugin_vite_css_post: use `root` instead of `cwd` (#7034) by
@shulaoda
- rolldown_plugin_vite_css: use `root` instead of `cwd` (#7033) by
@shulaoda
- rolldown_plugin_transform: use `root` instead of `cwd` (#7032) by
@shulaoda
- rolldown_plugin_reporter: use `root` instead of `cwd` (#7031) by
@shulaoda
- rolldown_plugin_asset: use `root` instead of `cwd` (#7030) by
@shulaoda
- rolldown_plugin_html_inline_proxy: use `root` instead of `cwd` (#7029)
by @shulaoda
- rust/dev: remove dead code of `rolldown_dev` crate (#6997) by @hyf0
- rust: move dev related code into new `rolldown_dev` crate (#6996) by
@hyf0
- rolldown_resolver: use consistent generic parameter name `Fs` (#6998)
by @shulaoda
- rolldown_resolver: improve resolve method clarity and documentation
(#6986) by @shulaoda
- rename `is_module_facade()` to `is_entry_point()` for clarity (#6994)
by @IWANABETHATGUY
- rust/dev: unwrap `Result<_>` from the return type of
`BundleCoordinator::schedule_build_if_stale` (#6980) by @sapphi-red
- rolldown_resolver: reorganize impl blocks (#6984) by @shulaoda
- rolldown_resolver: extract configuration logic into separate module
(#6983) by @shulaoda
- rolldown_resolver: improve error messages (#6982) by @shulaoda
- rust/dev: rename `CoordinatorStatus` to `CoordinatorStateSnapshot`
(#6973) by @hyf0
- rust/dev: replace `InitialBuildState` with `CoordinatorState` (#6972)
by @hyf0
- ast_scanner: derive `Debug`, `Clone`, `Copy` for
`CjsGlobalAssignmentType` (#6971) by @camc314
- rust: filter out devtools specific events for normal tracing (#6965)
by @hyf0
- rust/dev: replace `CoordinatorMsg::HasLatestBuildOutput` with
`GetStatus` (#6960) by @hyf0
- rust/dev: `ensure_current_build_finish` shouldn't block the
coordinator's event loop (#6959) by @hyf0

### 📚 Documentation

- in-depth/directives: remove TODOs and fix code (#7112) by @sapphi-red
- clarify concepts of rolldown's test infra (#7047) by @hyf0
- contrib/style: add suggestions about choosing file names (#6989) by
@hyf0

### ⚡ Performance

- rolldown_plugin_vite_css_post: cache CSS URL processing results
(#7056) by @shulaoda
- remove unnecessary `collect_vec` (#6999) by @IWANABETHATGUY

### 🧪 Testing

- add testcase for #6880 and #6879 (#7107) by @IWANABETHATGUY
- vite-tests: use integration branch for vite compatibility tests
(#7091) by @shulaoda

### ⚙️ Miscellaneous Tasks

- remove redundant chunk level linefeed (#7109) by @IWANABETHATGUY
- pin oxc-minify to 0.97.0 (#7108) by @IWANABETHATGUY
- deps: update oxc apps (#7104) by @renovate[bot]
- deps: update glob for security (#7105) by @shulaoda
- rolldown: add aliases for renamed vite plugins (#7087) by @shulaoda
- automate weekly beta releases (#7089) by @Boshen
- deps: update dependency oxlint-tsgolint to v0.7.0 (#7088) by
@renovate[bot]
- deps: update github-actions (#7075) by @renovate[bot]
- deps: update dependency oxlint-tsgolint to v0.6.0 (#7037) by
@renovate[bot]
- deps: update npm packages (#7076) by @renovate[bot]
- deps: update rust crates (#7077) by @renovate[bot]
- add retry to flaky tests (#7041) by @sapphi-red
- rust: rename `rolldown_debug` to `rolldown_devtools` (#7026) by @hyf0
- deps: update crate-ci/typos action to v1.39.2 (#7001) by
@renovate[bot]
- ai/github: make copilot review check rust api style (#6988) by @hyf0
- move test `recover_from_initial_build_error` to
`error_recovery/from_initial_build_syntax_error` (#6990) by @hyf0
- oxlint: enable `typescript/consistent-type-imports` rule (#6987) by
@shulaoda
- deps: update crate-ci/typos action to v1.39.1 (#6975) by
@renovate[bot]
- build.ts: separate import type (#6921) by @iiio2
- format rolldown runtime (#6966) by @IWANABETHATGUY

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.

undefined named exports are being added

3 participants