Migrate metassr-bundler to rspack Native Rust API#100
Migrate metassr-bundler to rspack Native Rust API#100suleman1412 wants to merge 7 commits intometacall:masterfrom
metassr-bundler to rspack Native Rust API#100Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates metassr-bundler away from the MetaCall/Node.js bundle.js bridge to using rspack’s native Rust API directly, aiming to keep the existing WebBundler::new / WebBundler::exec public interface intact while removing the Node dependency.
Changes:
- Replaced MetaCall-based bundling with rspack Rust API usage in
WebBundler::exec(with an internal async implementation). - Removed the Node-side bundling script (
bundle.js) and the MetaCall build script (build.rs). - Updated
metassr-bundlerdependencies to pull rspack from the upstream git repo and added Tokio runtime support.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/metassr-bundler/src/lib.rs | Reimplements bundling using rspack Rust API and introduces async build wrapped by a sync exec(). |
| crates/metassr-bundler/src/bundle.js | Removes the legacy Node.js rspack configuration and runner. |
| crates/metassr-bundler/build.rs | Removes MetaCall native build hook. |
| crates/metassr-bundler/Cargo.toml | Switches dependencies from MetaCall to rspack + tokio and updates versions. |
| Cargo.lock | Locks new rspack (git) dependency graph and related transitive crates. |
Comments suppressed due to low confidence (1)
crates/metassr-bundler/src/lib.rs:66
- The public struct/module docs still describe the old MetaCall +
bundle.jsapproach, but the implementation now uses rspack’s Rust API directly. Please update the doc comments (including theexecdocs) so they don’t mention Node.js/MetaCall, and document the new runtime behavior (sync wrapper around async build).
/// A web bundler that invokes the `web_bundling` function from the Node.js `bundle.js` script
/// using MetaCall. It is designed to bundle web resources like JavaScript and TypeScript files
/// by calling a custom `rspack` configuration.
///
/// The `exec` function blocks the execution until the bundling process completes.
#[derive(Debug)]
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/metassr-bundler/src/lib.rs
Outdated
| tokio::task::block_in_place(|| handle.block_on(async { self.exec_async().await })) | ||
| } | ||
| Err(_) => { | ||
| tokio::runtime::Runtime::new() | ||
| .map_err(|e| anyhow!("Failed to create runtime: {:?}", e))? | ||
| .block_on(async { self.exec_async().await }) |
There was a problem hiding this comment.
tokio::task::block_in_place will panic when exec() is called from a current-thread Tokio runtime. Since Handle::try_current() doesn’t guarantee a multi-thread runtime, this can crash callers. Please detect the runtime flavor and either (a) use block_in_place only on multi-thread runtimes, or (b) run the build on a dedicated thread/runtime (e.g., spawn a thread with a new Runtime and join) when already inside a current-thread runtime.
| tokio::task::block_in_place(|| handle.block_on(async { self.exec_async().await })) | |
| } | |
| Err(_) => { | |
| tokio::runtime::Runtime::new() | |
| .map_err(|e| anyhow!("Failed to create runtime: {:?}", e))? | |
| .block_on(async { self.exec_async().await }) | |
| match handle.runtime_flavor() { | |
| tokio::runtime::RuntimeFlavor::MultiThread => { | |
| tokio::task::block_in_place(|| handle.block_on(self.exec_async())) | |
| } | |
| tokio::runtime::RuntimeFlavor::CurrentThread => { | |
| // `block_in_place` panics on current-thread runtimes. Run the build | |
| // on a dedicated thread with its own runtime and join it instead. | |
| std::thread::scope(|s| { | |
| let join_handle = s.spawn(|| { | |
| tokio::runtime::Runtime::new() | |
| .map_err(|e| anyhow!("Failed to create runtime: {:?}", e))? | |
| .block_on(self.exec_async()) | |
| }); | |
| join_handle | |
| .join() | |
| .map_err(|_| anyhow!("Bundler thread panicked"))? | |
| }) | |
| } | |
| } | |
| } | |
| Err(_) => { | |
| tokio::runtime::Runtime::new() | |
| .map_err(|e| anyhow!("Failed to create runtime: {:?}", e))? | |
| .block_on(self.exec_async()) |
| let js_rule = ModuleRule { | ||
| test: Some(RuleSetCondition::Regexp( | ||
| RspackRegex::new(r#"\.(jsx|js)$"#).unwrap(), | ||
| )), | ||
| effect: ModuleRuleEffect { | ||
| r#use: ModuleRuleUse::Array(vec![ModuleRuleUseLoader { | ||
| loader: "builtin:swc-loader".to_string(), | ||
| options: Some(json!({ | ||
| "jsc": { | ||
| "parser": { | ||
| "syntax": "ecmascript", | ||
| "jsx": true, | ||
| }, | ||
| "transform": { | ||
| "react": { | ||
| "runtime": "automatic", | ||
| "pragma": "React.createElement", | ||
| "pragmaFrag": "React.Fragment", | ||
| "throwIfNamespace": true, | ||
| "useBuiltins": false | ||
| } | ||
| } | ||
| } | ||
| }).to_string()), | ||
| }]), | ||
| ..Default::default() | ||
| }, | ||
| ..Default::default() | ||
| }; |
There was a problem hiding this comment.
The new rspack config only defines a loader rule for .js/.jsx files. The previous implementation supported .ts/.tsx (and inline asset handling) as part of bundling; with this change, TypeScript entries or TS/TSX imports will not be transformed and will likely fail to compile. Please add an explicit TS/TSX rule (swc parser typescript/tsx) and restore any other rules (e.g., inline assets) that were part of the removed bundle.js behavior.
| let js_rule = ModuleRule { | ||
| test: Some(RuleSetCondition::Regexp( | ||
| RspackRegex::new(r#"\.(jsx|js)$"#).unwrap(), | ||
| )), | ||
| effect: ModuleRuleEffect { | ||
| r#use: ModuleRuleUse::Array(vec![ModuleRuleUseLoader { | ||
| loader: "builtin:swc-loader".to_string(), | ||
| options: Some(json!({ | ||
| "jsc": { | ||
| "parser": { | ||
| "syntax": "ecmascript", | ||
| "jsx": true, | ||
| }, | ||
| "transform": { | ||
| "react": { | ||
| "runtime": "automatic", | ||
| "pragma": "React.createElement", | ||
| "pragmaFrag": "React.Fragment", | ||
| "throwIfNamespace": true, | ||
| "useBuiltins": false | ||
| } | ||
| } | ||
| } | ||
| }).to_string()), | ||
| }]), | ||
| ..Default::default() | ||
| }, | ||
| ..Default::default() | ||
| }; |
There was a problem hiding this comment.
The JS loader rule no longer excludes node_modules (the old bundle.js config did). Without an exclusion, rspack/swc may attempt to transpile dependencies, which can significantly slow builds and potentially break on packages that expect to be left as-is. Please add an exclude condition for node_modules (or otherwise match the previous exclusion behavior).
|
|
||
| result | ||
| } | ||
| async fn exec_async(&self) -> Result<()> { |
There was a problem hiding this comment.
why are you making a new function? isn't doing that in exec(&self) is better?
There was a problem hiding this comment.
I chose to create a private helper exec_async() because rspack's compiler.build() returns a future and requires an async context, but exec() must remain synchronous to preserve the existing public API.. exec_async() contains all the async bundling logic, and exec() handles thread management and can call private async helper fn.
crates/metassr-bundler/src/lib.rs
Outdated
| compilation_wait.cond.notify_one(); | ||
| let js_rule = ModuleRule { | ||
| test: Some(RuleSetCondition::Regexp( | ||
| RspackRegex::new(r#"\.(jsx|js)$"#).unwrap(), |
There was a problem hiding this comment.
I think that is not working with typescript files
There was a problem hiding this comment.
added support for ts files in the latest commit
crates/metassr-bundler/Cargo.toml
Outdated
| rspack_core = { git = "https://github.com/web-infra-dev/rspack", version = "0.100.0-beta.5", package = "rspack_core" } | ||
| rspack_paths = { git = "https://github.com/web-infra-dev/rspack", version = "0.100.0-beta.5", package = "rspack_paths" } | ||
| rspack_regex = { git = "https://github.com/web-infra-dev/rspack", version = "0.100.0-beta.5", package = "rspack_regex" } | ||
| rspack_tasks = { git = "https://github.com/web-infra-dev/rspack", version = "0.100.0-beta.5", package = "rspack_tasks" } |
There was a problem hiding this comment.
do we need all these deps? also, cannot import them from crates.io instead of the git repo?
There was a problem hiding this comment.
All five rspack crates are required as each exposes a distinct set of types used directly in the implementation — rspack for the builder API, rspack_core for module/rule types, rspack_paths for UTF-8 path handling, rspack_regex for rule conditions, and rspack_fs for the native filesystem.
Unfortunately these cannot be pulled from crates.io. The versions published there are significantly outdated and do not expose the Rust-native builder API we are relying on (Compiler::builder, ModuleRule, OutputOptionsBuilder, etc.). The git dependency pointing to the v2.0.0-beta.5 tag is the only way to access these APIs at this time.
If and when rspack publishes an up-to-date version to crates.io that includes this API surface, the git dependencies can be replaced with registry ones. Happy to make that change if/when that becomes possible. :)
|
@hulxv I've addressed all the requested changes. Would appreciate your review when you get a chance! |
This PR replaces the previous MetaCall/
bundle.jsbased bundling approach with rspack's native Rust API, removing the Node.js bridge dependency entirely.Fixes #86
Changes done:
The
rspackcrate on crates.io is outdated. UpdatedCargo.tomlto use the latest beta tag along with its required dependencies, and rewrote the implementation against the new API.Read through the existing
lib.rsandbundle.jsto understand the intended behavior, then reimplemented it using rspack's Rust API directly. The public interface (WebBundler::new,WebBundler::exec) is unchanged.rspack's
compiler.build()returns a future, so an internalexec_async()was introduced to handle the async work.exec()remains synchronous to avoid breaking the rest of the codebase that calls it synchronously. This is handled viatokio::task::block_in_placewhen called from within an existing tokio runtime, with a fallback toRuntime::new().block_on()otherwise.Note: I used AI to help navigate the rspack codebase and understand available APIs, as this is my first Rust contribution. All logic has been reviewed and understood by me, and the reasoning behind each decision is documented above.