fix(napi): Add TypeName implementation for ArrayBuffer#3087
fix(napi): Add TypeName implementation for ArrayBuffer#3087Brooooooklyn merged 1 commit intonapi-rs:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a TypeName impl for Changes
Sequence Diagram(s)sequenceDiagram
participant JS as JavaScript caller
participant CJS as CommonJS/Binding shim
participant Native as Native binding (napi module)
participant Task as AsyncTaskArrayBuffer
participant Env as N-API Env / JS runtime
JS->>CJS: call asyncTaskArraybuffer(Uint8Array)
CJS->>Native: forward call with bytes
Native->>Task: spawn AsyncTaskArrayBuffer with Vec<u8>
Task->>Task: compute() (simulate work / sleep)
Task->>Native: resolve with Vec<u8>
Native->>Env: create ArrayBuffer from Vec<u8>
Env-->>Native: ArrayBuffer reference
Native-->>CJS: Promise resolves with ArrayBuffer
CJS-->>JS: Promise resolves
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🧹 Nitpick comments (1)
examples/napi/src/task.rs (1)
205-209: Consider avoiding the clone for better performance.The
clone()on line 208 copies the entire data vector. Sinceself.datais owned and won't be used aftercompute(), you could usestd::mem::take(&mut self.data)to move the data without cloning, which would be more efficient for large arrays.♻️ Proposed optimization
fn compute(&mut self) -> Result<Self::Output> { // Simulate some async computation sleep(std::time::Duration::from_millis(10)); - Ok(self.data.clone()) + Ok(std::mem::take(&mut self.data)) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/napi/__tests__/__snapshots__/values.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
crates/napi/src/bindgen_runtime/js_values/arraybuffer.rsexamples/napi/__tests__/__snapshots__/values.spec.ts.mdexamples/napi/__tests__/values.spec.tsexamples/napi/example.wasi-browser.jsexamples/napi/example.wasi.cjsexamples/napi/index.cjsexamples/napi/index.d.ctsexamples/napi/src/task.rs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
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.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(object)]` attribute for plain objects that should be mapped to TypeScript interfaces
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(js_name = "...")]` attribute to rename Rust items in JavaScript/TypeScript
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi]` attribute on impl blocks to define classes exported to JavaScript/TypeScript
Applied to files:
examples/napi/example.wasi-browser.jscrates/napi/src/bindgen_runtime/js_values/arraybuffer.rsexamples/napi/example.wasi.cjs
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(js_name = "...")]` attribute to rename Rust items in JavaScript/TypeScript
Applied to files:
crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(object)]` attribute for plain objects that should be mapped to TypeScript interfaces
Applied to files:
crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs
🧬 Code graph analysis (3)
examples/napi/src/task.rs (2)
crates/napi/src/task.rs (6)
compute(11-11)compute(33-35)compute(58-58)resolve(14-14)resolve(37-39)resolve(61-61)crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs (4)
env(262-262)env(1792-1792)from_data(157-214)from_data(1671-1742)
examples/napi/index.d.cts (1)
examples/napi/example.wasi-browser.js (2)
asyncTaskArraybuffer(146-146)asyncTaskArraybuffer(146-146)
examples/napi/__tests__/values.spec.ts (1)
examples/napi/example.wasi-browser.js (2)
asyncTaskArraybuffer(146-146)asyncTaskArraybuffer(146-146)
🔇 Additional comments (9)
examples/napi/index.d.cts (1)
440-440: LGTM! TypeScript declaration matches the Rust implementation.The function signature correctly maps
Vec<u8>toArray<number>andAsyncTask<AsyncTaskArrayBuffer>toPromise<ArrayBuffer>.crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs (1)
114-122: LGTM! TypeName implementation is correct and consistent.The implementation correctly returns "ArrayBuffer" as the type name and
ValueType::Objectas the value type, following the same pattern as other types in the codebase (e.g.,TypedArrayat lines 366-374).examples/napi/src/task.rs (1)
216-219: LGTM! Constructor follows established patterns.The implementation is consistent with other async task constructors in this file and correctly demonstrates using ArrayBuffer with ScopeTask.
examples/napi/example.wasi-browser.js (1)
146-146: LGTM! Export binding is consistent.The export follows the established pattern used throughout this file.
examples/napi/__tests__/__snapshots__/values.spec.ts.md (1)
480-480: LGTM! Snapshot correctly reflects the new API.The snapshot update is automatically generated and matches the TypeScript declaration.
examples/napi/index.cjs (1)
659-659: LGTM!The export wiring follows the established pattern in this auto-generated file.
examples/napi/example.wasi.cjs (1)
191-191: LGTM!The WASI binding export is consistent with the corresponding export in
index.cjsand follows the established pattern.examples/napi/__tests__/values.spec.ts (2)
97-97: LGTM!Import is correctly placed within the async task function imports.
1535-1558: Well-structured test coverage.The test comprehensively covers:
- Basic functionality with a simple 5-byte array
- Empty array edge case (important for boundary conditions)
- Larger data handling (1000 bytes)
The assertions properly verify both the
ArrayBuffertype and its contents viaUint8Arrayview.
Brooooooklyn
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/napi/src/task.rs (1)
196-214: Consider avoiding the clone in compute().The
compute()method currently clonesself.data(Line 208), which allocates a newVec<u8>. Since you have mutable access (&mut self), you could usestd::mem::take(&mut self.data)to move the data without cloning:fn compute(&mut self) -> Result<Self::Output> { // Simulate some async computation sleep(std::time::Duration::from_millis(10)); Ok(std::mem::take(&mut self.data)) }This avoids an unnecessary allocation and is more efficient, especially for large buffers.
♻️ Proposed refactor to eliminate the clone
fn compute(&mut self) -> Result<Self::Output> { // Simulate some async computation sleep(std::time::Duration::from_millis(10)); - Ok(self.data.clone()) + Ok(std::mem::take(&mut self.data)) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/napi/__tests__/__snapshots__/values.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
crates/napi/src/bindgen_runtime/js_values/arraybuffer.rsexamples/napi/__tests__/__snapshots__/values.spec.ts.mdexamples/napi/__tests__/values.spec.tsexamples/napi/example.wasi-browser.jsexamples/napi/example.wasi.cjsexamples/napi/index.cjsexamples/napi/index.d.ctsexamples/napi/src/task.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs
- examples/napi/index.d.cts
- examples/napi/tests/values.spec.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
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.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(object)]` attribute for plain objects that should be mapped to TypeScript interfaces
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi(js_name = "...")]` attribute to rename Rust items in JavaScript/TypeScript
📚 Learning: 2025-11-25T09:31:23.877Z
Learnt from: CR
Repo: napi-rs/napi-rs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:31:23.877Z
Learning: Applies to /crates/**/*.rs : Use `#[napi]` attribute on impl blocks to define classes exported to JavaScript/TypeScript
Applied to files:
examples/napi/example.wasi.cjsexamples/napi/example.wasi-browser.js
🧬 Code graph analysis (1)
examples/napi/src/task.rs (2)
crates/napi/src/task.rs (6)
compute(11-11)compute(33-35)compute(58-58)resolve(14-14)resolve(37-39)resolve(61-61)crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs (4)
env(262-262)env(1792-1792)from_data(157-214)from_data(1671-1742)
🔇 Additional comments (1)
examples/napi/src/task.rs (1)
216-219: LGTM!The wrapper function correctly follows the established pattern for async task functions in this file, properly constructing the
AsyncTaskArrayBufferand wrapping it in anAsyncTask.
2 participants
Add TypeName implementation for ArrayBuffer that allow us to use it in ScopeTask
Summary by CodeRabbit
New Features
Tests
Examples
✏️ Tip: You can customize this high-level summary in your review settings.