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

Add ArrayBuffer::Detach() and ::IsDetached() #659

Closed
wants to merge 2 commits into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Jan 26, 2020

I don't know what the proper guard is, seems like NAPI_EXPERIMENTAL and NAPI_VERSION are not good enough.

Refs: nodejs/node#29768
Refs: nodejs/node#30613

@tniessen tniessen force-pushed the add-detach-functions branch from 8d7c515 to bcd03dd Compare January 26, 2020 05:37
@legendecas
Copy link
Member

legendecas commented Jan 26, 2020

NODE_MAJOR_VERSION can be used in tests to guards on Node.js major version so that no-back-ported Node.js release can work on CI.

Note that NODE_MAJOR_VERSION shall only be used in tests but not in napi.h and napi-inl.h.

@mhdawson
Copy link
Member

@tniessen do you need any more info to progress this?

@mhdawson
Copy link
Member

mhdawson commented Jun 9, 2020

@tniessen are you still planning to work on this?

@tniessen
Copy link
Member Author

Yes, sorry @mhdawson, I do intend to work on this. I didn't find much time recently.

@mhdawson
Copy link
Member

@tniessen np, just wanted to check in as I was doing some cleanup on the issues.

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

This needs a rebase, but otherwise LGTM.

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

Oh, wait, and documentation 🙂

@mhdawson
Copy link
Member

@tniessen ping.

@tniessen tniessen force-pushed the add-detach-functions branch from bcd03dd to ad9f5b0 Compare November 5, 2020 14:12
@tniessen tniessen marked this pull request as ready for review November 5, 2020 14:18
@tniessen
Copy link
Member Author

tniessen commented Nov 5, 2020

Sorry it took me so long to get back to this!

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

mhdawson commented Nov 5, 2020

Looks good landing

mhdawson pushed a commit that referenced this pull request Nov 5, 2020
@mhdawson
Copy link
Member

mhdawson commented Nov 5, 2020

Landed as c9563ca

@mhdawson mhdawson closed this Nov 5, 2020
@tniessen tniessen deleted the add-detach-functions branch November 5, 2020 21:23
Superlokkus pushed a commit to Superlokkus/node-addon-api that referenced this pull request Nov 20, 2020
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants