Repository navigation
N-API documentation needs review (misleading / wrong code samples, etc) #20421
Copy link
Copy link
Closed
Labels
docIssues and PRs related to Node.js documentation.Issues and PRs related to Node.js documentation.node-apiIssues and PRs related to Node-API.Issues and PRs related to Node-API.
Description
I've been reading the N-API docs to understand it and it needs some cleanup. I'm not sure if all of these notes are still an issue in master.
I also somewhere saw a line which was missing its ;, and now I can't find it.
N-API
These wrappers are not part of N-API, nor will they be maintained as part of Node.js. One such example is: node-api.
- node-api has been renamed node-addon-api, and its repo has been renamed.
napi_status
- The typedef described in the documentation doesn't match the definition in the 10.0.0 header file.
napi_create_error
- Weird formatting of text 'be associated with the error':
Making handle lifespan shorter ...
napi_status status = napi_get_element(e, object, i, &result);e->env. Likewise in the second example in this block, where the environment is referred to asenvin calls to some methods but notnapi_get_element.
Module registration:
To add the method hello as a function ...
napi_value Init(napi_env env, napi_value exports) {
napi_status status;
napi_property_descriptor desc =
{"hello", Method, 0, 0, 0, napi_default, 0};
if (status != napi_ok) return NULL;
status = napi_define_properties(env, exports, 1, &desc);
if (status != napi_ok) return NULL;
return exports;
}statusis checked before it is assignednapi_property_descriptorfields are (utf8name,name,method,getter,setter,value,attr,data). There are 8 of them, not 7. Unless I'm missing something, the code should be{"hello", 0, Method, 0, 0, 0, napi_default, 0}. But imho it should useNULLinstead of 0. In modern C I would simply write this as{.utf8name="hello", .method=Method},although I'm not sure if the VC++ compiler can handle struct property initializers yet.The internet says yes- To make it more obvious how to extend the example, it might be better to make
descan array ofnapi_property_descriptorobjects. Although the class example does that... so maybe its not super important.
To define a class ...
napi_value Init(napi_env env, napi_value exports) {
napi_status status;
napi_property_descriptor properties[] = {
{ "value", NULL, GetValue, SetValue, 0, napi_default, 0 },
DECLARE_NAPI_METHOD("plusOne", PlusOne),
DECLARE_NAPI_METHOD("multiply", Multiply),
};
// ...- Again, the property descriptor is invalid. It should be
{ "value", NULL, 0, GetValue, SetValue, 0, napi_default, 0 },. DECLARE_NAPI_METHODis not a real thing - it does not exist anywhere else in the documentation or header files. This example should either define it locally or not use it.
napi_property_descriptor
- The order of the documentation for the
dataandattributesfields should be swapped
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
docIssues and PRs related to Node.js documentation.Issues and PRs related to Node.js documentation.node-apiIssues and PRs related to Node-API.Issues and PRs related to Node-API.

Activity
vsemozhetbyt commentedon Apr 30, 2018
Thank you.
I will fix
'be associated with the error'formatting issue soon.cc @nodejs/n-api @nodejs/documentation
mhdawson commentedon Apr 30, 2018
PR to cleanup all by the modules section and the 'be associated with the error' which @vsemozhetbyt mentioned he is looking at: #20430
Will look at the modules section next.
mhdawson commentedon Apr 30, 2018
@josephg thanks for the comments/suggestions for cleanup.
gabrielschulhof commentedon Apr 30, 2018
#20433 fixes the
napi_property_descriptor-related parts.vsemozhetbyt commentedon Apr 30, 2018
'be associated with the error'formatting issues were fixed during the broader sweep in #20438 (the cause was wrongly parsed indentation).doc: cleanup n-api.md doc
doc: cleanup n-api.md doc
doc: cleanup n-api.md doc
doc: cleanup n-api.md doc
doc: cleanup n-api.md doc
doc: cleanup n-api.md doc
doc: cleanup n-api.md doc
jasnell commentedon Oct 24, 2018
has this been resolved?
josephg commentedon Oct 24, 2018
Yes! Thanks everyone