module: import assertions improvements

PR-URL: nodejs#40785 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
aduh95 · Jan 30, 2022 · b8cacb6 · b8cacb6
1 parent 94e2d10
commit b8cacb6
Show file tree

Hide file tree

Showing 6 changed files with 41 additions and 31 deletions.
diff --git a/doc/api/esm.md b/doc/api/esm.md
@@ -225,6 +225,8 @@ import fs from 'node:fs/promises';
 added: REPLACEME
 -->
 
+> Stability: 1 - Experimental
+
 The [Import Assertions proposal][] adds an inline syntax for module import
 statements to pass on more information alongside the module specifier.
 
@@ -235,11 +237,12 @@ const { default: barData } =
   await import('./bar.json', { assert: { type: 'json' } });
 ```
 
-Node.js supports the following `type` values:
+Node.js supports the following `type` values, for which the assertion is
+mandatory:
 
-| `type`   | Resolves to      |
-| -------- | ---------------- |
-| `'json'` | [JSON modules][] |
+| Assertion `type` | Needed for       |
+| ---------------- | ---------------- |
+| `'json'`         | [JSON modules][] |
 
 ## Builtin modules
 
@@ -548,6 +551,8 @@ node index.mjs # fails
 node --experimental-json-modules index.mjs # works
 ```
 
+The `assert { type: 'json' }` syntax is mandatory; see [Import Assertions][].
+
 <i id="esm_experimental_wasm_modules"></i>
 
 ## Wasm modules
@@ -1406,6 +1411,7 @@ 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 WebAssembly]: https://github.com/webassembly/esm-integration
+[Import Assertions]: #import-assertions
 [Import Assertions proposal]: https://github.com/tc39/proposal-import-assertions
 [JSON modules]: #json-modules
 [Node.js Module Resolution Algorithm]: #resolver-algorithm-specification

diff --git a/lib/internal/modules/esm/assert.js b/lib/internal/modules/esm/assert.js
@@ -1,11 +1,11 @@
 'use strict';
 
 const {
+  ArrayPrototypeFilter,
   ArrayPrototypeIncludes,
   ObjectCreate,
   ObjectValues,
   ObjectPrototypeHasOwnProperty,
-  Symbol,
 } = primordials;
 const { validateString } = require('internal/validators');
 
@@ -15,24 +15,32 @@ const {
   ERR_IMPORT_ASSERTION_TYPE_UNSUPPORTED,
 } = require('internal/errors').codes;
 
-const kImplicitAssertType = Symbol('implicit assert type');
+// The HTML spec has an implied default type of `'javascript'`.
+const kImplicitAssertType = 'javascript';
 
 /**
- * Define a map of module formats to import assertion types (the value of `type`
- * in `assert { type: 'json' }`).
- * @type {Map<string, string | typeof kImplicitAssertType}
+ * Define a map of module formats to import assertion types (the value of
+ * `type` in `assert { type: 'json' }`).
+ * @type {Map<string, string>}
  */
 const formatTypeMap = {
   '__proto__': null,
   'builtin': kImplicitAssertType,
   'commonjs': kImplicitAssertType,
   'json': 'json',
   'module': kImplicitAssertType,
-  'wasm': kImplicitAssertType, // Should probably be 'webassembly' per https://github.com/tc39/proposal-import-assertions
+  'wasm': kImplicitAssertType, // It's unclear whether the HTML spec will require an assertion type or not for Wasm; see https://github.com/WebAssembly/esm-integration/issues/42
 };
 
-/** @type {Array<string, string | typeof kImplicitAssertType} */
-const supportedAssertionTypes = ObjectValues(formatTypeMap);
+/**
+ * The HTML spec disallows the default type to be explicitly specified
+ * (for now); so `import './file.js'` is okay but
+ * `import './file.js' assert { type: 'javascript' }` throws.
+ * @type {Array<string, string>}
+ */
+const supportedAssertionTypes = ArrayPrototypeFilter(
+  ObjectValues(formatTypeMap),
+  (type) => type !== kImplicitAssertType);
 
 
 /**
@@ -50,14 +58,10 @@ function validateAssertions(url, format,
 
   switch (validType) {
     case undefined:
-      // Ignore assertions for module types we don't recognize, to allow new
+      // Ignore assertions for module formats we don't recognize, to allow new
       // formats in the future.
       return true;
 
-    case importAssertions.type:
-      // The asserted type is the valid type for this format.
-      return true;
-
     case kImplicitAssertType:
       // This format doesn't allow an import assertion type, so the property
       // must not be set on the import assertions object.
@@ -66,9 +70,13 @@ function validateAssertions(url, format,
       }
       return handleInvalidType(url, importAssertions.type);
 
+    case importAssertions.type:
+      // The asserted type is the valid type for this format.
+      return true;
+
     default:
       // There is an expected type for this format, but the value of
-      // `importAssertions.type` was not it.
+      // `importAssertions.type` might not have been it.
       if (!ObjectPrototypeHasOwnProperty(importAssertions, 'type')) {
         // `type` wasn't specified at all.
         throw new ERR_IMPORT_ASSERTION_TYPE_MISSING(url, validType);
@@ -86,7 +94,7 @@ function handleInvalidType(url, type) {
   // `type` might have not been a string.
   validateString(type, 'type');
 
-  // `type` was not one of the types we understand.
+  // `type` might not have been one of the types we understand.
   if (!ArrayPrototypeIncludes(supportedAssertionTypes, type)) {
     throw new ERR_IMPORT_ASSERTION_TYPE_UNSUPPORTED(type);
   }

diff --git a/lib/internal/modules/esm/module_map.js b/lib/internal/modules/esm/module_map.js
@@ -12,20 +12,17 @@ let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
 const { ERR_INVALID_ARG_TYPE } = require('internal/errors').codes;
 const { validateString } = require('internal/validators');
 
-const validateAssertType = (type) =>
-  type === kImplicitAssertType || validateString(type, 'type');
-
 // Tracks the state of the loader-level module cache
 class ModuleMap extends SafeMap {
   constructor(i) { super(i); } // eslint-disable-line no-useless-constructor
   get(url, type = kImplicitAssertType) {
     validateString(url, 'url');
-    validateAssertType(type);
+    validateString(type, 'type');
     return super.get(url)?.[type];
   }
   set(url, type = kImplicitAssertType, job) {
     validateString(url, 'url');
-    validateAssertType(type);
+    validateString(type, 'type');
     if (job instanceof ModuleJob !== true &&
         typeof job !== 'function') {
       throw new ERR_INVALID_ARG_TYPE('job', 'ModuleJob', job);
@@ -39,7 +36,7 @@ class ModuleMap extends SafeMap {
   }
   has(url, type = kImplicitAssertType) {
     validateString(url, 'url');
-    validateAssertType(type);
+    validateString(type, 'type');
     return super.get(url)?.[type] !== undefined;
   }
 }

diff --git a/test/es-module/test-esm-import-assertion-errors.js b/test/es-module/test-esm-import-assertion-errors.js
@@ -25,7 +25,7 @@ async function test() {
   );
 
   await rejects(
-    import('data:text/javascript,', { assert: { type: 'unsupported' } }),
+    import(jsModuleDataUrl, { assert: { type: 'unsupported' } }),
     { code: 'ERR_IMPORT_ASSERTION_TYPE_UNSUPPORTED' }
   );
 

diff --git a/test/es-module/test-esm-import-assertion-validation.js b/test/es-module/test-esm-import-assertion-validation.js
@@ -22,8 +22,8 @@ assert.throws(() => validateAssertions(url, 'module', { type: 'json' }), {
   code: 'ERR_IMPORT_ASSERTION_TYPE_FAILED',
 });
 
-// This should be allowed according to HTML spec. Let's keep it disabled
-// until WASM module import is sorted out.
+// The HTML spec specifically disallows this for now, while Wasm module import
+// and whether it will require a type assertion is still an open question.
 assert.throws(() => validateAssertions(url, 'module', { type: 'javascript' }), {
   code: 'ERR_IMPORT_ASSERTION_TYPE_UNSUPPORTED',
 });

diff --git a/test/es-module/test-esm-loader-modulemap.js b/test/es-module/test-esm-loader-modulemap.js
@@ -7,7 +7,6 @@ const { strictEqual, throws } = require('assert');
 const { ESMLoader } = require('internal/modules/esm/loader');
 const ModuleMap = require('internal/modules/esm/module_map');
 const ModuleJob = require('internal/modules/esm/module_job');
-const { kImplicitAssertType } = require('internal/modules/esm/assert');
 const createDynamicModule = require(
   'internal/modules/esm/create_dynamic_module');
 
@@ -38,14 +37,14 @@ const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module,
   strictEqual(moduleMap.get(jsonModuleDataUrl, 'json'), jsonModuleJob);
 
   strictEqual(moduleMap.has(jsModuleDataUrl), true);
-  strictEqual(moduleMap.has(jsModuleDataUrl, kImplicitAssertType), true);
+  strictEqual(moduleMap.has(jsModuleDataUrl, 'javascript'), true);
   strictEqual(moduleMap.has(jsonModuleDataUrl, 'json'), true);
 
   strictEqual(moduleMap.has('unknown'), false);
 
   // The types must match
   strictEqual(moduleMap.has(jsModuleDataUrl, 'json'), false);
-  strictEqual(moduleMap.has(jsonModuleDataUrl, kImplicitAssertType), false);
+  strictEqual(moduleMap.has(jsonModuleDataUrl, 'javascript'), false);
   strictEqual(moduleMap.has(jsonModuleDataUrl), false);
   strictEqual(moduleMap.has(jsModuleDataUrl, 'unknown'), false);
   strictEqual(moduleMap.has(jsonModuleDataUrl, 'unknown'), false);