esm: add experimental support for addon modules #55844
Conversation
|
Review requested:
|
nodejs-github-bot
added
lib / src
legendecas
force-pushed
the
esm/addon
branch
from
d4b9a9a
to
b384e80
Compare
legendecas
force-pushed
the
esm/addon
branch
2 times, most recently
from
304bd08
to
c6ce9fd
Compare
legendecas
force-pushed
the
esm/addon
branch
from
c6ce9fd
to
0c724cd
Compare
|
The commit message does not meet our guidelines, the word after the subsystem should be an infinitive verb. I suggest |
|
|
||
| // The addon must be loaded from file system with dlopen. Assert | ||
| // the source is null. | ||
| validateNull(source, 'source'); |
There was a problem hiding this comment.
It's not an argument, it wouldn't make sense to throw ERR_INVALID_ARG_TYPE. TBH I think we can simply ignore the source
There was a problem hiding this comment.
IMO asserting it to be null could prevent loader from unexpectedly reading the .node file from fs. It is true that it is not used. Discarding it silently could be confusing.
There was a problem hiding this comment.
In any case, let's not throw ERR_INVALID_ARG_TYPE, it's only going to be confusing. A more fitting error code would be ERR_INVALID_RETURN_PROPERTY_VALUE
| import('./test-esm.mjs') | ||
| .then(common.mustCall(), common.mustNotCall()); |
There was a problem hiding this comment.
FWIW the common.mustNotCall() is not necessary. I assume we also want to test if loading it with require works
| import('./test-esm.mjs') | |
| .then(common.mustCall(), common.mustNotCall()); | |
| import('./test-esm.mjs').then(common.mustCall()); | |
| require('./test-esm.mjs'); |
legendecas
changed the title
legendecas
changed the title
| added: REPLACEME | ||
| --> | ||
|
|
||
| Enable experimental addon modules with extension `.node` support. |
There was a problem hiding this comment.
| Enable experimental addon modules with extension `.node` support. | |
| > Stability: 1.0 - Early development | |
| Enable experimental addon modules with extension `.node` support. |
| * @param {string} [source] - The source code of the module. | ||
| */ | ||
| function cjsPreparseModuleExports(filename, source) { | ||
| function cjsEmplaceModuleCacheEntry(filename) { |
There was a problem hiding this comment.
Can this please get a JSDoc?
| @@ -459,6 +509,19 @@ translators.set('wasm', async function(url, source) { | |||
| }).module; | |||
| }); | |||
|
|
|||
| // Strategy for loading a addon | |||
| translators.set('addon', async function(url, source, isMain) { | |||
There was a problem hiding this comment.
Nit: This function doesn’t contain await, so it doesn’t need to be async?
| translators.set('addon', async function(url, source, isMain) { | |
| translators.set('addon', function(url, source, isMain) { |
| * @callback validateNull | ||
| * @param {*} value | ||
| * @param {string} name | ||
| * @returns {asserts value is null} |
There was a problem hiding this comment.
| * @returns {asserts value is null} | |
| * @returns whether or not the value is null |
Nit: The return description isn’t a value, it’s an explanation, so it doesn’t go in braces (I think?).
There was a problem hiding this comment.
| @@ -411,6 +411,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { | |||
| "Treat the entrypoint as a URL", | |||
| &EnvironmentOptions::entry_is_url, | |||
| kAllowedInEnvvar); | |||
| AddOption("--experimental-addon-modules", | |||
| "experimental ES Module support for addons", | |||
There was a problem hiding this comment.
| "experimental ES Module support for addons", | |
| "experimental import support for addons", |
Technically we’re adding support to import, which exists in CommonJS too as import().
Add experimental support to loading
.nodeextension modules in ESM.An addon exports two names
defaultandmodule.exports, as same asimport(cjs)whereits export names can not be preparsed. Addon export names can not be inferred until it is evaluated.
Fixes: #40541
Fixes: #55821