feat(rust-plugins): rewrite wasm plugin #115
Conversation
|
@CCherry07 ptal |
WalkthroughThis pull request introduces a patch to the Changes
Sequence Diagram(s)sequenceDiagram
participant M as Main Component
participant R as rust-wasm Module
participant I as WASM Init (json_typegen_wasm)
participant L as Logger/Console
M->>R: Call greet()
R->>L: Log "Hello, rust-wasm!"
M->>I: Call init({})
I-->>M: Resolve promise with wasm module
M->>L: Log module loaded
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
rust-plugins/wasm/rust-wasm/src/utils.rs (1)
1-10: Robust panic hook setup viaset_panic_hook().The new
set_panic_hook()function leverages theconsole_error_panic_hookfeature flag to improve error messages on panic, which is particularly useful for WebAssembly modules. Consider adding Rust doc comments (e.g.,///) to explain its usage context for future maintainers.rust-plugins/wasm/playground/src/main.tsx (1)
60-62: Robust initialization of the wasm module.The updated initialization pattern using
init({}).then((wasm) => { ... })correctly handles the asynchronous loading of the wasm module. To further enhance robustness, consider adding error handling (e.g., a.catch(...)callback) to manage potential promise rejections during module initialization.rust-plugins/wasm/src/lib.rs (1)
95-100: Commented Out Code for WASM URL Construction
There is a block of commented-out code (lines 95-100) for constructingwasm_url. If this code is no longer needed, please remove it to keep the codebase clean.rust-plugins/wasm/rust-wasm/Cargo.toml (1)
1-30: New Rust Package Configuration
This newCargo.tomlsets up therust-wasmpackage with appropriate metadata, library types, and dependencies. The configuration is solid and follows common practices. As a suggestion, if there are no constraints, you might consider updating the Rust edition from"2018"to"2021"to take advantage of the latest language improvements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yamlrust-plugins/wasm/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.changeset/shy-glasses-sparkle.md(1 hunks)rust-plugins/wasm/Cargo.toml(1 hunks)rust-plugins/wasm/package.json(2 hunks)rust-plugins/wasm/playground/index.js(0 hunks)rust-plugins/wasm/playground/package.json(1 hunks)rust-plugins/wasm/playground/src/main.tsx(2 hunks)rust-plugins/wasm/rust-wasm/.gitignore(1 hunks)rust-plugins/wasm/rust-wasm/Cargo.toml(1 hunks)rust-plugins/wasm/rust-wasm/src/lib.rs(1 hunks)rust-plugins/wasm/rust-wasm/src/utils.rs(1 hunks)rust-plugins/wasm/src/lib.rs(4 hunks)rust-plugins/wasm/src/utils.rs(1 hunks)
💤 Files with no reviewable changes (1)
- rust-plugins/wasm/playground/index.js
✅ Files skipped from review due to trivial changes (1)
- rust-plugins/wasm/rust-wasm/.gitignore
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-arm64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-ia32-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-x64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-gnu
🔇 Additional comments (14)
.changeset/shy-glasses-sparkle.md (1)
1-6: Ensure clarity and completeness in changeset documentation.The changeset entry clearly marks the patch for
@farmfe/plugin-wasmand provides a concise description of rewriting the wasm plugin to parse wasm binary imports. Consider including additional context (such as rationale, backward compatibility notes, or related issue references) if applicable.rust-plugins/wasm/Cargo.toml (1)
15-15: New dependency added:wasmparser = "0.225.0".This dependency is critical for the new wasm binary import parsing functionality. Please ensure that this version is fully compatible with the rest of the crate and that any related documentation (e.g., upgrade notes) is updated.
rust-plugins/wasm/rust-wasm/src/lib.rs (1)
1-15: Seamless integration of wasm-bindgen with addedgreet()function.The file correctly declares the new
utilsmodule and sets up the wasm-bindgen environment. Thegreet()function, which calls the imported JavaScriptlogfunction, is structured clearly and demonstrates a simple interop between Rust and JavaScript. This foundation will make further enhancements easier.rust-plugins/wasm/playground/src/main.tsx (3)
6-6: Proper import of the newgreetfunction.The addition of
import { greet } from "rust-wasm"correctly brings in the new functionality. Verify that the module resolution is handled appropriately by the bundler and that the path"rust-wasm"accurately reflects the output package configuration.
10-10: Immediate invocation ofgreet()for startup feedback.Calling
greet()at the start of theMaincomponent ensures that the WebAssembly module communicates with JavaScript upon initialization. This proactive call helps verify that the new wasm bindings are working as intended.
53-59: Asynchronous transformation handling is clear and concise.The async function
transformleveragestransformToInterface()to convert JSON to TypeScript interfaces and logs the result. Ensure that the transformed interfaces are either consumed later or validated, in order to maintain the integrity of downstream operations.rust-plugins/wasm/package.json (2)
9-10: New DevDependencies Added
The addition of@farmfe/plugin-toolsand@types/nodeenhances type checking and build tooling. Please ensure these versions are aligned with the rest of the project.
37-39: Enhanced Build & Play Scripts
Updating the"play"script to runpnpm build:wasmfirst ensures that the WebAssembly module is built before the playground starts. The new"build:wasm"script usingwasm-packis well integrated. Verify that the relative output directory (../playground/wasm) is valid within your project structure.rust-plugins/wasm/src/lib.rs (2)
1-1: Module Exposure for Utility Functions
The newpub mod utils;declaration and its corresponding import (use utils::generate_glue_code;) correctly expose and use helper functions from the utilities module.
102-116: Conditional Generation of Glue Code
The conditional branch for generating content based on theinitflag is logical and correctly integrates thegenerate_glue_codeutility. Make sure that any potential errors fromgenerate_glue_codeare handled appropriately upstream.rust-plugins/wasm/src/utils.rs (2)
1-17: New Utility Module Declarations
The introduction ofWasmInfoandImportInfo, along with the use ofCompilationErrorfor error mapping, is well implemented. These declarations set a clear contract for parsing and processing WASM binaries.
64-125: Overall Glue Code Generation Logic
The function efficiently combines parsing results into JS glue code, assembling import statements, initialization code, and export statements. Error handling is properly cascaded fromparse_wasm. The structure is clear and should integrate seamlessly with the WebAssembly plugin.rust-plugins/wasm/playground/package.json (2)
16-18: Updated Dependencies for React & Local Rust-WASM
Upgradingreactandreact-domto^18.3.1promotes compatibility with newer features. The inclusion of"rust-wasm": "link:./wasm"integrates the new local Rust-WASM package; please ensure contributors understand the implications of local linking (e.g., via pnpm's linking mechanism).
21-27: DevDependencies Update
The updates to@farmfe/cli,@farmfe/core,@farmfe/plugin-react, and type definitions reflect a positive modernization of the playground environment. Verify that all these versions work well together after the upgrade.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rust-plugins/wasm/src/lib.rs (1)
99-104: Remove commented-out code.The commented-out code for
wasm_urlconstruction should be removed as it's no longer needed. This improves code maintainability.- // let wasm_url = if !context.config.output.public_path.is_empty() { - // let normalized_public_path = context.config.output.public_path.trim_end_matches('/'); - // format!("{}/{}", normalized_public_path, resolved_path) - // } else { - // format!("/{}", resolved_path) - // };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust-plugins/wasm/src/lib.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-arm64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-ia32-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-x64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-gnu
🔇 Additional comments (5)
rust-plugins/wasm/src/lib.rs (5)
1-1: LGTM! Clean module organization.The new
utilsmodule is properly declared and its function is imported following Rust conventions.Also applies to: 14-14
39-54: LGTM! Clean resolve hook implementation.The resolve hook correctly handles the WASM helper ID case and follows the early return pattern for better readability.
73-77: LGTM! Improved error handling.The file reading error handling has been improved by properly propagating errors using
map_errinstead ofunwrap().
131-172: LGTM! Robust cache handling implementation.The cache handling code properly manages serialization/deserialization and follows good error handling practices.
119-119:✅ Verification successful
Verify error handling in generate_glue_code.
The
generate_glue_codecall properly propagates errors using the?operator. However, let's verify its error handling implementation.
🏁 Script executed:
#!/bin/bash # Search for error handling in generate_glue_code implementation ast-grep --pattern 'fn generate_glue_code($_) -> Result<$_, $_> { $$$ }'Length of output: 74
I'll run a new shell script to search for the definition of
generate_glue_codeto better inspect its error-handling implementation.
🏁 Script executed:
#!/bin/bash # Search for the definition of generate_glue_code across the codebase rg -n "fn generate_glue_code" .Length of output: 96
Error handling in
generate_glue_codeverifiedThe implementation in
rust-plugins/wasm/src/utils.rs(around line 64) correctly returns aResultand propagates errors as expected with the?operator. The call inrust-plugins/wasm/src/lib.rsat line 119 properly leverages this behavior. No changes are required.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rust-plugins/wasm/src/lib.rs (1)
99-104: Remove commented-out code.If the public path handling is no longer needed, remove these commented lines to improve code maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rust-plugins/wasm/src/lib.rs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-arm64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-ia32-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-x64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-gnu
🔇 Additional comments (4)
rust-plugins/wasm/src/lib.rs (4)
1-19: LGTM! Well-organized imports and module declarations.The new
utilsmodule and its imported function are properly integrated with the existing imports.
20-55: LGTM! Clean plugin implementation.The resolve hook is properly simplified to handle only the wasm helper ID case.
74-77: LGTM! Proper error handling implementation.The error handling for file reading is now properly implemented using
map_errinstead ofunwrap().
131-173: LGTM! Solid cache handling implementation.The cache handling logic is well-implemented with proper error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
rust-plugins/wasm/src/lib.rs (1)
99-104: Remove commented-out code.The commented-out URL construction code should be removed as it's no longer needed and adds unnecessary noise to the codebase.
- // let wasm_url = if !context.config.output.public_path.is_empty() { - // let normalized_public_path = context.config.output.public_path.trim_end_matches('/'); - // format!("{}/{}", normalized_public_path, resolved_path) - // } else { - // format!("/{}", resolved_path) - // };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rust-plugins/wasm/rust-wasm/Cargo.toml(1 hunks)rust-plugins/wasm/rust-wasm/src/lib.rs(1 hunks)rust-plugins/wasm/src/lib.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- rust-plugins/wasm/rust-wasm/src/lib.rs
- rust-plugins/wasm/rust-wasm/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-arm64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-ia32-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-x64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-gnu
🔇 Additional comments (4)
rust-plugins/wasm/src/lib.rs (4)
1-19: LGTM! Clean module organization.The module declarations and imports are well-organized, with proper separation of concerns through the new
utilsmodule.
20-55: LGTM! Clean plugin implementation.The plugin implementation is concise and follows best practices. The resolve method is properly simplified.
73-77: LGTM! Improved error handling.The error handling has been improved by replacing
unwrap()with proper error propagation usingmap_err. This addresses the previous review comment and makes the code more robust.
130-172: LGTM! Robust cache handling.The cache handling implementation is well-structured with proper error handling and resource management.
Summary by CodeRabbit
New Features
Chores