Skip to content

Conversation

@KevinEady
Copy link
Contributor

The previous implementation does not support calling napi_create_dataview with a SharedArrayBuffer, as the check for value->IsArrayBuffer() returns false, causing the Node-API call to return an napi_invalid_arg.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels Oct 28, 2025
@KevinEady KevinEady force-pushed the node-api-add-sharedarraybuffer-to-dataview branch from f2b1f83 to 68ea53e Compare October 28, 2025 23:35
@KevinEady KevinEady marked this pull request as ready for review October 28, 2025 23:36
@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

❌ Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.56%. Comparing base (4e9e5a1) to head (7b1f34e).
⚠️ Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
src/js_native_api_v8.cc 70.58% 2 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #60473   +/-   ##
=======================================
  Coverage   88.56%   88.56%           
=======================================
  Files         704      704           
  Lines      207826   207864   +38     
  Branches    40035    40052   +17     
=======================================
+ Hits       184059   184094   +35     
- Misses      15802    15806    +4     
+ Partials     7965     7964    -1     
Files with missing lines Coverage Δ
src/js_native_api_v8.cc 76.56% <70.58%> (+0.06%) ⬆️

... and 33 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
@KevinEady
Copy link
Contributor Author

I do not think we add macro definitions for changes in behavior (eg. no NODE_API_DATAVIEW_SUPPORTS_SHAREDARRAYBUFFER) ? If this is the case, node-addon-api will not know at compile-time whether the Node.js version used to build the addon supports SharedArrayBuffer with napi_create_dataview.

I see three paths to introduce this to node-addon-api:

  1. Catch the invalid_arg status, and do the c++ equivalent of new DataView( buffer, byteOffset, byteLength ).
  2. Return any non-ok status and do not do anything special.
  3. Introduce a macro definition in this PR that node-addon-api can recognize.

Thoughts? Or any other ideas?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API.

Projects

Status: Need Triage

Development

Successfully merging this pull request may close these issues.

4 participants