GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: clarify use of Uint8Array for n-api by indutny · Pull Request #48742 · nodejs/node

Conversation

@indutny

@indutny indutny commented Jul 12, 2023

Copy link
Copy Markdown
Member

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to Node.js documentation. node-api Issues and PRs related to Node-API. labels Jul 12, 2023
@indutny
doc: clarify use of Uint8Array for n-api
d664cf3 
`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.
@indutny
indutny force-pushed the feature/n-api-uint8array branch from 3d03df1 to d664cf3 Compare July 12, 2023 00:45
lpinca
lpinca approved these changes Jul 12, 2023
View reviewed changes
@mhdawson

mhdawson commented Jul 12, 2023

Copy link
Copy Markdown
Member

@nodejs/node-api lets discuss in the next weekly meeting
indutny reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@legendecas

legendecas commented Jul 13, 2023

Copy link
Copy Markdown
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?
KevinEady reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mhdawson

mhdawson commented Jul 14, 2023

Copy link
Copy Markdown
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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@indutny-signal

indutny-signal commented Jul 17, 2023

Copy link
Copy Markdown

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 .
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@legendecas legendecas mentioned this pull request Jul 21, 2023
src: remove duplicate buffer info calls nodejs/node-addon-api#1354
Merged
@legendecas

legendecas commented Jul 24, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aymen94
aymen94 approved these changes Jul 24, 2023
View reviewed changes
@indutny-signal

indutny-signal commented Jul 24, 2023

Copy link
Copy Markdown

Amazing. Thank you!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
legendecas
legendecas reviewed Jul 26, 2023
View reviewed changes
Comment thread doc/api/n-api.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread doc/api/n-api.md
* `[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

@legendecas legendecas Jul 26, 2023

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@indutny indutny Jul 28, 2023

Copy link
Copy Markdown
Member Author

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mhdawson

mhdawson commented Jul 28, 2023

Copy link
Copy Markdown
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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
indutny and others added 2 commits July 28, 2023 09:21
@indutny @legendecas
Update doc/api/n-api.md
0bc66ea 
Co-authored-by: Chengzhong Wu <[email protected]>
@indutny
update
2fbc2f1
@indutny

indutny commented Jul 28, 2023

Copy link
Copy Markdown
Member Author

@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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gabrielschulhof
gabrielschulhof approved these changes Aug 18, 2023
View reviewed changes
@gabrielschulhof

gabrielschulhof commented Aug 18, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
indutny added 2 commits August 18, 2023 09:48
@indutny
format
24b6365
@indutny
fix lint
4ba8e6f
@indutny

indutny commented Aug 18, 2023

Copy link
Copy Markdown
Member Author

Done, sorry about that!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
legendecas
legendecas approved these changes Aug 21, 2023
View reviewed changes
@legendecas legendecas added commit-queue-squash PRs the Commit Queue should land as one squashed commit. commit-queue PRs queued for automated landing through the Commit Queue. labels Aug 21, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Aug 21, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 62b2cf3 into nodejs:main Aug 21, 2023
@nodejs-github-bot

nodejs-github-bot commented Aug 21, 2023

Copy link
Copy Markdown
Collaborator

Landed in 62b2cf3
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@indutny @UlisesGascon
doc: clarify use of Uint8Array for n-api
a22e0d9 
`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]>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
v20.7.0 proposal #49592
Merged
targos pushed a commit that referenced this pull request Nov 27, 2023
@indutny @targos
doc: clarify use of Uint8Array for n-api
f9c2a3f 
`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]>
@targos targos mentioned this pull request Nov 28, 2023
v18.19.0 release proposal #50953
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
doc: clarify use of Uint8Array for n-api
0c6297f 
`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]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
@sercher
doc: clarify use of Uint8Array for n-api
a4cea0d 
`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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes
@lpinca lpinca lpinca approved these changes
@legendecas legendecas legendecas approved these changes
+1 more reviewer
@aymen94 aymen94 aymen94 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

commit-queue-squash PRs the Commit Queue should land as one squashed commit. doc Issues and PRs related to Node.js documentation. node-api Issues and PRs related to Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@indutny @mhdawson @legendecas @indutny-signal @gabrielschulhof @nodejs-github-bot @lpinca @aymen94
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.