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

module: unflag import assertions #39921

Closed
wants to merge 13 commits into from
16 changes: 16 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1108,6 +1108,14 @@ The JS execution context is not associated with a Node.js environment.
This may occur when Node.js is used as an embedded library and some hooks
for the JS engine are not set up properly.

<a id="ERR_FAILED_IMPORT_ASSERTION"></a>
### `ERR_FAILED_IMPORT_ASSERTION`
<!-- YAML
added: REPLACEME
-->

An import assertion has failed, preventing the specified module to be imported.

<a id="ERR_FALSY_VALUE_REJECTION"></a>
### `ERR_FALSY_VALUE_REJECTION`

Expand Down Expand Up @@ -1659,6 +1667,14 @@ for more information.

An invalid HTTP token was supplied.

<a id="ERR_INVALID_IMPORT_ASSERTION"></a>
### `ERR_INVALID_IMPORT_ASSERTION`
<!-- YAML
added: REPLACEME
-->

An import assertion is not supported by this version of Node.js.

<a id="ERR_INVALID_IP_ADDRESS"></a>
### `ERR_INVALID_IP_ADDRESS`

Expand Down
27 changes: 25 additions & 2 deletions doc/api/esm.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
<!-- YAML
added: v8.5.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39921
description: Add support for import assertions.
- version:
- REPLACEME
pr-url: https://github.com/nodejs/node/pull/37468
Expand Down Expand Up @@ -228,6 +231,25 @@ absolute URL strings.
import fs from 'node:fs/promises';
```

## Import assertions
<!-- YAML
added: REPLACEME
-->

The [Import Assertions proposal][] adds an inline syntax for module import
statements to pass on more information alongside the module specifier.

```js
import json from './foo.json' assert { type: "json" };
await import('foo.json', { assert: { type: "json" } });
```

Node.js supports the following `type` values:

| `type` | Resolves to |
| -------- | ---------------- |
| `"json"` | [JSON modules][] |

## Builtin modules

[Core modules][] provide named exports of their public API. A
Expand Down Expand Up @@ -516,9 +538,8 @@ same path.

Assuming an `index.mjs` with

<!-- eslint-skip -->
```js
import packageConfig from './package.json';
import packageConfig from './package.json' assert { type: 'json' };
```

The `--experimental-json-modules` flag is needed for the module
Expand Down Expand Up @@ -1367,6 +1388,8 @@ success!
[Dynamic `import()`]: https://wiki.developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import#Dynamic_Imports
[ECMAScript Top-Level `await` proposal]: https://github.com/tc39/proposal-top-level-await/
[ES Module Integration Proposal for Web Assembly]: https://github.com/webassembly/esm-integration
[Import Assertions proposal]: https://github.com/tc39/proposal-import-assertions
[JSON modules]: #json-modules
[Node.js Module Resolution Algorithm]: #resolver-algorithm-specification
[Terminology]: #terminology
[URL]: https://url.spec.whatwg.org/
Expand Down
39 changes: 39 additions & 0 deletions doc/api/vm.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ executed in specific contexts.
<!-- YAML
added: v0.3.1
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39921
description: Added suppoort of import assertions to the
`importModuleDynamically` parameter.
- version: v10.6.0
pr-url: https://github.com/nodejs/node/pull/20300
description: The `produceCachedData` is deprecated in favour of
Expand Down Expand Up @@ -91,6 +95,8 @@ changes:
using it in a production environment.
* `specifier` {string} specifier passed to `import()`
* `script` {vm.Script}
* `import_assertions` {Object} The `"assert"` value passed to the
`optionExpression` optional parameter.
* Returns: {Module Namespace Object|vm.Module} Returning a `vm.Module` is
recommended in order to take advantage of error tracking, and to avoid
issues with namespaces that contain `then` function exports.
Expand Down Expand Up @@ -642,6 +648,13 @@ The `vm.SourceTextModule` class provides the [Source Text Module Record][] as
defined in the ECMAScript specification.

### `new vm.SourceTextModule(code[, options])`
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39921
description: Added suppoort of import assertions to the
`importModuleDynamically` parameter.
-->

* `code` {string} JavaScript Module code to parse
* `options`
Expand All @@ -667,6 +680,8 @@ defined in the ECMAScript specification.
`import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
* `specifier` {string} specifier passed to `import()`
* `module` {vm.Module}
* `import_assertions` {Object} The `"assert"` value passed to the
`optionExpression` optional parameter.
* Returns: {Module Namespace Object|vm.Module} Returning a `vm.Module` is
recommended in order to take advantage of error tracking, and to avoid
issues with namespaces that contain `then` function exports.
Expand Down Expand Up @@ -852,6 +867,10 @@ const vm = require('vm');
<!-- YAML
added: v10.10.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39921
description: Added suppoort of import assertions to the
`importModuleDynamically` parameter.
- version: v15.9.0
pr-url: https://github.com/nodejs/node/pull/35431
description: Added `importModuleDynamically` option again.
Expand Down Expand Up @@ -893,6 +912,8 @@ changes:
considered stable.
* `specifier` {string} specifier passed to `import()`
* `function` {Function}
* `import_assertions` {Object} The `"assert"` value passed to the
`optionExpression` optional parameter.
* Returns: {Module Namespace Object|vm.Module} Returning a `vm.Module` is
recommended in order to take advantage of error tracking, and to avoid
issues with namespaces that contain `then` function exports.
Expand Down Expand Up @@ -1068,6 +1089,10 @@ vm.measureMemory({ mode: 'detailed', execution: 'eager' })
<!-- YAML
added: v0.3.1
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39921
description: Added suppoort of import assertions to the
`importModuleDynamically` parameter.
- version: v6.3.0
pr-url: https://github.com/nodejs/node/pull/6635
description: The `breakOnSigint` option is supported now.
Expand Down Expand Up @@ -1113,6 +1138,8 @@ changes:
using it in a production environment.
* `specifier` {string} specifier passed to `import()`
* `script` {vm.Script}
* `import_assertions` {Object} The `"assert"` value passed to the
`optionExpression` optional parameter.
* Returns: {Module Namespace Object|vm.Module} Returning a `vm.Module` is
recommended in order to take advantage of error tracking, and to avoid
issues with namespaces that contain `then` function exports.
Expand Down Expand Up @@ -1145,6 +1172,10 @@ console.log(contextObject);
<!-- YAML
added: v0.3.1
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39921
description: Added suppoort of import assertions to the
`importModuleDynamically` parameter.
- version: v14.6.0
pr-url: https://github.com/nodejs/node/pull/34023
description: The `microtaskMode` option is supported now.
Expand Down Expand Up @@ -1211,6 +1242,8 @@ changes:
using it in a production environment.
* `specifier` {string} specifier passed to `import()`
* `script` {vm.Script}
* `import_assertions` {Object} The `"assert"` value passed to the
`optionExpression` optional parameter.
* Returns: {Module Namespace Object|vm.Module} Returning a `vm.Module` is
recommended in order to take advantage of error tracking, and to avoid
issues with namespaces that contain `then` function exports.
Expand Down Expand Up @@ -1247,6 +1280,10 @@ console.log(contextObject);
<!-- YAML
added: v0.3.1
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/39921
description: Added suppoort of import assertions to the
`importModuleDynamically` parameter.
- version: v6.3.0
pr-url: https://github.com/nodejs/node/pull/6635
description: The `breakOnSigint` option is supported now.
Expand Down Expand Up @@ -1290,6 +1327,8 @@ changes:
using it in a production environment.
* `specifier` {string} specifier passed to `import()`
* `script` {vm.Script}
* `import_assertions` {Object} The `"assert"` value passed to the
`optionExpression` optional parameter.
* Returns: {Module Namespace Object|vm.Module} Returning a `vm.Module` is
recommended in order to take advantage of error tracking, and to avoid
issues with namespaces that contain `then` function exports.
Expand Down
6 changes: 6 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -953,6 +953,9 @@ E('ERR_ENCODING_NOT_SUPPORTED', 'The "%s" encoding is not supported',
RangeError);
E('ERR_EVAL_ESM_CANNOT_PRINT', '--print cannot be used with ESM input', Error);
E('ERR_EVENT_RECURSION', 'The event "%s" is already being dispatched', Error);
E('ERR_FAILED_IMPORT_ASSERTION', (request, key, expectedValue, actualValue) => {
return `Failed to load module "${request}", expected ${key} to be ${JSONStringify(expectedValue)}, got ${JSONStringify(actualValue)} instead`;
}, TypeError);
E('ERR_FALSY_VALUE_REJECTION', function(reason) {
this.reason = reason;
return 'Promise was rejected with falsy value';
Expand Down Expand Up @@ -1250,6 +1253,9 @@ E('ERR_INVALID_FILE_URL_HOST',
E('ERR_INVALID_FILE_URL_PATH', 'File URL path %s', TypeError);
E('ERR_INVALID_HANDLE_TYPE', 'This handle type cannot be sent', TypeError);
E('ERR_INVALID_HTTP_TOKEN', '%s must be a valid HTTP token ["%s"]', TypeError);
E('ERR_INVALID_IMPORT_ASSERTION',
(type, value) => `Invalid ${JSONStringify(type)} import assertion: ${JSONStringify(value)}`,
TypeError);
E('ERR_INVALID_IP_ADDRESS', 'Invalid IP address: %s', TypeError);
E('ERR_INVALID_MODULE_SPECIFIER', (request, reason, base = undefined) => {
return `Invalid module "${request}" ${reason}${base ?
Expand Down
10 changes: 6 additions & 4 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -1021,9 +1021,10 @@ function wrapSafe(filename, content, cjsModuleInstance) {
filename,
lineOffset: 0,
displayErrors: true,
importModuleDynamically: async (specifier) => {
importModuleDynamically: async (specifier, _, import_assertions) => {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t import_assertions be camelcased everywhere it appears?

const loader = asyncESM.esmLoader;
return loader.import(specifier, normalizeReferrerURL(filename));
return loader.import(specifier, normalizeReferrerURL(filename),
import_assertions);
},
});
}
Expand All @@ -1036,9 +1037,10 @@ function wrapSafe(filename, content, cjsModuleInstance) {
'__dirname',
], {
filename,
importModuleDynamically(specifier) {
importModuleDynamically(specifier, _, import_assertions) {
const loader = asyncESM.esmLoader;
return loader.import(specifier, normalizeReferrerURL(filename));
return loader.import(specifier, normalizeReferrerURL(filename),
import_assertions);
},
});
} catch (err) {
Expand Down
59 changes: 51 additions & 8 deletions lib/internal/modules/esm/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@ require('internal/modules/cjs/loader');
const {
Array,
ArrayIsArray,
ArrayPrototypeIncludes,
ArrayPrototypeJoin,
ArrayPrototypePush,
FunctionPrototypeBind,
FunctionPrototypeCall,
ObjectCreate,
ObjectFreeze,
ObjectSetPrototypeOf,
PromiseAll,
RegExpPrototypeExec,
Expand All @@ -20,8 +22,10 @@ const {
} = primordials;

const {
ERR_FAILED_IMPORT_ASSERTION,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_INVALID_IMPORT_ASSERTION,
ERR_INVALID_MODULE_SPECIFIER,
ERR_INVALID_RETURN_PROPERTY_VALUE,
ERR_INVALID_RETURN_VALUE,
Expand All @@ -44,6 +48,10 @@ const { translators } = require(
'internal/modules/esm/translators');
const { getOptionValue } = require('internal/options');

const importAssertionTypeCache = new SafeWeakMap();
const finalFormatCache = new SafeWeakMap();
const supportedTypes = ObjectFreeze([undefined, 'json']);

/**
* An ESMLoader instance is used as the main entry point for loading ES modules.
* Currently, this is a singleton -- there is only one used for loading
Expand Down Expand Up @@ -202,33 +210,66 @@ class ESMLoader {
const { ModuleWrap, callbackMap } = internalBinding('module_wrap');
const module = new ModuleWrap(url, undefined, source, 0, 0);
callbackMap.set(module, {
importModuleDynamically: (specifier, { url }) => {
return this.import(specifier, url);
importModuleDynamically: (specifier, { url }, import_assertions) => {
return this.import(specifier, url, import_assertions);
}
});

return module;
};
const job = new ModuleJob(this, url, evalInstance, false, false);
this.moduleMap.set(url, job);
finalFormatCache.set(job, 'module');
const { module } = await job.run();

return {
namespace: module.getNamespace(),
};
}

async getModuleJob(specifier, parentURL) {
async getModuleJob(specifier, parentURL, import_assertions) {
if (!ArrayPrototypeIncludes(supportedTypes, import_assertions.type)) {
throw new ERR_INVALID_IMPORT_ASSERTION('type', import_assertions.type);
}

const { format, url } = await this.resolve(specifier, parentURL);
let job = this.moduleMap.get(url);
// CommonJS will set functions for lazy job evaluation.
if (typeof job === 'function') this.moduleMap.set(url, job = job());

if (job !== undefined) return job;
if (job != null) {
const currentImportAssertionType = importAssertionTypeCache.get(job);
if (currentImportAssertionType === import_assertions.type) return job;

try {
// To avoid race conditions, wait for previous module to fulfill first.
await job.modulePromise;
aduh95 marked this conversation as resolved.
Show resolved Hide resolved
} catch {
// If the other job failed with a different `type` assertion, we got
// another chance.
job = undefined;
}

if (job !== undefined) {
const finalFormat = finalFormatCache.get(job);
if (
import_assertions.type == null ||
(import_assertions.type === 'json' && finalFormat === 'json')
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this always check if the finalFormat is 'json' and not skip it if it is nullish?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If import_assertions.type is nullish (I.e. assertionless import) and the previous job succeeded, we return the cached module. Because there was no assertion made, there’s no reason to check what format the module was loaded with. If/when we want to force assertion for some format, we would need to add some additional logic, but that’s not what this PR aims to do.

shouldn't this always check if the finalFormat is 'json'

we still want non-JSON modules to load (such as module, commonjs, etc.), I’m not sure I understand what you mean by that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If import_assertions.type is nullish (I.e. assertionless import) and the previous job succeeded, we return the cached module. Because there was no assertion made, there’s no reason to check what format the module was loaded with. If/when we want to force assertion for some format, we would need to add some additional logic, but that’s not what this PR aims to do.

shouldn't this always check if the finalFormat is 'json'

we still want non-JSON modules to load (such as module, commonjs, etc.), I’m not sure I understand what you mean by that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I think I get what you mean, indeed because json is the only valid value for an assertion type for now, indeed json is the only format acceptable here. I think we should keep the code ready for more values to be added, so when new assertion types are introduced, it won’t have to refactor the whole implementation.

Copy link
Member

Choose a reason for hiding this comment

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

the problem is that we have 2 cache entries coalescing to the same module job and that makes me trying to figure out how a userland loader could recreate that behavior very complicated and I am definitely not confident in that collision being safe even with the mitigation I mentioned and requested above. currently we always have 1-1 fully resolved cache key to module instance. The concern is the many-1 model that we introduced by coalescing.

) return job;
throw new ERR_FAILED_IMPORT_ASSERTION(
url, 'type', import_assertions.type, finalFormat);
}
}

const moduleProvider = async (url, isMain) => {
const { format: finalFormat, source } = await this.load(url, { format });

if (import_assertions.type === 'json' && finalFormat !== 'json') {
throw new ERR_FAILED_IMPORT_ASSERTION(
url, 'type', import_assertions.type, finalFormat);
}
finalFormatCache.set(job, finalFormat);

const translator = translators.get(finalFormat);

if (!translator) throw new ERR_UNKNOWN_MODULE_FORMAT(finalFormat);
Expand All @@ -249,6 +290,7 @@ class ESMLoader {
inspectBrk
);

importAssertionTypeCache.set(job, import_assertions.type);
this.moduleMap.set(url, job);

return job;
Expand All @@ -262,18 +304,19 @@ class ESMLoader {
* loader module.
*
* @param {string | string[]} specifiers Path(s) to the module
* @param {string} [parentURL] Path of the parent importing the module
* @returns {object | object[]} A list of module export(s)
* @param {string} parentURL Path of the parent importing the module
* @param {Record<string, Record<string, string>>} import_assertions
* @returns {Promise<object | object[]>} A list of module export(s)
*/
async import(specifiers, parentURL) {
async import(specifiers, parentURL, import_assertions) {
const wasArr = ArrayIsArray(specifiers);
if (!wasArr) specifiers = [specifiers];

const count = specifiers.length;
const jobs = new Array(count);

for (let i = 0; i < count; i++) {
jobs[i] = this.getModuleJob(specifiers[i], parentURL)
jobs[i] = this.getModuleJob(specifiers[i], parentURL, import_assertions)
.then((job) => job.run())
.then(({ module }) => module.getNamespace());
}
Expand Down
Loading