Repository navigation
ES6 module loader compatibility #256
Description
Current napi_addon_register_func callback is quite obviously designed around CommonJS (for understandable reasons), but given that N-API is a new API, it might be easy enough to provide something that will be module-system-agnostic and work well with CommonJS while also being aligned better with ES6 import statements that are upcoming in Node.js.
One of the main incompatibilities from aligning current CommonJS ecosystem with ES6 comes from the fact that in CommonJS you can either export arbitrary named declarations, or you can completely redefine module.exports object with any JS value.
ES6 module system, on the other hand, does not allow such conflicts and presents all the exports on a single object with explicit names (or default as a key), so there are arguments about how to import module.exports case - should it be imported as default one and then effectively each CommonJS module can be used only as import fs from "fs" and not import { readFile } from "fs", or should it do some magic and destructure itself into ES6 names exports as well.
To avoid similar incompatibilies, my suggestion is to make loader system opaque to the registration callback and either:
1. Allow only named exports by having something like:
typedef struct {
const char* name; // NULL for default export
napi_value value;
} napi_export_descriptor;
NAPI_EXTERN napi_status napi_export(napi_env env, size_t export_count, const napi_export_descriptor* descriptors);Which would do equivalent of exports.name = value / exports.default = value in CommonJS and export const name = value / export default value in ES6 module system.
This might be incompatible with some existing modules though that effectively used module.exports = ... and such change would require them updating require(...) part too.
2. Allow only "default" export. Effectively this means changing a callback to:
typedef void (*napi_addon_register_func)(napi_env env, napi_value *result);For modules that just defined properties on given exports empty object, the only difference would be that they will have to call napi_create_object theirselves and return it in the result.
For modules that used to override module.exports, this only simplifies things as they don't need to use property modification APIs to change "exports" property on a given module and can just return the value directly.
For CommonJS users nothing will change and require(...) will work exactly like before.
For ES6, this will allow to import such modules with import nativeModule from "..." and using its properties - this won't allow having named imports unfortunately, but that's a minor inconvenience, and it will allow module loader to load native module without having to simulate CommonJS' module and exports wrapper objects.
Personally, I'm leaning towards option #2 as that's a pretty simple change that aligns well with both CommonJS and ES6 loaders without breaking compatibility with existing native modules in the ecosystem, but would love to hear your thoughts.
Activity
jasongin commentedon Jul 24, 2017
@nodejs/n-api I think this is an interesting suggestion, and worthy of further consideration soon, before it's too late to make breaking changes.
I think a combination of the two described options above could provide the best of both. The object returned from the registration callback would be the default export for ES6, but modules could additionally call
napi_export()from the registration callback if they want to define named exports.RReverser commentedon Jul 26, 2017
@jasongin Combination of both is problematic since it doesn't map very clearly to CommonJS even in JS land (that's the interop issue I was talking above).
On one hand, ES6 exports default as implicit property "default" on the module object, and then CommonJS consumers must do things like
require("...").default, on another usual expectation is justrequire("...")for convenience, which means that somehow module default must be mapped ontomodule.exportsand then named exports be merged into that same object, which kinda breaks semantics.This is why I suggested to provide only one of them, since in that case modules will be compatible with either semantics with no extra effort.
boingoing commentedon Aug 8, 2017
I'm not sure why we couldn't have a combination approach like Jason is proposing. Instead of the result object of
napi_addon_register_funcbeing the default export, we could return the module namespace object instead. That object has all the named exports as own properties. The import syntax is a little clumsierimport * as fs from 'fs'but named imports could also work if we suck those out of the module namespace object.RReverser commentedon Aug 8, 2017
Yes, that's the second approach I suggested.
As long as we return only one object instead of mixing various export types, it can be mapped just fine to either module loader.
digitalinfinity commentedon Aug 17, 2017
@kkoopa @Fishrock123 @bmeck do you guys have any thoughts on this discussion? The concern from the n-api team is to make sure we're aligned with whatever Node's plans for ES6 modules are
bmeck commentedon Aug 17, 2017
I'd like to setup a call on this if possible because this is a lot of nuanced discussion. I don't think we have live binding hooks / circular reference hooks for ESM etc. There was a minor spec change in particular that expects non-JavaScript Source Text Module Records to stay as leaves of the graph.
Perhaps the WASM discussions currently going on this area are also relevant.
digitalinfinity commentedon Aug 17, 2017
@bmeck we have a regular hangout for the n-api team on Thursdays at 10:30 AM PST. Would that work as a forum for this discussion? @sampsongao can provide details
bmeck commentedon Aug 17, 2017
@digitalinfinity sure
RReverser commentedon Aug 17, 2017
@bmeck I don't think live bindings matter for native modules, at least at this point. I've raised this mostly to discuss a single way to export something from native modules so that they don't run into similar incompatibility issues and questions to CommonJS. Anyway, I'll try to join the call too, let's discuss there.
aruneshchandra commentedon Aug 24, 2017
@bmeck we are having a N-API meeting right now to discuss this if you want to join https://plus.google.com/u/0/events/c0eevtrlajniu7h8cjrdk0f56c8?authkey=COH04YCalJS8Ug
RReverser commentedon Aug 26, 2017
In the meeting, we discussed various options and came to the conclusion that native modules are rarely exposed to the end user directly, so it doesn't make much sense to complicate the design and attempt to provide a first-class support for any specific module loader.
Instead, we are going to go with option (2) with slight modifications as it doesn't require the module to know anything about the environment it's being used in. This way native module will be able to export a single item (function, custom object or any other JavaScript value) and it will be up to the JavaScript side to adapt it to the API that package consumers expect.
As a result, suggested callback format is:
Returning a value from such callback will have the same effect as
module.exports = ...currently does in CommonJS.jasongin commentedon Aug 26, 2017
I missed the meeting, but that plan and reasoning makes sense to me.
We should try to get this breaking change over with soon.
boingoing commentedon Aug 30, 2017
PR opened for this refactor:
nodejs/node#15088
6 remaining items
n-api: refactor napi_addon_register_func
n-api: refactor napi_addon_register_func
RReverser commentedon Nov 12, 2017
I believe this was addressed now, thanks to everyone!
n-api: refactor napi_addon_register_func