nodejs / abi-stable-node

Repository used by the Node-API team to manage work related to Node-API and node-addon-api
239 stars 47 forks source link

Add support for napi_create_boolean() #362

Closed nkreeger closed 5 years ago

nkreeger commented 5 years ago

I noticed that the N-API does not have support to create boolean types from C-code. There are methods for various integer types, floating points, and strings - but nothing for booleans. In order to surface a return value that is a true boolean it looks like I need to include something like:

Nan::New<v8::Boolean>(value);

I'm happy to contribute a PR - I just want to make sure there are no concerns about x-platform support (I'd think booleans are no big deal). We are heavy users of this API for TensorFlow.js Node bindings and a headless WebGL project.

mhdawson commented 5 years ago

I see this in: https://github.com/nodejs/node/blob/master/src/js_native_api.h

NAPI_EXTERN napi_status napi_get_boolean(napi_env env,
                                         bool value,
                                         napi_value* result);

which is what you'd use to create a boolean with the C API.

There is also support in node-addon-api: https://github.com/nodejs/node-addon-api/blob/master/doc/boolean.md.

So I think its already there unless I misunderstood.

DaAitch commented 5 years ago

Yeah I also think it's already there. Maybe it's a bit confusing, because it's not "create boolean" but "get boolean", like with undefined, simply because in JS they are constants.

gabrielschulhof commented 5 years ago

@nkreeger hopefully this has addressed your issue. Please feel free to reopen otherwise.

bpasero commented 5 years ago

jm2c: took me almost half day to figure this out. How is this not documented on https://nodejs.org/api/n-api.html, at least as a hint?

In general I have to say, https://nodejs.org/api/n-api.html lacks some serious examples of the most basic things like having a method with some values passed in and a result value returned back. I have a frustrating experience migrating to N-API :-|

mhdawson commented 5 years ago

@bpasero have you seen the examples in nodejs/node-addon-examples?

waruqi commented 6 months ago

I think napi_get_boolean should be rename to napi_create_boolean, or add a napi_create_boolean macro at least.