n-api: add napi_get_module()

[gabrielschulhof]

We make the napi_env per-module once more because it no longer
holds any state that needs to be shared among add-ons.

We then add a Node.js-specific N-API which allows one to obtain the
module whose exports were initialized at addon startup.

Re: nodejs/node-addon-api#449
Re: nodejs/node-addon-api#482
Re: nodejs/node-addon-api#505

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[mhdawson]

@gabrielschulhof we've had quite a few discussion on the topic and I thought we were still worried about exposing the module object directly because it's specific to CJS module loading ?

[gabrielschulhof]

@mhdawson, @devsnek I was keeping that in mind. Yet it looks like at least a few folks were using the module in their addon when it was implemented in Nan. I'm fine with changing this PR so that it doesn't simply serve up module. I'm happy to restrict it to providing certain metadata items which happen to be available via module.

Before I submitted a PR I had stuff like

NAPI_EXTERN napi_status
napi_get_module_filename(napi_env env, napi_value* result);

NAPI_EXTERN napi_status
napi_require(napi_env env,
             napi_value module_name,
             napi_value* result);

In the case of napi_require() I thought folks might want require.resolve() as well, so we'd just have to revisit this later for another N-API addition.

In the end, this N-API is in node_api.h, so that at least makes it clear that it's not language-related, but runtime-related. It is still possible to write runtime-agnostic code by including "js_native_api.h" instead.

As for the CJS "smell" of it, I'm hoping the fact that we're not upending the module initialization, but providing an N-API that one can call, if needed, will minimize the "smell".

I do not feel strongly about landing this PR. The only reason I wrote it is that it seems there's a fair chunk of folks who were using the Nan equivalent of this, and now have no reasonable recourse – especially not nodejs/node-addon-api#449, where the native addon is not fronted by a pure-JS module.

[gabrielschulhof]

I guess I would like to spur some discussion on what to do with module now that we're storing it on the napi_env as an alternative to simply exposing it – if we decide to do anything with it.

IMO the other important aspect of this PR is that we're moving back away from having a shared napi_env. This was made possible by removing all the v8::ObjectTemplates we were using for wrapping and callback info and stuff. Now we're beginning to rely on node::Environment to store shared things like the v8::Private values we use as keys.

[addaleax]

@gabrielschulhof I don’t know how people use this, but if I may offer an alternative suggestion: I’d like it if we could provide the addon with a way to store native data which is accessible through the napi_env, but whose lifetime is managed by N-API to not exceed the lifetime of the addon, because that is

1. currently hard to achieve properly,
2. helps with Worker integration, because people are still somewhat used to using static global data for addons,
3. what I imagine how people how would use napi_get_module() as suggested in this PR, and
4. allows storing the module object if so desired, so napi_get_module() could be implemented by users on top of it if that’s really necessary.

My suggestion for an API would basically be:

• napi_store_module_data(napi_env env, void* data, void(*cleanup)(void* data));
• napi_get_module_data(napi_env env);

with cleanup(data); being called when the native addon exits its current scope.

[gabrielschulhof]

@addaleax is it safe to use napi_env and make N-API calls from an environment cleanup hook? If so, this is already possible with existing APIs. The data need not be stored on the napi_env because it can be passed to bindings using the void* data parameter of napi_property_descriptor or napi_create_function() and retrieved using napi_get_cb_info(). Async callbacks always have a void* data, so the addon data can reach them that way. The only problem has been the life cycle management of it, which is why I added napi_add_finalizer() so that maintainers might tie its life cycle to that of exports, but that, as we know, doesn't get called during environment teardown, unless we land #28428. We are already discouraging the use of global static variables in https://nodejs.org/dist/latest-v12.x/docs/api/addons.html#addons_context_aware_addons. So, perhaps the pattern we should be encouraging is this:

#include <stdlib.h>
#include <node_api.h>

typedef struct {
  int data_item_1;
  double data_item_2;
} AddonData;

static void DeleteAddonData(void* data) {
  free(data);
}

static napi_value DoSomething(napi_env env, napi_callback_info info) {
  AddonData* addon_data;
  NAPI_CALL(env, napi_get_cb_info(env, info, NULL, NULL, NULL, &addon_data));
  return NULL;
}

static napi_value DoSomethingElse(napi_env env, napi_callback_info info) {
  AddonData* addon_data;
  NAPI_CALL(env, napi_get_cb_info(env, info, NULL, NULL, NULL, &addon_data));
  return NULL;
}

/* napi_value */ NAPI_MODULE_INIT(/* napi_env env, napi_value exports */) {
  AddonData* addon_data = malloc(sizeof(*addon_data);
  NAPI_CALL(env, napi_add_env_cleanup_hook(env, DeleteAddonDta, addon_data));
  napi_property_descriptor bindings[] = {
    {
      "doSomething",
      NULL,
      DoSomething,
      NULL,
      NULL,
      NULL,
      napi_default,
      addon_data
    },
    {
      "doSomethingElse",
      NULL,
      DoSomethingElse,
      NULL,
      NULL,
      NULL,
      napi_default,
      addon_data
    },
  };

  NAPI_CALL(env, napi_define_properties(env,
                                        exports,
                                        sizeof(bindings) / sizeof(*bindings),
                                        bindings));

  return exports;
}

This has the shortcoming that the napi_env doesn't make it into the env cleanup hook, so it'd have to be stored in AddonData 😕 if the hook were to make calls into N-API.

This PR is more about providing access to v8::Local<v8::Object> module as passed into the V8 initializer. That object has information that is otherwise unavailable to the addon, and really difficult (filename) or impossible (require()) to reconstruct by other means.

[mhdawson]

Exposing things like "filename" makese sense to but it would be great to expose that in a way that is agnostic to CJS or ESM. Maybe napi_get_module_data(napi_env env, char* name); with some fixed values for name, one being 'filename' ?

[devsnek]

This seems like something a module author should expose themselves with a js wrapper. Not even the base filename concept is compatible between esm and cjs.

[addaleax]

is it safe to use napi_env and make N-API calls from an environment cleanup hook? If so, this is already possible with existing APIs.

@gabrielschulhof It’s not safe to run JS at that point, but otherwise I think this should work. And sure, it’s possible to do this, it’s just not very ergonomical at this point (imo).

This has the shortcoming that the napi_env doesn't make it into the env cleanup hook, so it'd have to be stored in AddonData 😕 if the hook were to make calls into N-API.

I think storing it in AddonData is fine, it’s just a small oversight in the API design here.

This PR is more about providing access to v8::Local<v8::Object> module as passed into the V8 initializer. That object has information that is otherwise unavailable to the addon, and really difficult (filename) or impossible (require()) to reconstruct by other means.

I would agree with @devsnek on this – I’m not a fan of introducing something that’s not going to be ESM-compatible without having a good story for how that’s going to evolve in the context of ESM.

But honestly, I also feel like some of these things just belong in the JS wrapper that people usually write for their addon code.

[gabrielschulhof]

@addaleax since our discussion is OT wrt. this issue, I opened nodejs/abi-stable-node#378.

[gabrielschulhof]

@devsnek I agree that module authors should give themselves any such information they need via a JS module that fronts the native addon. The only reason I made this PR is because of the issues it references.

It seems that there are packages out there which make available the native addon directly, the consumers of which expect to be able to simply require('./path/to/native-addon.node') without any extra dance by which said consumers would supply the native addon with information otherwise available in module.

Moving these use cases to N-API entails an additional effort of re-obtaining buy-in from their consumers for a new loading convention, like maybe

const theAddon =
  require('./path/to/native-addon.node')(
    require.resolve('./path/to/native-addon.node'));

It does seem like this use case is not overly widespread though. I'm happy to close this PR if we can agree that the risk of opening a can of worms in the CJS/ESM space outweighs the advantage of supporting the case where the native addon is not fronted by a JS module.

[mhdawson]

Based on discussions in the N-API team meeting with Gabriel my current understanding is that we don't plan to land this.

[mhdawson]

Added blocked so that it does not in-inadvertently land. @gabrielschulhof let me know if this is not the right thing to do.

[gabrielschulhof]

@mhdawson yeah, let's leave it in the blocked state for now.

[fhinkel]

@gabrielschulhof should this remain open?

[jasnell]

Ping @nodejs/n-api ...Unfortunately this has stalled. Closing but it can be reopened if anyone wants to pick this back up.