GeezerVison

As the comment at the beginning of this function says:

   // XXX: If you add an option here, please also add it to doc/node.1 and
   // doc/api/cli.md

I think this would be our only option that has a [=yes|no] format (apart from configure flags, I guess). Is there a reason for that?

If somebody really wants to avoid loading N-API modules even after we've eventually set the default to "yes", thus rendering the use of the command line option unnecessary, they will be unable to do so unless we give them the option to say "no".

Of course, if the consensus is that we do not need to provide users with a way to explicitly disable the support for N-API modules, then the option can be reducded to --napi-modules.

If somebody really wants to avoid loading N-API modules even after we've eventually set the default to "yes"

I'm not sure that's something that we'll need to support at that point. If N-API is successful it should become the normal way of writing native modules, so it wouldn't really make sense to disable it.

I agree. Once non-experimental we would would most likely just remove the flag. I think at this point we could just likely make it --napi-modules with no options.

I personally would prefer --napi-modules and then once it goes live we could add an optional =false or something to force-disable?

@Fishrock123 can you explain why you (or someone) might want to disable the feature after it is out of experimental status?

No, actually. I really do not know, but I would prefer to not have =yes.

Resolved by 87c42e7

why no napi_create_string_latin1? That's a fairly important type for string conversion.

Good feedback, we should consider adding it. I opened nodejs/abi-stable-node#179 to track this.

Note missing APIs should probably not block this PR unless it is something critical. The goal for this PR is to establish an initial set of APIs that provide enough functionality for native module developers to begin experimenting and give us feedback. We have plans to expand on these APIs in the coming months, before the feature graduates out of experimental status.

As @jasongin mentioned, we don't believe missing APIs should be a blocker. There are a few others we have in our plans to add which we will prioritized based on feedback from people using the API.

Why not use v8::Object::GetPrototype() here?

Did you mean to put this comment somewhere else?

I'm guessing you might be referring to the access of the prototype property in napi_instanceof() here. In that case, it is really a function's prototype property that is needed, that is different from the __proto__ property that is returned by GetPrototype(). The "instanceof" logic had to be implemented in this way rather than using v8::FunctionTemplate::HasInstance() because N-API does not expose the V8-specific concept of function or object templates, nor does it track them internally, so the FunctionTemplate that was used to create the function passed to napi_instanceof() is not available at that point.

(There is also a napi_get_prototype() API that simply calls v8::Object::GetPrototype().)

ah yup. see what you're saying.

Looks like NAP_PREAMBLE is used in many functions that don't execute javascript. what's the rational for using TryCatch when no JS is called and the v8 Maybe APIs are used?

We have made an effort to avoid using NAPI_PREAMBLE for functions which don't execute JavaScript (see napi_is_error for example) but we haven't done that exhaustively. I think we took the more conservative approach of including TryCatch when we weren't sure if it was strictly required or not because the guidance seems to be that using TryCatch was cheap enough that this would be fine. If we're sure no JavaScript is getting executed, removing the NAPI_PREAMBLE seems like the right thing to do.

Also we know we need to remove the NAPI_PREAMBLE from at least a few of the functions, for https://github.com/nodejs/abi-stable-node/issues/122 

Thanks for the explanation. To clarify, I don't see this as a blocker.

Not sure I can appreciate this auto-magical deletion of the the pointer. One reason is because there's no way to know whether the pointer was made via new or malloc. Even in node_base_object.h there's an explicit call to make the object weak.

Edit: missed the "self-deleting" part. Nothing to see here.

It feels counter intuitive to go to such lengths to make the public API be C, but then use delete on a user-supplied pointer.

The delete here is deleting our Reference instance - not the user-supplied pointer. We aren't (hopefully) doing anything which would preclude a module written in C.

There is another delete reference in napi_delete_reference which operates on a user-supplied pointer (the napi_ref argument). We are expecting the user to hand us back a Reference* which we allocated (via new) in napi_create_reference. This Reference class acts as a container for us to do ref-counting.

If the user code calls napi_delete_reference with any random pointer, I guess behavior would be undefined. We would try to use ~Reference which might crash. But this is true of all the API which take an opaque pointer and cast it to an engine-specific type. We have a similar pattern for napi_close_handle_scope and napi_close_escapable_handle_scope.

Yeah, the code looks correct. I’d say for these cases it would be nice to turn the constructor private and re-expose it as a static method (New() or something), so that it’s more obvious that the instance was allocated using new. (Plus, imho it’s good practice to try and keep corresponding new and delete calls reasonably close to each other)

@trevnorris it sounds like we should have a test where an addon is implemented in C, just to keep us honest.

fb8ced4 adds New() and Delete() methods on the Reference class for clarity.

my bad. misunderstood what was going on here.

This error message could be a bit more specific than the Node.js API, I’d say. Also, maybe display the flag name here directly?

Resolved by b129624

I think this would be our only option that has a [=yes|no] format (apart from configure flags, I guess). Is there a reason for that?

I’m not a fan of printing warnings like these… if the user opted into the feature, then they opted in because they know what they were asking for.

CTC has specifically requested command-line enablement along with a warning message for features with experimental status. Tracing does the same thing.

Is there a link somewhere to that decision?

I don't quite understand the justification for a printed warning for a feature that can't even be used without a flag. If you're using the flag, you wanted the feature and are quite aware that it is experimental. Do any V8 features do this?

VM Summit 3 meeting notes has this
For 8.0, include an opt-in runtime flag, show experimental warning when a NAPI module is loaded.

If consensus is that documentation of experimental status is sufficient and this warning message is not necessary, we can take it out. I don't recall anyone feeling very strongly about this at the VM summit.

@jasongin I’d say take it out, and we can re-visit that in the unlikely case anybody complains.

I think the main motivation was to ensure that anybody turning it on is fully aware of the status and to be consistent with what we have done for other experimental features.

static_assert for all of these

Resolved by 116bd19

Yeah, awkward is a good description, I’m not even sure these would work if we didn’t compile with -fno-strict-aliasing. Is there anything speaking against a simple return reinterpret_cast<napi_value>(*local), at least for the V8-to-napi_value conversion?

return reinterpret_cast<napi_value>(*local)

Yes, I agree that's correct and more straightforward. I'll update it.

(This is some of the oldest code in the N-API project; I'm guessing whoever wrote it was initially trying to avoid using reinterpret_cast<>, though the union accomplishes the same thing in a roundabout way.)

Resolved by 116bd19

It seems like int is a V8 implementation detail here too, size_t is probably better for string lengths (also here and below)?

Yes, good point.

Resolved by 9e3ab83

It seems to me like int is a V8 implementation detail, size_t might better for argument counts (here and elsewhere)?

Resolved by 9e3ab83

(re: my comment above … maybe also make the Reference destructor private and add a Delete static? I know it might seem a bit superfluous but I think it would really help with readability)

Resolved by fb8ced4

v8::Handle → v8::Local (also something for search & replace… 😄)

Resolved by 116bd19

If we want to support an instanceof implementation that doesn’t conform to ES6 (in that it isn’t based on [Symbol.hasInstance]()), could/should we call it something else? Alternatively, could this wrapper just manually call out to constructor[Symbol.hasInstance](object)?

This does conform to ES6 instanceof. There are extensive tests at test/addons-napi/test_instanceof that re-run all of V8's instanceof tests using this API.

@addaleax I implemented napi_instanceof() this way because I didn't have access to the implementation of the operator. However, if v8 provides access to an implementation that does not require retaining/acquiring the FunctionTemplate then I'd be happy to change this API. Additionally, it looks like Symbol.hasInstance() allows users to customize the behaviour of the instanceof operator, so we absolutely have to use Symbol.hasInstance() when available.

@jasongin Yeah, it matches OrdinaryHasInstance(). I’m good with just naming it that.

Otoh, it should really not be a problem for V8 to expose InstanceOf() and OrdinaryHasInstance() directly…