Review requested:

• @nodejs/n-api

Will need more time to review the content but the first thing is that all new APIs need to be added as experimental so they should be added in the section guarded by #ifdef NAPI_EXPERIMENTAL in js_native_api.h

@codebytere , one other question is if isFrozen and isSealed should go along with the freeze and seal or if there is some reason the freeze and seal are needed by native modules by the is methods won't be?

@codebytere some of the requirements for adding an API - https://github.com/nodejs/node/blob/master/doc/guides/adding-new-napi-api.md.

Of that list A new API addition should be simultaneously implemented in at least one other VM implementation of Node.js. and The API must be implemented in a Node.js implementation with an alternate VM. have not been enforced since Chakra core is no longer an option for implementation and we should probably update that.

@mhdawson i can definitely add isFrozen/isSealed - there's no specific reason i didn't!

@codebytere thanks for the quick response. I'll tag for discussion in the N-API team meeting this friday to see if there is any other feedback/concerns as well.

One other thing that would be good for that discussion is the use case that driving the addition. I'm guessing you need them and understanding that need will help the discussion.

You are also welcome to come to the meeting - from the calendar:

Node.js N-API team weekly meeting
Friday, October 2⋅11:00am – 12:00pm
Weekly on Friday
https://zoom.us/j/363665824

@mhdawson i should be there! what time zone is that? Also - the reason I initially didn't add isFrozen and/or isSealed i recall now is bc V8 doesn't really provide a good approach to that. They check this themselves by compiling some JS and running which doesn't necessarily feel like an approach we'd want to emulate.

A primary driver for this is passing objects across process boundaries (e.g between renderer & main) with more robust security guarantees.

@jschlight ty, bit of a suffering hour but i should make it 😆

@codebytere is there a reason why these APIs take a third parameter that just receives the second parameter back? Is there ever any chance that the object returned will be an object other than the object passed in?

If not, then I don't think we need the out parameter. I understand that it can be useful in JS to return the same object that was passed in so one can compose calls, but on the native side we can never chain N-API calls because they all return napi_status. Our C++ wrappers can implement this composably even without the out parameter.

As an existing example, napi_detach_arraybuffer also modifies the napi_value containing an array buffer without returning it. Since the detach API implements directly an abstract operation as defined in the spec, and since both freeze and seal look like convenience methods around the SetIntegrityLevel abstract operation, I think it's OK to drop the out parameter, because then we're basically implementing the abstract operation, but without the need to introduce a corresponding enum of { napi_frozen, napi_sealed }.

Edit: linkified some of the text.

Also, fewer parameters help keep the stack lean for low-memory implementations like IoT.js.

@gabrielschulhof sounds good to me! There wasn't a specific reason I did it this way to begin with beyond that I was seeking to emulate conventions for similar functions

@gabrielschulhof i think that should do it - let me know if i missed anything!

@gabrielschulhof sorry for the churn 🙃

turns out trying to context switch extremely fast between this and doing an LTS release is not the greatest idea in the world - linting should finally be good now.

@codebytere NP 🙂

CI: https://ci.nodejs.org/job/node-test-pull-request/33436/

CI: https://ci.nodejs.org/job/node-test-pull-request/33445/

Landed in 19f1451