Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esm: add experimental support for addon modules #55844

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ Otherwise, the file is loaded using the CommonJS module loader. See
When loading, the [ES module loader][Modules loaders] loads the program
entry point, the `node` command will accept as input only files with `.js`,
`.mjs`, or `.cjs` extensions; with `.wasm` extensions when
[`--experimental-wasm-modules`][] is enabled; and with no extension when
[`--experimental-wasm-modules`][] is enabled; with `.node` extensions when
[`--experimental-addon-modules`][] is enabled; and with no extension when
[`--experimental-default-type=module`][] is passed.

## Options
Expand Down Expand Up @@ -903,6 +904,14 @@ and `"` are usable.
It is possible to run code containing inline types by passing
[`--experimental-strip-types`][].

### `--experimental-addon-modules`

<!-- YAML
added: REPLACEME
-->

Enable experimental addon modules with extension `.node` support.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Enable experimental addon modules with extension `.node` support.
> Stability: 1.0 - Early development
Enable experimental addon modules with extension `.node` support.


### `--experimental-default-type=type`

<!-- YAML
Expand Down Expand Up @@ -3031,6 +3040,7 @@ one is included in the list below.
* `--enable-source-maps`
* `--entry-url`
* `--experimental-abortcontroller`
* `--experimental-addon-modules`
* `--experimental-default-type`
* `--experimental-detect-module`
* `--experimental-eventsource`
Expand Down Expand Up @@ -3589,6 +3599,7 @@ node --stack-trace-limit=12 -p -e "Error.stackTraceLimit" # prints 12
[`--diagnostic-dir`]: #--diagnostic-dirdirectory
[`--env-file-if-exists`]: #--env-file-if-existsconfig
[`--env-file`]: #--env-fileconfig
[`--experimental-addon-modules`]: #--experimental-addon-modules
[`--experimental-default-type=module`]: #--experimental-default-typetype
[`--experimental-sea-config`]: single-executable-applications.md#generating-single-executable-preparation-blobs
[`--experimental-strip-types`]: #--experimental-strip-types
Expand Down
19 changes: 11 additions & 8 deletions doc/api/esm.md
Original file line number Diff line number Diff line change
Expand Up @@ -1027,18 +1027,21 @@ _isImports_, _conditions_)
> 5. If `--experimental-wasm-modules` is enabled and _url_ ends in
> _".wasm"_, then
> 1. Return _"wasm"_.
> 6. Let _packageURL_ be the result of **LOOKUP\_PACKAGE\_SCOPE**(_url_).
> 7. Let _pjson_ be the result of **READ\_PACKAGE\_JSON**(_packageURL_).
> 8. Let _packageType_ be **null**.
> 9. If _pjson?.type_ is _"module"_ or _"commonjs"_, then
> 1. Set _packageType_ to _pjson.type_.
> 10. If _url_ ends in _".js"_, then
> 6. If `--experimental-addon-modules` is enabled and _url_ ends in
> _".node"_, then
> 1. Return _"addon"_.
> 7. Let _packageURL_ be the result of **LOOKUP\_PACKAGE\_SCOPE**(_url_).
> 8. Let _pjson_ be the result of **READ\_PACKAGE\_JSON**(_packageURL_).
> 9. Let _packageType_ be **null**.
> 10. If _pjson?.type_ is _"module"_ or _"commonjs"_, then
> 1. Set _packageType_ to _pjson.type_.
> 11. If _url_ ends in _".js"_, then
> 1. If _packageType_ is not **null**, then
> 1. Return _packageType_.
> 2. If the result of **DETECT\_MODULE\_SYNTAX**(_source_) is true, then
> 1. Return _"module"_.
> 3. Return _"commonjs"_.
> 11. If _url_ does not have any extension, then
> 12. If _url_ does not have any extension, then
> 1. If _packageType_ is _"module"_ and `--experimental-wasm-modules` is
> enabled and the file at _url_ contains the header for a WebAssembly
> module, then
Expand All @@ -1048,7 +1051,7 @@ _isImports_, _conditions_)
> 3. If the result of **DETECT\_MODULE\_SYNTAX**(_source_) is true, then
> 1. Return _"module"_.
> 4. Return _"commonjs"_.
> 12. Return **undefined** (will throw during load phase).
> 13. Return **undefined** (will throw during load phase).

**LOOKUP\_PACKAGE\_SCOPE**(_url_)

Expand Down
3 changes: 3 additions & 0 deletions doc/node.1
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,9 @@ Enable Source Map V3 support for stack traces.
.It Fl -entry-url
Interpret the entry point as a URL.
.
.It Fl -experimental-addon-modules
Enable experimental addon module support.
.
.It Fl -experimental-default-type Ns = Ns Ar type
Interpret as either ES modules or CommonJS modules input via --eval or STDIN, when --input-type is unspecified;
.js or extensionless files with no sibling or parent package.json;
Expand Down
5 changes: 5 additions & 0 deletions lib/internal/modules/esm/formats.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const fsBindings = internalBinding('fs');
const { fs: fsConstants } = internalBinding('constants');

const experimentalWasmModules = getOptionValue('--experimental-wasm-modules');
const experimentalAddonModules = getOptionValue('--experimental-addon-modules');

const extensionFormatMap = {
'__proto__': null,
Expand All @@ -23,6 +24,10 @@ if (experimentalWasmModules) {
extensionFormatMap['.wasm'] = 'wasm';
}

if (experimentalAddonModules) {
extensionFormatMap['.node'] = 'addon';
}

if (getOptionValue('--experimental-strip-types')) {
extensionFormatMap['.ts'] = 'module-typescript';
extensionFormatMap['.mts'] = 'module-typescript';
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/modules/esm/load.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,9 @@ async function defaultLoad(url, context = kEmptyObject) {
if (urlInstance.protocol === 'node:') {
source = null;
format ??= 'builtin';
} else if (format === 'addon') {
// Skip loading addon file content. It must be loaded with dlopen from file system.
source = null;
} else if (format !== 'commonjs' || defaultType === 'module') {
if (source == null) {
({ responseURL, source } = await getSource(urlInstance, context));
Expand Down
81 changes: 72 additions & 9 deletions lib/internal/modules/esm/translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const {
ERR_UNKNOWN_BUILTIN_MODULE,
} = require('internal/errors').codes;
const { maybeCacheSourceMap } = require('internal/source_map/source_map_cache');
const { validateNull } = require('internal/validators');
const moduleWrap = internalBinding('module_wrap');
const { ModuleWrap } = moduleWrap;

Expand Down Expand Up @@ -225,6 +226,48 @@ function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) {
}, module);
}

/**
* Creates a ModuleWrap object for a CommonJS module without source texts.
* @param {string} url - The URL of the module.
* @param {boolean} isMain - Whether the module is the main module.
* @returns {ModuleWrap} The ModuleWrap object for the CommonJS module.
*/
function createCJSNoSourceModuleWrap(url, isMain) {
debug(`Translating CJSModule without source ${url}`);

const filename = urlToFilename(url);

const { exportNames, module } = cjsEmplaceModuleCacheEntry(filename);
if (exportNames === undefined) {
// Addon export names are not known until the addon is loaded.
module[kModuleExportNames] = ['default', 'module.exports'];
}
cjsCache.set(url, module);

if (isMain) {
setOwnProperty(process, 'mainModule', module);
}

const wrapperNames = module[kModuleExportNames];
return new ModuleWrap(url, undefined, wrapperNames, function() {
debug(`Loading CJSModule ${url}`);

if (!module.loaded) {
wrapModuleLoad(filename, null, isMain);
}

let exports;
if (module[kModuleExport] !== undefined) {
exports = module[kModuleExport];
module[kModuleExport] = undefined;
} else {
({ exports } = module);
}
this.setExport('default', exports);
this.setExport('module.exports', exports);
}, module);
}

translators.set('commonjs-sync', function requireCommonJS(url, source, isMain) {
initCJSParseSync();

Expand Down Expand Up @@ -276,12 +319,7 @@ translators.set('commonjs', function commonjsStrategy(url, source, isMain) {
return createCJSModuleWrap(url, source, isMain, cjsLoader);
});

/**
* Pre-parses a CommonJS module's exports and re-exports.
* @param {string} filename - The filename of the module.
* @param {string} [source] - The source code of the module.
*/
function cjsPreparseModuleExports(filename, source) {
function cjsEmplaceModuleCacheEntry(filename) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this please get a JSDoc?

// TODO: Do we want to keep hitting the user mutable CJS loader here?
let module = CJSModule._cache[filename];
if (module && module[kModuleExportNames] !== undefined) {
Expand All @@ -293,10 +331,23 @@ function cjsPreparseModuleExports(filename, source) {
module.filename = filename;
module.paths = CJSModule._nodeModulePaths(module.path);
module[kIsCachedByESMLoader] = true;
module[kModuleSource] = source;
CJSModule._cache[filename] = module;
}

return { module };
}

/**
* Pre-parses a CommonJS module's exports and re-exports.
* @param {string} filename - The filename of the module.
* @param {string} [source] - The source code of the module.
*/
function cjsPreparseModuleExports(filename, source) {
const { module, exportNames: cachedExportNames } = cjsEmplaceModuleCacheEntry(filename);
if (cachedExportNames !== undefined) {
return { module, exportNames: cachedExportNames };
}

let exports, reexports;
try {
({ exports, reexports } = cjsParse(source || ''));
Expand All @@ -308,11 +359,10 @@ function cjsPreparseModuleExports(filename, source) {
const exportNames = new SafeSet(new SafeArrayIterator(exports));

// Set first for cycles.
module[kModuleSource] = source;
module[kModuleExportNames] = exportNames;

if (reexports.length) {
module.filename = filename;
module.paths = CJSModule._nodeModulePaths(module.path);
for (let i = 0; i < reexports.length; i++) {
const reexport = reexports[i];
let resolved;
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This function doesn’t contain await, so it doesn’t need to be async?

Suggested change
translators.set('addon', async function(url, source, isMain) {
translators.set('addon', function(url, source, isMain) {

emitExperimentalWarning('Importing addons');

// The addon must be loaded from file system with dlopen. Assert
// the source is null.
validateNull(source, 'source');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@aduh95 aduh95 Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


debug(`Translating addon ${url}`);

return createCJSNoSourceModuleWrap(url, isMain);
});

// Strategy for loading a commonjs TypeScript module
translators.set('commonjs-typescript', function(url, source) {
emitExperimentalWarning('Type Stripping');
Expand Down
14 changes: 14 additions & 0 deletions lib/internal/validators.js
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,19 @@ const validateUndefined = hideStackFrames((value, name) => {
throw new ERR_INVALID_ARG_TYPE(name, 'undefined', value);
});

/**
* @callback validateNull
* @param {*} value
* @param {string} name
* @returns {asserts value is null}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @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?).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*/

/** @type {validateNull} */
const validateNull = hideStackFrames((value, name) => {
if (value !== null)
throw new ERR_INVALID_ARG_TYPE(name, 'null', value);
});

/**
* @template T
* @param {T} value
Expand Down Expand Up @@ -623,6 +636,7 @@ module.exports = {
validateFunction,
validateInt32,
validateInteger,
validateNull,
validateNumber,
validateObject,
kValidateObjectNone,
Expand Down
4 changes: 4 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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().

&EnvironmentOptions::experimental_addon_modules,
kAllowedInEnvvar);
AddOption("--experimental-abortcontroller", "", NoOp{}, kAllowedInEnvvar);
AddOption("--experimental-eventsource",
"experimental EventSource API",
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ class EnvironmentOptions : public Options {
bool require_module = true;
std::string dns_result_order;
bool enable_source_maps = false;
bool experimental_addon_modules = false;
bool experimental_eventsource = false;
bool experimental_fetch = true;
bool experimental_websocket = true;
Expand Down
17 changes: 17 additions & 0 deletions test/addons/esm/binding-export-default.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#include <node.h>
#include <uv.h>
#include <v8.h>

static void Method(const v8::FunctionCallbackInfo<v8::Value>& args) {
v8::Isolate* isolate = args.GetIsolate();
args.GetReturnValue().Set(
v8::String::NewFromUtf8(isolate, "hello world").ToLocalChecked());
}

static void InitModule(v8::Local<v8::Object> exports,
v8::Local<v8::Value> module,
v8::Local<v8::Context> context) {
NODE_SET_METHOD(exports, "default", Method);
}

NODE_MODULE_CONTEXT_AWARE(Binding, InitModule)
17 changes: 17 additions & 0 deletions test/addons/esm/binding-export-primitive.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#include <node.h>
#include <uv.h>
#include <v8.h>

static void InitModule(v8::Local<v8::Object> exports,
v8::Local<v8::Value> module_val,
v8::Local<v8::Context> context) {
v8::Isolate* isolate = context->GetIsolate();
v8::Local<v8::Object> module = module_val.As<v8::Object>();
module
->Set(context,
v8::String::NewFromUtf8(isolate, "exports").ToLocalChecked(),
v8::String::NewFromUtf8(isolate, "hello world").ToLocalChecked())
.FromJust();
}

NODE_MODULE_CONTEXT_AWARE(Binding, InitModule)
17 changes: 17 additions & 0 deletions test/addons/esm/binding.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#include <node.h>
#include <uv.h>
#include <v8.h>

static void Method(const v8::FunctionCallbackInfo<v8::Value>& args) {
v8::Isolate* isolate = args.GetIsolate();
args.GetReturnValue().Set(
v8::String::NewFromUtf8(isolate, "world").ToLocalChecked());
}

static void InitModule(v8::Local<v8::Object> exports,
v8::Local<v8::Value> module,
v8::Local<v8::Context> context) {
NODE_SET_METHOD(exports, "hello", Method);
}

NODE_MODULE_CONTEXT_AWARE(Binding, InitModule)
19 changes: 19 additions & 0 deletions test/addons/esm/binding.gyp
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
'targets': [
{
'target_name': 'binding',
'sources': [ 'binding.cc' ],
'includes': ['../common.gypi'],
},
{
'target_name': 'binding-export-default',
'sources': [ 'binding-export-default.cc' ],
'includes': ['../common.gypi'],
},
{
'target_name': 'binding-export-primitive',
'sources': [ 'binding-export-primitive.cc' ],
'includes': ['../common.gypi'],
}
]
}
Loading