refactor: use ModuleId as the type of ResolvedId#id#7694
Merged
graphite-app[bot] merged 1 commit intomainfrom Dec 30, 2025
Merged
refactor: use ModuleId as the type of ResolvedId#id#7694graphite-app[bot] merged 1 commit intomainfrom
ModuleId as the type of ResolvedId#id#7694graphite-app[bot] merged 1 commit intomainfrom
Conversation
NormalizedId to ensure consistent behavior for idModuleId as the type of ResolvedId#id
Member
Author
How to use the Graphite Merge QueueAdd 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. |
✅ Deploy Preview for rolldown-rs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
8efaaba to
6d1b0f2
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This refactoring changes the type of ResolvedId#id from ArcStr to ModuleId, making the data flow clearer by using a proper domain type instead of a raw string type. The change propagates through the codebase, requiring updates to various conversion points where ModuleId values need to be created, extracted, or converted.
Key Changes:
ResolvedId#idtype changed fromArcStrtoModuleId- Added
Displaytrait implementation and helper methods (as_str(),as_arc_str()) toModuleId - Updated all code paths that create or consume
ResolvedIdto useModuleId::new()for construction - Updated binding layer conversions to use
ToString::to_stringmethod reference instead of closures
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rolldown_common/src/types/module_id.rs | Added Display impl, as_str(), and as_arc_str() methods; updated documentation; simplified new() implementation |
| crates/rolldown_common/src/types/resolved_id.rs | Changed id field type from ArcStr to ModuleId; updated helper methods accordingly |
| crates/rolldown_resolver/src/resolver.rs | Wrapped resolved path with ModuleId::new() when creating ResolvedId |
| crates/rolldown_plugin/src/utils/resolve_id_with_plugins.rs | Wrapped string IDs with ModuleId::new() at multiple creation points |
| crates/rolldown_plugin/src/utils/resolve_id_check_external.rs | Wrapped external ID with ModuleId::new() when creating ResolvedId |
| crates/rolldown_plugin/src/types/hook_resolve_id_output.rs | Extracted ArcStr from ModuleId using .as_arc_str().clone() |
| crates/rolldown_plugin/src/plugin_context/native_plugin_context.rs | Created ModuleId for plugin context operations; inefficient extraction for watch files |
| crates/rolldown_plugin_vite_import_glob/src/utils.rs | Converted ModuleId to string using .to_string() for glob path handling |
| crates/rolldown_plugin_vite_import_glob/src/utils_2.rs | Converted ModuleId to string using .to_string() for glob path handling |
| crates/rolldown/src/module_loader/module_loader.rs | Updated cache lookups to use .as_str() and insertions to use .as_arc_str().clone(); inefficient re-creation of ModuleId |
| crates/rolldown/src/module_loader/module_task.rs | Cloned ModuleId directly instead of reconstructing; extracted ArcStr for watch files |
| crates/rolldown/src/module_loader/runtime_module_task.rs | Inefficiently recreated ModuleId from constant |
| crates/rolldown/src/module_loader/resolve_utils.rs | Wrapped specifiers with ModuleId::new(); extracted ArcStr for diagnostics |
| crates/rolldown/src/module_loader/external_module_task.rs | Inefficiently recreated ModuleId; extracted ArcStr where needed |
| crates/rolldown/src/ecmascript/ecma_module_view_factory.rs | Inefficiently recreated ModuleId from existing one |
| crates/rolldown/src/bundle/bundle.rs | Changed to use ToString::to_string method reference |
| crates/rolldown_binding/src/types/binding_rendered_chunk.rs | Changed to use ToString::to_string method reference |
| crates/rolldown_binding/src/types/binding_module_info.rs | Changed to use ToString::to_string method reference |
| crates/rolldown_binding/src/options/binding_output_options/binding_pre_rendered_chunk.rs | Changed to use ToString::to_string method reference |
| crates/rolldown/tests/rolldown/plugin/plugin_context/custom_arg_in_resolve/mod.rs | Updated test assertion to use .as_str() method |
Comments suppressed due to low confidence (1)
crates/rolldown_common/src/types/module_id.rs:11
- Documentation inconsistency: The documentation claims that backslashes are converted to forward slashes for absolute Windows paths, but the implementation of
ModuleId::newdoesn't perform any such conversion. Either implement the conversion or remove this misleading documentation claim.
#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Default)]
pub struct ModuleId {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/rolldown_plugin/src/plugin_context/native_plugin_context.rs
Outdated
Show resolved
Hide resolved
Contributor
Benchmarks Rust |
Member
|
Copilot's suggestion may be worth looking into |
6d1b0f2 to
e27ab58
Compare
Member
Author
Merge activity
|
IWANABETHATGUY
approved these changes
Dec 30, 2025
`Resolved#id` is the id we're gonna used for module id, now we make it `ModuleId` directly to make the data flow more clear
e27ab58 to
6399853
Compare
IWANABETHATGUY
pushed a commit
that referenced
this pull request
Dec 30, 2025
`Resolved#id` is the id we're gonna used for module id, now we make it `ModuleId` directly to make the data flow more clear
This was referenced Dec 31, 2025
shulaoda
added a commit
that referenced
this pull request
Dec 31, 2025
## [1.0.0-beta.58] - 2025-12-31 ### 💥 BREAKING CHANGES - experimental/devtools: rename InputOptions#debug to InputOptions#devtools (#7686) by @Copilot ### 🚀 Features - implement target feature check in `should_transform_js` for raw options (#7697) by @shulaoda - support `output.dynamicImportInCjs` option (#7677) by @shulaoda - types: expose `ChecksOptions` type (#7653) by @sapphi-red ### 🐛 Bug Fixes - export runtime helpers for cross-chunk access (#7658) by @shulaoda - cjs namespace merging regression (#7665) by @IWANABETHATGUY - replace panic with proper error handling for hash placeholder generation (#7661) by @shulaoda - remove the blank line between shebang and postBanner (#7643) by @btea - rolldown_plugin_vite_reporter: apply padding before ANSI coloring for proper size column alignment (#7649) by @shulaoda ### 🚜 Refactor - rust: use `StableModuleId` as the map key if possible (#7718) by @hyf0 - rust: return `StableModuleId` instead of `&str` from `Module#stable_id()` (#7717) by @hyf0 - rust: return correct stable id of external module from `Module#stable_id()` (#7716) by @hyf0 - rust: introduce `StableModuleId` type (#7715) by @hyf0 - rust: reduce unnecessary `id.as_arc_str().clone().into()` (#7714) by @hyf0 - rust: remove `ModuleId#resource_id` and use `as_arc_str` directly (#7710) by @hyf0 - rust: remove unused `Module#id_clone` (#7709) by @hyf0 - rust: remove `Module#id_as_str` and use `Module#id` directly (#7708) by @hyf0 - consolidate namespace call analysis into import analyzer (#7657) by @IWANABETHATGUY - rust: make `ExternalModule#id` have the type `ModuleId` (#7707) by @hyf0 - rust: rename `Module#id` to `Module#id_as_str` (#7706) by @hyf0 - rust: use `ModuleId` instead of raw `ArcStr` for `ScanStageCache` (#7701) by @hyf0 - simplify error propagation in cache merge (#7702) by @shulaoda - use `ModuleId` as the type of `ResolvedId#id` (#7694) by @hyf0 - types: rename `resolved_request_info.rs` to `resolved_id.rs` and move its contents (#7687) by @hyf0 - devtools: emit data to `<CWD>/node_modules/.rolldown` (#7692) by @hyf0 - use `InvalidOption` for hash placeholder generation errors (#7674) by @shulaoda - rolldown_error: remove dependency on rolldown_utils (#7672) by @shulaoda - use nodejs-built-in-modules v1.0.0 directly in callsites (#7667) by @Boshen ### 📚 Documentation - migrate input options content from options to auto gen docs (#7663) by @mdong1909 - create reference index page (#7659) by @mdong1909 - tweak auto-generated reference output (#7654) by @sapphi-red - initialize auto-gen docs (#7252) by @mdong1909 ### ⚙️ Miscellaneous Tasks - deps: update napi (#7705) by @renovate[bot] - pin Node.js version to 24.12.0 LTS in .node-version file (#7713) by @Copilot - update esbuild test reasons (#7703) by @sapphi-red - deps: update crate-ci/typos action to v1.40.1 (#7696) by @renovate[bot] - deps: update oxc to v0.106.0 (#7512) by @renovate[bot] - js: replace dprint with oxfmt (#7214) by @Boshen - deps: update dependency oxlint to v1.36.0 (#7691) by @renovate[bot] - deps: update github-actions (#7679) by @renovate[bot] - deps: update npm packages (#7680) by @renovate[bot] - deps: update rust crates (#7678) by @renovate[bot] - deps: update oxc resolver to v11.16.2 (#7668) by @renovate[bot] - add API reference files to knip entry points (#7669) by @Copilot - deps: update notify (#7651) by @sapphi-red - add `homepage` field to package.json (#7648) by @trivikr - deps: update oxc resolver to v11.16.1 (#7647) by @renovate[bot] - deps: update rolldown-plugin-dts to 0.20.0 (#7645) by @shulaoda Co-authored-by: shulaoda <165626830+shulaoda@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolved#idis the id we're gonna used for module id, now we make itModuleIddirectly to make the data flow more clear