Function and FunctionReference documentation #299
Conversation
doc/function.md
Outdated
| [C++ wrapper classes for the ABI-stable C APIs for Node.js](https://nodejs.github.io/node-addon-api/) | ||
| The **Function** class is a representation of the JavaScript function. A function | ||
| is a JavaScript procedure, a set of statements that performs a task or calculates | ||
| a value. This class provides some methods to create and exectue a function. |
There was a problem hiding this comment.
How about
+The Function class is a representation of the JavaScript function. A function
+is a set of statements that performs a task or calculates
+a value. This class provides some methods to create and execute a function.
doc/function.md
Outdated
| - std::string (encoded using UTF-8) | ||
| - std::u16string | ||
| - napi::Value | ||
| - napi_value |
There was a problem hiding this comment.
I don't think this is right? It likely needs to be a napi_value which is handle for a JavaScript function.
doc/function.md
Outdated
| ``` | ||
|
|
||
| - `[in] env`: The `napi_env` environment in which to construct the Function object. | ||
| - `[in] cb`: Object that implement the operator **()** accepting a `const CallbackInfo&` |
There was a problem hiding this comment.
Should this just say it needs to be an instance of Callable ?
There was a problem hiding this comment.
is it good for you: Object that implements Callable.?
doc/function.md
Outdated
|
|
||
| - `[in] env`: The `napi_env` environment in which to construct the Function object. | ||
| - `[in] cb`: Object that implement the operator **()** accepting a `const CallbackInfo&` | ||
| and returns either void or Value. |
There was a problem hiding this comment.
Ditto regarding "void or Value".
doc/function.md
Outdated
| ``` | ||
|
|
||
| - `[in] recv`: The `this` object passed to the called function. | ||
| - `[in] args`: List of JavaScript values as `napi_value` representing the |
There was a problem hiding this comment.
Maybe
Vector of JavaScript values as napi_value representing the
doc/function.md
Outdated
| - `[in] args`: Initializer list of JavaScript values as `napi_value` representing | ||
| the arguments of the function. | ||
|
|
||
| Returns a Value representing the JavaScript object returned by the function. |
There was a problem hiding this comment.
I think we need to have somewhere, either in the intro or in the description of the MakeCallback function where you should use Call versus MakeCallback. You should use Call if there is already a JavaScript function on the stack (for example when running a native method called from JavaScript), and only use MakeCallback in special cases like running OnError, OnOk in AsyncWorker which don't have an existing JavaScript function on the stack.
doc/function.md
Outdated
|
|
||
| Returns a Value representing the JavaScript object returned by the function. | ||
|
|
||
| ### New |
There was a problem hiding this comment.
I think all the new methods should be together.
doc/function_reference.md
Outdated
|
|
||
| ### Constructor | ||
|
|
||
| Creates a new |
doc/function_reference.md
Outdated
| FunctionReference(); | ||
| ``` | ||
|
|
||
| - `[in] Env`: The environment in which to construct the |
There was a problem hiding this comment.
partial line. I see that in a bunch of places throughout.
doc/function_reference.md
Outdated
|
|
||
| - `[in] Env`: The environment in which to construct the | ||
|
|
||
| Returns a new |
There was a problem hiding this comment.
Function reference overall looks like its missing the substitution. For example "Returns a new" should probably be "Returns a new FunctionReference"
doc/function_reference.md
Outdated
|
|
||
| - `[in] Env`: The environment in which to construct the | ||
|
|
||
| Returns a new |
There was a problem hiding this comment.
I think we also need some explanation of why you use a FunctionReference instead of a Function and when.
|
@NickNaso just wondering if you are going to be able to update this week? |
doc/function.md
Outdated
|
|
||
| ### Constructor | ||
|
|
||
| Creates a new instance of `Function` object. |
There was a problem hiding this comment.
of the Function object.
doc/function.md
Outdated
|
|
||
| ### New | ||
|
|
||
| Creates instance of a `Function` object. |
There was a problem hiding this comment.
Creates an instance
doc/function.md
Outdated
| ``` | ||
|
|
||
| - `[in] env`: The `napi_env` environment in which to construct the Function object. | ||
| - `[in] cb`: Object that implement the operator **()** accepting a `const CallbackInfo&` |
doc/function.md
Outdated
|
|
||
| - `[in] env`: The `napi_env` environment in which to construct the Function object. | ||
| - `[in] cb`: Object that implement the operator **()** accepting a `const CallbackInfo&` | ||
| and returns either void or Value. |
There was a problem hiding this comment.
Perhaps change this to "void or a Napi::Value".
doc/function.md
Outdated
| - `[in] data`: User-provided data context. This will be passed back into the | ||
| function when invoked later. | ||
|
|
||
| Returns instance of a `Function` object. |
doc/function.md
Outdated
|
|
||
| - `[in] env`: The `napi_env` environment in which to construct the Function object. | ||
| - `[in] cb`: Object that implement the operator **()** accepting a `const CallbackInfo&` | ||
| and returns either void or Value. |
There was a problem hiding this comment.
Ditto regarding "void or Value".
doc/function.md
Outdated
| - `[in] data`: User-provided data context. This will be passed back into the | ||
| function when invoked later. | ||
|
|
||
| Returns instance of a `Function` object. |
doc/function.md
Outdated
| - `[in] args`: Initializer list of JavaScript values as `napi_value` representing | ||
| the arguments of the function. | ||
|
|
||
| Returns a Value representing the JavaScript object returned by the function. |
There was a problem hiding this comment.
"Returns a Napi::Value representing the JavaScript value..." we shouldn't say "JavaScript object" because that has a special meaning.
There was a problem hiding this comment.
You're right. Sorry.
Done.
There was a problem hiding this comment.
NP 🙂 Weird though ... github hasn't updated to show that 😕
There was a problem hiding this comment.
Oh, I see - you fixed the ones below, but you missed this one. NP, but please say "`Napi::Value`" rather than merely "`Value`" – that is, please use the name space with the type name.
doc/function.md
Outdated
| - `[in] args`: List of JavaScript values as `napi_value` representing the | ||
| arguments of the function. | ||
|
|
||
| Returns a Value representing the JavaScript object returned by the function. |
doc/function.md
Outdated
| - `[in] args`: Array of JavaScript values as `napi_value` representing the | ||
| arguments of the function. | ||
|
|
||
| Returns a Value representing the JavaScript object returned by the function. |
There was a problem hiding this comment.
Ditto also for all instances to follow.
doc/function.md
Outdated
| arguments of the function. | ||
|
|
||
| Returns a Value representing the JavaScript object returned by the function. | ||
|
|
There was a problem hiding this comment.
I'm surprised we don't have overloads which accept an array of Napi::Value items 😕
There was a problem hiding this comment.
So do I, what do you think if I open an issue about that? Just to remember this and maybe after the documentation I can try to work on that.
There was a problem hiding this comment.
From what I recall, these were my reasons for not adding such an overload:
- Function arguments are almost always specified inline with the function call, not prepared ahead of time as an array.
- Such an overload would be less efficient. The underlying
napi_call_function()takes an array ofnapi_value. So converting from an array ofNapi::Valueto that would require allocating a new array and copying each of the (non-contiguous)napi_valueelements into the array. - The implicit conversion
operator napi_valuemeans it's usually just as convenient to create avectororstd::initializer_listofnapi_valueinstead ofNapi::Value. And it's more efficient because it uses less memory by not needlessly duplicating theenvvalue.
But I can understand why the lack of such an overload could be unexpected, given the pattern of most other wrapper functions that take Napi::Value types directly.
There was a problem hiding this comment.
Good points. Thanks for sharing this.
|
@mhdawson I'm starting working now. |
doc/function.md
Outdated
| Calls a Javascript function from a native add-on. | ||
|
|
||
| ```cpp | ||
| Value Call(const std::initializer_list<napi_value>& args) const; |
There was a problem hiding this comment.
This needs an example, because some people may less familiar with syntax for initializer_list. Especially since this is the most convenient and efficient overload to use for most typical cases.
NickNaso
changed the title
|
Just tried to address all the things issued after first review. |
|
@mhdawson They are both good to me. Thanks for your help. |
|
@NickNaso thanks, will leave until tomorrow to let @gabrielschulhof time to take a look and then will go ahead and land. |
doc/function.md
Outdated
| You are reading a draft of the next documentation and it's in continuous update so | ||
| if you don't find what you need please refer to: | ||
| [C++ wrapper classes for the ABI-stable C APIs for Node.js](https://nodejs.github.io/node-addon-api/) | ||
| The **Function** class provides a set of methods to create a function object in |
There was a problem hiding this comment.
"... a set of methods to createfor creating ..."
doc/function.md
Outdated
| [C++ wrapper classes for the ABI-stable C APIs for Node.js](https://nodejs.github.io/node-addon-api/) | ||
| The **Function** class provides a set of methods to create a function object in | ||
| native code that can later be called from JavaScript. The created function is not | ||
| automatically visible from JavaScript, instead it needs to be part of the add-on's |
There was a problem hiding this comment.
This is a run-on sentance. Let's have "... from JavaScript. Instead it ..."
doc/function.md
Outdated
| The **Function** class provides a set of methods to create a function object in | ||
| native code that can later be called from JavaScript. The created function is not | ||
| automatically visible from JavaScript, instead it needs to be part of the add-on's | ||
| module exports or be returned by one of the modules exported functions. |
There was a problem hiding this comment.
"module's" instead of "modules"
doc/function.md
Outdated
| module exports or be returned by one of the modules exported functions. | ||
|
|
||
| In addition the `Function` class also provides methods that can be used to call | ||
| functions that were created in JavaScript and passed to the native. |
There was a problem hiding this comment.
"... to the native add-on", perhaps?
doc/function.md
Outdated
| addon.fn(); | ||
| ``` | ||
|
|
||
| The `Function` class allows to call a JavaScript function object from a native |
There was a problem hiding this comment.
Maybe we should rephrase this as "With the Function class it is possible to call..."
doc/function_reference.md
Outdated
| if you don't find what you need please refer to: | ||
| [C++ wrapper classes for the ABI-stable C APIs for Node.js](https://nodejs.github.io/node-addon-api/) | ||
| FunctionReference is a subclass of [Reference](reference.md), and is equivalent to | ||
| an instance of `Reference<Function>`. This means that a FunctionReference holds a |
There was a problem hiding this comment.
"FunctionReference" should be in backticks.
doc/function_reference.md
Outdated
| [C++ wrapper classes for the ABI-stable C APIs for Node.js](https://nodejs.github.io/node-addon-api/) | ||
| FunctionReference is a subclass of [Reference](reference.md), and is equivalent to | ||
| an instance of `Reference<Function>`. This means that a FunctionReference holds a | ||
| [Function](function.md), and a count of the number of references to that Function. |
There was a problem hiding this comment.
Backticks around "Function" (both in the link and at the end of the line).
doc/function_reference.md
Outdated
| FunctionReference is a subclass of [Reference](reference.md), and is equivalent to | ||
| an instance of `Reference<Function>`. This means that a FunctionReference holds a | ||
| [Function](function.md), and a count of the number of references to that Function. | ||
| When the count is greater than 0, a FunctionReference is not eligible for garbage |
There was a problem hiding this comment.
Backticks around FunctionReference please!
There was a problem hiding this comment.
In general, please make another pass and add backticks around type names etc.!
doc/function_reference.md
Outdated
| ### New | ||
|
|
||
| Creates a new JavaScript value from one that represents the constructor for the | ||
| object. |
There was a problem hiding this comment.
Perhaps "Constructs a new instance by calling the constructor held by this reference."?
doc/function_reference.md
Outdated
| - `[in] args`: Vector of JavaScript values as napi_value representing the | ||
| arguments of the constructor function. | ||
|
|
||
| Returns a new JavaScript object. |
|
@gabrielschulhof Could you take a look at my last update? Thanks :-) |
doc/function.md
Outdated
| module exports or be returned by one of the module's exported functions. | ||
|
|
||
| In addition the `Function` class also provides methods that can be used to call | ||
| functions that were created in JavaScript and passed to the native add-on. |
There was a problem hiding this comment.
nit: two spaces between "native" and "add-on"
doc/function.md
Outdated
| With the `Function` class it is possible to call a JavaScript function object | ||
| from a native add-on with two different methods: `Call` and `MakeCallback`. | ||
| The API of these two methods is very similar, but they are used in different | ||
| context. The `MakeCallback` method is used to call from native code back into |
doc/function.md
Outdated
| ``` | ||
|
|
||
| - `[in] env`: The `napi_env` environment in which to construct the Value object. | ||
| - `[in] value`: The `napi_value` which is handle for a JavaScript function. |
There was a problem hiding this comment.
"...which is a handle..."
doc/function.md
Outdated
| static Function New(napi_env env, Callable cb, const char* utf8name = nullptr, void* data = nullptr); | ||
| ``` | ||
|
|
||
| - `[in] env`: The `napi_env` environment in which to construct the Function object. |
There was a problem hiding this comment.
Please put backticks around "Function"
doc/function_reference.md
Outdated
| if you don't find what you need please refer to: | ||
| [C++ wrapper classes for the ABI-stable C APIs for Node.js](https://nodejs.github.io/node-addon-api/) | ||
| `FunctionReference` is a subclass of [Reference](reference.md), and is equivalent to | ||
| an instance of `Reference<Function>`. This means that a FunctionReference holds a |
There was a problem hiding this comment.
Please put backticks around "FunctionReference"!
doc/function_reference.md
Outdated
| When the count is greater than 0, a FunctionReference is not eligible for garbage | ||
| collection. This ensures that the `Function` will remain accessible, even if the | ||
| original reference to it is no longer available. | ||
| `FunctionReference` allows the referenced JavaScript function object to be called |
There was a problem hiding this comment.
Two spaces between "object" and "to".
doc/function_reference.md
Outdated
| ### Weak | ||
|
|
||
| Creates a "weak" reference to the value, in that the initial count of number of | ||
| references is set to 0. |
There was a problem hiding this comment.
I think it's sufficient to say "in that the initial reference count is set to 0."
doc/function_reference.md
Outdated
| ### Persistent | ||
|
|
||
| Creates a "persistent" reference to the value, in that the initial count of | ||
| number of references is set to 1. |
There was a problem hiding this comment.
Ditto re "reference count".
doc/function_reference.md
Outdated
| Napi::Object New(const std::initializer_list<napi_value>& args) const; | ||
| ``` | ||
|
|
||
| - `[in] args`: Initializer list of JavaScript values as napi_value representing |
There was a problem hiding this comment.
Backticks around "napi_value", please!
doc/function_reference.md
Outdated
| Napi::Object New(const std::vector<napi_value>& args) const; | ||
| ``` | ||
|
|
||
| - `[in] args`: Vector of JavaScript values as napi_value representing the |
There was a problem hiding this comment.
Backticks around "napi_value", please!
|
@gabrielschulhof I think this is waiting for you now. |
|
I actually have to take a few hours before I can review. BRB 🙂 |
doc/function.md
Outdated
| Function(napi_env env, napi_value value); | ||
| ``` | ||
|
|
||
| - `[in] env`: The `napi_env` environment in which to construct the Value object. |
There was a problem hiding this comment.
"... the Function object." (with backticks around "Function").
doc/function.md
Outdated
| static Function New(napi_env env, Callable cb, const std::string& utf8name, void* data = nullptr); | ||
| ``` | ||
|
|
||
| - `[in] env`: The `napi_env` environment in which to construct the Function object. |
There was a problem hiding this comment.
Backticks around "Function"
doc/function.md
Outdated
| - `[in] env`: The `napi_env` environment in which to construct the Value object. | ||
| - `[in] value`: The `napi_value` which is a handle for a JavaScript function. | ||
|
|
||
| Returns a non-empty `Function` instance. |
There was a problem hiding this comment.
We should really be consistent about whether we say Napi::Value, Napi::Function, Napi::FunctionReference, etc. or Value, Function, or FunctionReference. IMO we should always add the name space in front of the class name. @mhdawson what do you think?
There was a problem hiding this comment.
Ok for me. Is it ok for you @mhdawson ?
doc/function_reference.md
Outdated
| You are reading a draft of the next documentation and it's in continuous update so | ||
| if you don't find what you need please refer to: | ||
| [C++ wrapper classes for the ABI-stable C APIs for Node.js](https://nodejs.github.io/node-addon-api/) | ||
| `FunctionReference` is a subclass of [Reference](reference.md), and is equivalent to |
There was a problem hiding this comment.
Here too, and in all cases where we use class names in the documentation, we should add the name space explicitly – that is, Napi::FunctionReference and Napi::Reference in this case. Additionally, "Reference" should be in backquotes.
There was a problem hiding this comment.
@NickNaso the word "Reference" inside the square bracket also needs backticks.
doc/function_reference.md
Outdated
| instead of `MakeCallback` and vice-versa. | ||
|
|
||
| The `FunctionReference` class inherits its behavior from the `Reference` class | ||
| (for more info see: [Reference](reference.md)). |
There was a problem hiding this comment.
Backticks around "Reference"
doc/function_reference.md
Outdated
| ``` | ||
|
|
||
| - `[in] env`: The environment in which to construct the FunctionReference object. | ||
| - `[in] ref`: The N-API reference to be held by the FunctionReference. |
There was a problem hiding this comment.
Backticks around "FunctionReference".
|
@mhdawson shall we go with full-name-space notation like |
|
I agree that full-name-space would be best. |
|
ok, lets land and then update to always use the name-space |
PR-URL: #299 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
|
Landed as 757eb1f |
PR-URL: nodejs/node-addon-api#299 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs/node-addon-api#299 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs/node-addon-api#299 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs/node-addon-api#299 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
First pass on function documentation. Please review only function.md. I'm actually writing the function reference documentation and I will push it as soon as possible.