fix(cli): properly load WASM package in JS binding template#3038
Conversation
How to use the Graphite Merge QueueAdd the label ready-to-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. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughWASI fallback now triggers when Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Loader as JS loader
participant Native as nativeBinding
participant WASI as wasiBinding
participant FS as require.resolve (filesystem)
Client->>Loader: require module
Loader->>Native: attempt load nativeBinding
alt nativeBinding loaded and not forcing WASI
Native-->>Loader: success
Loader-->>Client: export native API
else nativeBinding missing or NAPI_RS_FORCE_WASI set
Loader->>WASI: attempt load wasiBinding
note right of WASI `#DDEBF7`: new path resolution may use FS for wasm file
WASI->>FS: require.resolve('${packageName}-wasm32-wasi/${wasmFileName}.wasm')
alt wasiBinding loaded
WASI-->>Loader: success
Loader-->>Client: export WASI API
else wasiBinding load fails
WASI-->>Loader: error
alt NAPI_RS_FORCE_WASI == 'error'
Loader-->>Client: throw forced-WASI not-found error
else
Loader-->>Client: push/record errors (chain cause if present)
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (5)📚 Learning: 2025-11-25T09:31:23.877ZApplied to files:
📚 Learning: 2025-11-25T09:31:23.877ZApplied to files:
📚 Learning: 2025-11-25T09:31:23.877ZApplied to files:
📚 Learning: 2025-11-25T09:31:23.877ZApplied to files:
📚 Learning: 2025-11-25T09:31:23.877ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (42)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/api/templates/js-binding.ts (1)
230-257:NAPI_RS_FORCE_WASIpath can throw via nullwasiBindingErrorwhen${pkgName}-wasm32-wasiis missingWith the updated condition at Line 241:
if (!nativeBinding || process.env.NAPI_RS_FORCE_WASI) {the second WASI fallback is executed even when
nativeBindingis already successfully set to a WASI binding from./${localName}.wasi.cjs.In the scenario:
process.env.NAPI_RS_FORCE_WASIis set (e.g.'1'or'error'),require('./${localName}.wasi.cjs')succeeds →nativeBindingbecomes truthy andwasiBindingErrorstaysnull,require('${pkgName}-wasm32-wasi')then fails,the catch at Lines 245–249 runs and does:
wasiBindingError.cause = errbut
wasiBindingErroris stillnull, so this will raise a TypeError while evaluating the binding, instead of giving a clean, intentional error.You can avoid this by:
- Only trying the
${pkgName}-wasm32-wasipackage when we still don’t have a WASI binding, and- Guarding
wasiBindingErrorbefore assigningcause.A minimal patch:
- if (!nativeBinding || process.env.NAPI_RS_FORCE_WASI) { + if (!nativeBinding) { try { wasiBinding = require('${pkgName}-wasm32-wasi') nativeBinding = wasiBinding } catch (err) { if (process.env.NAPI_RS_FORCE_WASI) { - wasiBindingError.cause = err - loadErrors.push(err) + if (wasiBindingError) { + wasiBindingError.cause = err + } else { + wasiBindingError = err + } + loadErrors.push(err) } } }This still honors
NAPI_RS_FORCE_WASIsemantics:
- Native binding is bypassed whenever the env var is set (via the outer
if (!nativeBinding || process.env.NAPI_RS_FORCE_WASI)),- You try
./${localName}.wasi.cjsfirst, then${pkgName}-wasm32-wasionly if no WASI binding is loaded yet,- Aggregated error information for the
'error'mode remains intact, without risking a null dereference.
🧹 Nitpick comments (1)
cli/src/api/templates/load-wasi-template.ts (1)
141-148: Fallback tonode_modules/${packageName}-wasm32-wasiis straightforward but assumes cwd layoutPointing
__wasmFilePathstraight at:__nodePath.resolve('node_modules/${packageName}-wasm32-wasi/${wasmFileName}.debug.wasm')is a nice simplification and will work for the common case where the process runs from the project root.
If consumers might execute from subdirectories or in monorepos/hoisted setups, you may eventually want a more robust resolution (e.g. via
require.resolveon the WASI package and then joining${wasmFileName}.debug.wasm) so you follow Node’s module resolution instead of depending onprocess.cwd()and a flatnode_moduleslayout. Not a blocker, just something to keep in mind.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/src/api/templates/js-binding.ts(1 hunks)cli/src/api/templates/load-wasi-template.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.866Z
Learning: Applies to /crates/**/*.rs : Use `#[napi]` attribute on impl blocks to define classes exported to JavaScript/TypeScript
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.866Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(js_name = "...")]` attribute to rename Rust items in JavaScript/TypeScript
There was a problem hiding this comment.
Pull request overview
This PR fixes WASM package loading in the JS binding template by improving support for the NAPI_RS_FORCE_WASI environment variable and updating the fallback path resolution for WASM files.
Key Changes:
- Added explicit support for forcing WASI binding via the
NAPI_RS_FORCE_WASIenvironment variable - Improved error handling to properly chain load errors when multiple attempts fail
- Updated WASM file resolution to use a direct package path instead of generic require resolution
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cli/src/api/templates/load-wasi-template.ts |
Changed WASM fallback path from try-catch with package resolution to direct node_modules path |
cli/src/api/templates/js-binding.ts |
Enhanced NAPI_RS_FORCE_WASI support by checking it before package loading and fixing null-safe error chaining |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
551ad82 to
7c8ac81
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 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.
0683bbc to
088534e
Compare
Summary by CodeRabbit
New Features
Changes
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Strengthens WASI loading and asset resolution across templates and examples.
js-binding.tsandexamples/napi/index.cjs, attempt WASI loading whenNAPI_RS_FORCE_WASIis set (even if native binding exists) and preserve/chainwasiBindingErrorcauses; add errors toloadErrors.load-wasi-template.tsandexamples/napi/example.wasi.cjs, locate.wasmviarequire.resolve('<pkg>/<file>.wasm')instead ofpath.resolve, with clearer errors when the WASM package is missing.Written by Cursor Bugbot for commit 18fe2c3. This will update automatically on new commits. Configure here.