N-API documentation needs review (misleading / wrong code samples, etc)

[josephg]

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 as env in calls to some methods but not napi_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;
}

• status is checked before it is assigned
• napi_property_descriptor fields 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 use NULL instead 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 desc an array of napi_property_descriptor objects. 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_METHOD is 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 data and attributes fields should be swapped

[vsemozhetbyt]

Thank you.

I will fix 'be associated with the error' formatting issue soon.

cc @nodejs/n-api @nodejs/documentation

[mhdawson]

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]

@josephg thanks for the comments/suggestions for cleanup.

[gabrielschulhof]

#20433 fixes the napi_property_descriptor-related parts.

[vsemozhetbyt]

'be associated with the error' formatting issues were fixed during the broader sweep in #20438 (the cause was wrongly parsed indentation).

[jasnell]

has this been resolved?

[josephg]

Yes! Thanks everyone