Skip to content
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

doc: clarify use of Uint8Array for n-api #48742

Merged
merged 5 commits into from
Aug 21, 2023

Conversation

indutny
Copy link
Member

@indutny indutny commented Jul 12, 2023

napi_get_buffer_info always supported receiving Uint8Array as a value argument because node::Buffer is a subclass of Uint8Array and the underlying V8 APIs don't distinguish between two. With this change we mark both types as supported by the API so that the user code doesn't have to unknowingly use oficially unsupported type of the value argument.


Hello!

I think it'd be great to officially mark Uint8Arrays as supported since it is very likely that there is existing code in the wild that relies on this. The N-API never distinguished between node::Buffer and Uint8Array so it'd be a breaking change if we started to bail on Uint8Array anyway. Let's mark it as supported now to signify existing code as compliant to official APIs!

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API. labels Jul 12, 2023
`napi_get_buffer_info` always supported receiving `Uint8Array` as a
`value` argument because `node::Buffer` is a subclass of `Uint8Array`
and the underlying V8 APIs don't distinguish between two. With this
change we mark both types as supported by the API so that the user code
doesn't have to unknowingly use oficially unsupported type of the
`value` argument.
@mhdawson
Copy link
Member

@nodejs/node-api lets discuss in the next weekly meeting

@legendecas
Copy link
Member

In fact, napi_get_buffer_info supports any array buffer view as its argument. Compared to napi_get_typedarray_info, napi_get_buffer_info returns the byte length while napi_get_typedarray_info returns the element length (which should be identical when the bytes-per-element is 1).

Are there any use cases that prefer napi_get_buffer_info to napi_get_typedarray_info on typed arrays that are not node::Buffer?

@mhdawson
Copy link
Member

@indutny we discussed a bit in the meeting today. Just waiting on the answer from you on:

Are there any use cases that prefer napi_get_buffer_info to napi_get_typedarray_info on typed arrays that are not node::Buffer?

@indutny-signal
Copy link

Great news! Thanks!

Are there any use cases that prefer napi_get_buffer_info to napi_get_typedarray_info on typed arrays that are not node::Buffer?

Our use case comes through Neon wrapper for Rust addons which supports either JSArrayBuffer or JSBuffer: https://docs.rs/neon/latest/neon/types/buffer/trait.TypedArray.html .

@legendecas
Copy link
Member

legendecas commented Jul 24, 2023

I've created a PR to remove usages on napi_get_buffer_info in node-addon-api c++ headers: nodejs/node-addon-api#1354. It can be replaced with napi_get_typedarray_info as inheriting Napi::Uint8Array.

I think we can document that napi_get_typedarray_info can be used instead of napi_get_buffer_info.

@indutny-signal
Copy link

Amazing. Thank you!

doc/api/n-api.md Show resolved Hide resolved
@@ -3879,8 +3880,8 @@ napi_status napi_is_buffer(napi_env env, napi_value value, bool* result)

* `[in] env`: The environment that the API is invoked under.
* `[in] value`: The JavaScript value to check.
* `[out] result`: Whether the given `napi_value` represents a `node::Buffer`
object.
* `[out] result`: Whether the given `napi_value` represents a `node::Buffer` or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

napi_is_typedarray should be preferred if the caller needs to check if the value is a Uint8Array.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Committed! Thanks!

@mhdawson
Copy link
Member

From my understading in the discussion in the Node-api team meeting today the preferred path forward is that this PR be updated to suggest that napi_get_typedarray_info be used instead for uint8arrays instead of indicating that napi_get_buffer_info supports them. This is because we'd prefer that people working with uint8arrays use napi_get_typearray_info versus encouraging more use of napi_get_buffer_info.

@indutny are you ok with updating the PR along those lines?

indutny and others added 2 commits July 28, 2023 09:21
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
@indutny
Copy link
Member Author

indutny commented Jul 28, 2023

@mhdawson totally! If this is the direction you are more comfortable with - I'm happy to go with it. It definitely hurts most to have two different APIs for objects that are the same for most practical uses. Thank you!

@gabrielschulhof
Copy link
Contributor

@indutny could you please fix the lint check? We can then land this.

@indutny
Copy link
Member Author

indutny commented Aug 18, 2023

Done, sorry about that!

@legendecas legendecas added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 21, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 21, 2023
@nodejs-github-bot nodejs-github-bot merged commit 62b2cf3 into nodejs:main Aug 21, 2023
20 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 62b2cf3

UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
`napi_get_buffer_info` always supported receiving `Uint8Array` as a
`value` argument because `node::Buffer` is a subclass of `Uint8Array`
and the underlying V8 APIs don't distinguish between two. With this
change we mark both types as supported by the API so that the user code
doesn't have to unknowingly use oficially unsupported type of the
`value` argument.

PR-URL: #48742
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
targos pushed a commit that referenced this pull request Nov 27, 2023
`napi_get_buffer_info` always supported receiving `Uint8Array` as a
`value` argument because `node::Buffer` is a subclass of `Uint8Array`
and the underlying V8 APIs don't distinguish between two. With this
change we mark both types as supported by the API so that the user code
doesn't have to unknowingly use oficially unsupported type of the
`value` argument.

PR-URL: #48742
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
`napi_get_buffer_info` always supported receiving `Uint8Array` as a
`value` argument because `node::Buffer` is a subclass of `Uint8Array`
and the underlying V8 APIs don't distinguish between two. With this
change we mark both types as supported by the API so that the user code
doesn't have to unknowingly use oficially unsupported type of the
`value` argument.

PR-URL: nodejs/node#48742
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
`napi_get_buffer_info` always supported receiving `Uint8Array` as a
`value` argument because `node::Buffer` is a subclass of `Uint8Array`
and the underlying V8 APIs don't distinguish between two. With this
change we mark both types as supported by the API so that the user code
doesn't have to unknowingly use oficially unsupported type of the
`value` argument.

PR-URL: nodejs/node#48742
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants