-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
node-api: add napi_create_buffer_from_arraybuffer method #54505
Conversation
Review requested:
|
6a34c19
to
32529a6
Compare
I made |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is actually creating a node buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but would like another review from someone more familiar with this part of the code.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54505 +/- ##
==========================================
+ Coverage 88.24% 88.41% +0.16%
==========================================
Files 651 652 +1
Lines 183856 186628 +2772
Branches 35853 36065 +212
==========================================
+ Hits 162251 165001 +2750
+ Misses 14896 14885 -11
- Partials 6709 6742 +33
|
@ronag would you mind sharing a use case for this method? Does https://nodejs.org/api/n-api.html#napi_create_typedarray work for the use case? We talked about this in Node-API meeting, a new node-api can not be easily changed or deprecated, a concrete use case would really help us understand the necessity to add the new API and if the API interface sufficient. |
It doesn't work if I need to return node buffers in the js api. I guess I could change the prototype in js but that seems a bit ugly. |
@nodejs/node-api |
I suspect you are running gyp for the installed Node.js instance instead of the one built on your machine. This is why it cannot find the new function that is added in your PR. I typically build the tests on Windows with vcbuild.bat and then iterate on them inside of Visual Studio. To run them using the built Node.js instance I usually do |
cad379f
to
5224e25
Compare
this worked very well thank you very much |
5224e25
to
8b41532
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The CI is passed and we have two approvals. @gabrielschulhof would you mind taking a last review? thanks |
Landed in fdf838a |
PR-URL: nodejs#54505 Fixes: nodejs#54440 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
PR-URL: nodejs#54505 Fixes: nodejs#54440 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
PR-URL: #54505 Fixes: #54440 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Vladimir Morozov <[email protected]>
introduces a new N-API method
napi_create_buffer_from_arraybuffer
that allows the creation of a Buffer from an ArrayBuffer. This new method provides functionality similar tonapi_create_typedarray
, enabling developers to specify a byte offset and length to create a Buffer from a portion of the ArrayBuffer. This feature addresses the limitation where it was previously not possible to create a Buffer directly from an ArrayBuffer using N-API.Closes: #54440