Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
doc: clarify use of Uint8Array for n-api#48742
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
`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.
3d03df1 to d664cf3Comparemhdawson commented Jul 12, 2023
@nodejs/node-api lets discuss in the next weekly meeting |
legendecas commented Jul 13, 2023
In fact, Are there any use cases that prefer |
mhdawson commented Jul 14, 2023
@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 commented Jul 17, 2023
Great news! Thanks!
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 commented Jul 24, 2023 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
I've created a PR to remove usages on I think we can document that |
indutny-signal commented Jul 24, 2023
Amazing. Thank you! |
Uh oh!
There was an error while loading. Please reload this page.
| * `[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 |
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.
napi_is_typedarray should be preferred if the caller needs to check if the value is a Uint8Array.
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.
Committed! Thanks!
mhdawson commented Jul 28, 2023
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? |
Co-authored-by: Chengzhong Wu <[email protected]>
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 commented Aug 18, 2023
@indutny could you please fix the lint check? We can then land this. |
indutny commented Aug 18, 2023
Done, sorry about that! |
nodejs-github-bot commented Aug 21, 2023
Landed in 62b2cf3 |
`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 <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
`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 <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
`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 <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
`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 <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
napi_get_buffer_infoalways supported receivingUint8Arrayas avalueargument becausenode::Bufferis a subclass ofUint8Arrayand 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 thevalueargument.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 betweennode::BufferandUint8Arrayso it'd be a breaking change if we started to bail onUint8Arrayanyway. Let's mark it as supported now to signify existing code as compliant to official APIs!