module: allow JSONC imports

doc/api/esm.md

@@ -262,6 +262,9 @@ added:
   - v17.1.0
   - v16.14.0
 changes:
+  - version: REPLACEME
+    pr-url: TODO
+    description: JSONC can now be used as a valid type attribute.
   - version:
     - v21.0.0
     - v20.10.0
@@ -289,9 +292,10 @@ const { default: barData } =
 Node.js supports the following `type` values, for which the attribute is
 mandatory:
 
-| Attribute `type` | Needed for       |
-| ---------------- | ---------------- |
-| `'json'`         | [JSON modules][] |
+| Attribute `type`                           | Used with        |
+| ------------------------------------------ | ---------------- |
+| `'json'`                                   | [JSON modules][] |
+| `'jsonc'` <sup><i>(Non-Standard)</i></sup> | [JSON modules][] |
 
 ## Builtin modules
 

lib/internal/modules/esm/assert.js

@@ -25,11 +25,11 @@ const kImplicitAssertType = 'javascript';
  */
 const formatTypeMap = {
   '__proto__': null,
-  'builtin': kImplicitAssertType,
-  'commonjs': kImplicitAssertType,
-  'json': 'json',
-  'module': kImplicitAssertType,
-  'wasm': kImplicitAssertType, // It's unclear whether the HTML spec will require an attribute type or not for Wasm; see https://github.com/WebAssembly/esm-integration/issues/42
+  'builtin': [kImplicitAssertType],
+  'commonjs': [kImplicitAssertType],
+  'json': ['json', 'jsonc'],
+  'module': [kImplicitAssertType],
+  'wasm': [kImplicitAssertType], // It's unclear whether the HTML spec will require an attribute type or not for Wasm; see https://github.com/WebAssembly/esm-integration/issues/42
 };
 
 /**
@@ -62,33 +62,30 @@ function validateAttributes(url, format,
   }
   const validType = formatTypeMap[format];
 
-  switch (validType) {
-    case undefined:
-      // Ignore attributes for module formats we don't recognize, to allow new
-      // formats in the future.
-      return true;
-
-    case kImplicitAssertType:
-      // This format doesn't allow an import assertion type, so the property
-      // must not be set on the import attributes object.
-      if (!ObjectPrototypeHasOwnProperty(importAttributes, 'type')) {
-        return true;
-      }
-      return handleInvalidType(url, importAttributes.type);
+  if (validType === undefined) {
+    return true;
+  }
 
-    case importAttributes.type:
-      // The asserted type is the valid type for this format.
+  if (validType[0] === kImplicitAssertType && validType.length === 1) {
+    // This format doesn't allow an import assertion type, so the property
+    // must not be set on the import attributes object.
+    if (!ObjectPrototypeHasOwnProperty(importAttributes, 'type')) {
       return true;
+    }
+    return handleInvalidType(url, importAttributes.type);
+  }
+
+  if (ArrayPrototypeIncludes(validType, importAttributes.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
-      // `importAttributes.type` might not have been it.
-      if (!ObjectPrototypeHasOwnProperty(importAttributes, 'type')) {
-        // `type` wasn't specified at all.
-        throw new ERR_IMPORT_ATTRIBUTE_MISSING(url, 'type', validType);
-      }
-      return handleInvalidType(url, importAttributes.type);
+  if (!ObjectPrototypeHasOwnProperty(importAttributes, 'type')) {
+    // `type` wasn't specified at all.
+    throw new ERR_IMPORT_ATTRIBUTE_MISSING(url, 'type', validType);
   }
+
+  return handleInvalidType(url, importAttributes.type);
 }
 
 /**

lib/internal/modules/esm/formats.js

@@ -17,6 +17,7 @@ const extensionFormatMap = {
   '.cjs': 'commonjs',
   '.js': 'module',
   '.json': 'json',
+  '.jsonc': 'jsonc',
   '.mjs': 'module',
 };
 

lib/internal/modules/esm/load.js

@@ -152,6 +152,14 @@ async function defaultLoad(url, context = kEmptyObject) {
 
   validateAttributes(url, format, importAttributes);
 
+  // Allow for .json / .jsonc to be used interchangeably with the json format.
+  if (format === 'json' || format === 'jsonc') {
+    const type = importAttributes?.type;
+    if (type) {
+      format = type;
+    }
+  }
+
   // Use the synchronous commonjs translator which can deal with cycles.
   if (format === 'commonjs' && getOptionValue('--experimental-require-module')) {
     format = 'commonjs-sync';

lib/internal/modules/esm/translators.js

@@ -8,6 +8,7 @@ const {
   ObjectKeys,
   ObjectPrototypeHasOwnProperty,
   ReflectApply,
+  RegExpPrototypeSymbolReplace,
   SafeArrayIterator,
   SafeMap,
   SafeSet,
@@ -87,7 +88,7 @@ async function initCJSParse() {
     initCJSParseSync();
   } else {
     const { parse, init } =
-        require('internal/deps/cjs-module-lexer/dist/lexer');
+      require('internal/deps/cjs-module-lexer/dist/lexer');
     try {
       await init();
       cjsParse = parse;
@@ -177,7 +178,7 @@ translators.set('module', function moduleStrategy(url, source, isMain) {
  */
 function enrichCJSError(err, content, filename) {
   if (err != null && ObjectGetPrototypeOf(err) === SyntaxErrorPrototype &&
-      containsModuleSyntax(content, filename)) {
+    containsModuleSyntax(content, filename)) {
     // Emit the warning synchronously because we are in the middle of handling
     // a SyntaxError that will throw and likely terminate the process before an
     // asynchronous warning would be emitted.
@@ -228,7 +229,7 @@ function loadCJSModule(module, source, url, filename) {
         case '.node':
           return CJSModule._load(specifier, module);
         default:
-            // fall through
+        // fall through
       }
       specifier = `${pathToFileURL(path)}`;
     }
@@ -248,8 +249,7 @@ function loadCJSModule(module, source, url, filename) {
   });
   setOwnProperty(requireFn, 'main', process.mainModule);
 
-  ReflectApply(compiledWrapper, module.exports,
-               [module.exports, requireFn, module, filename, __dirname]);
+  ReflectApply(compiledWrapper, module.exports, [module.exports, requireFn, module, filename, __dirname]);
   setOwnProperty(module, 'loaded', true);
 }
 
@@ -295,7 +295,7 @@ function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) {
     }
     for (const exportName of exportNames) {
       if (!ObjectPrototypeHasOwnProperty(exports, exportName) ||
-          exportName === 'default') {
+        exportName === 'default') {
         continue;
       }
       // We might trigger a getter -> dont fail.
@@ -417,7 +417,7 @@ function cjsPreparseModuleExports(filename, source) {
       // `format: 'commonjs'` instead of relying on file extensions.
       const ext = extname(resolved);
       if ((ext === '.js' || ext === '.cjs' || !CJSModule._extensions[ext]) &&
-      isAbsolute(resolved)) {
+        isAbsolute(resolved)) {
         // TODO: this should be calling the `load` hook chain to get the source
         // (and fallback to reading the FS only if the source is nullish).
         const source = readFileSync(resolved, 'utf-8');
@@ -449,65 +449,80 @@ translators.set('builtin', function builtinStrategy(url) {
 
 // Strategy for loading a JSON file
 const isWindows = process.platform === 'win32';
-translators.set('json', function jsonStrategy(url, source) {
-  emitExperimentalWarning('Importing JSON modules');
-  assertBufferSource(source, true, 'load');
-  debug(`Loading JSONModule ${url}`);
-  const pathname = StringPrototypeStartsWith(url, 'file:') ?
-    fileURLToPath(url) : null;
-  const shouldCheckAndPopulateCJSModuleCache =
-    // We want to involve the CJS loader cache only for `file:` URL with no search query and no hash.
-    pathname && !StringPrototypeIncludes(url, '?') && !StringPrototypeIncludes(url, '#');
-  let modulePath;
-  let module;
-  if (shouldCheckAndPopulateCJSModuleCache) {
-    modulePath = isWindows ?
-      StringPrototypeReplaceAll(pathname, '/', '\\') : pathname;
-    module = CJSModule._cache[modulePath];
-    if (module && module.loaded) {
-      const exports = module.exports;
-      return new ModuleWrap(url, undefined, ['default'], function() {
-        this.setExport('default', exports);
-      });
+function jsonStrategyBuilder(parser) {
+  return function jsonStrategy(url, source) {
+    emitExperimentalWarning('Importing JSON modules');
+    assertBufferSource(source, true, 'load');
+    debug(`Loading JSONModule ${url}`);
+    const pathname = StringPrototypeStartsWith(url, 'file:') ?
+      fileURLToPath(url) : null;
+    const shouldCheckAndPopulateCJSModuleCache =
+      // We want to involve the CJS loader cache only for `file:` URL with no search query and no hash.
+      pathname && !StringPrototypeIncludes(url, '?') && !StringPrototypeIncludes(url, '#');
+    let modulePath;
+    let module;
+    if (shouldCheckAndPopulateCJSModuleCache) {
+      modulePath = isWindows ?
+        StringPrototypeReplaceAll(pathname, '/', '\\') : pathname;
+      module = CJSModule._cache[modulePath];
+      if (module && module.loaded) {
+        const exports = module.exports;
+        return new ModuleWrap(url, undefined, ['default'], function() {
+          this.setExport('default', exports);
+        });
+      }
     }
-  }
-  source = stringify(source);
-  if (shouldCheckAndPopulateCJSModuleCache) {
-    // A require call could have been called on the same file during loading and
-    // that resolves synchronously. To make sure we always return the identical
-    // export, we have to check again if the module already exists or not.
-    // TODO: remove CJS loader from here as well.
-    module = CJSModule._cache[modulePath];
-    if (module && module.loaded) {
-      const exports = module.exports;
-      return new ModuleWrap(url, undefined, ['default'], function() {
-        this.setExport('default', exports);
-      });
+    source = stringify(source);
+    if (shouldCheckAndPopulateCJSModuleCache) {
+      // A require call could have been called on the same file during loading and
+      // that resolves synchronously. To make sure we always return the identical
+      // export, we have to check again if the module already exists or not.
+      // TODO: remove CJS loader from here as well.
+      module = CJSModule._cache[modulePath];
+      if (module && module.loaded) {
+        const exports = module.exports;
+        return new ModuleWrap(url, undefined, ['default'], function() {
+          this.setExport('default', exports);
+        });
+      }
     }
-  }
-  try {
-    const exports = JSONParse(stripBOM(source));
-    module = {
-      exports,
-      loaded: true,
-    };
-  } catch (err) {
-    // TODO (BridgeAR): We could add a NodeCore error that wraps the JSON
-    // parse error instead of just manipulating the original error message.
-    // That would allow to add further properties and maybe additional
-    // debugging information.
-    err.message = errPath(url) + ': ' + err.message;
-    throw err;
-  }
-  if (shouldCheckAndPopulateCJSModuleCache) {
-    CJSModule._cache[modulePath] = module;
-  }
-  cjsCache.set(url, module);
-  return new ModuleWrap(url, undefined, ['default'], function() {
-    debug(`Parsing JSONModule ${url}`);
-    this.setExport('default', module.exports);
-  });
-});
+    try {
+      const exports = parser(stripBOM(source));
+      module = {
+        exports,
+        loaded: true,
+      };
+    } catch (err) {
+      // TODO (BridgeAR): We could add a NodeCore error that wraps the JSON
+      // parse error instead of just manipulating the original error message.
+      // That would allow to add further properties and maybe additional
+      // debugging information.
+      err.message = errPath(url) + ': ' + err.message;
+      throw err;
+    }
+    if (shouldCheckAndPopulateCJSModuleCache) {
+      CJSModule._cache[modulePath] = module;
+    }
+    cjsCache.set(url, module);
+    return new ModuleWrap(url, undefined, ['default'], function() {
+      debug(`Parsing JSONModule ${url}`);
+      this.setExport('default', module.exports);
+    });
+  };
+}
+
+// This RegExp matches all comments into matching group $2
+// and all strings into matching group $1. This way, we can replace
+// the entire match with $1, which will preserve strings (including
+// strings containing comments), and remove comments.
+//
+// eslint-disable-next-line node-core/no-unescaped-regexp-dot
+const JSONAntiCommentRE = /("(?:\\.|[^"\\])*?")|(\/\*[^]*?\*\/)|\/\/.*/g;
+
+translators.set('json', jsonStrategyBuilder(JSONParse));
+translators.set('jsonc', jsonStrategyBuilder(function jsoncParse(source) {
+  return JSONParse(RegExpPrototypeSymbolReplace(JSONAntiCommentRE, source, '$1'));
+}));
 
 // Strategy for loading a wasm module
 translators.set('wasm', async function(url, source) {
@@ -528,8 +543,8 @@ translators.set('wasm', async function(url, source) {
   }
 
   const imports =
-      ArrayPrototypeMap(WebAssembly.Module.imports(compiled),
-                        ({ module }) => module);
+    ArrayPrototypeMap(WebAssembly.Module.imports(compiled),
+                      ({ module }) => module);
   const exports =
     ArrayPrototypeMap(WebAssembly.Module.exports(compiled),
                       ({ name }) => name);

test/es-module/test-import-jsonc.js

@@ -0,0 +1,22 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+
+const jsonFiles = [
+  // [file, [throwWithTypeJSON, throwWithTypeJSONC]]
+  ['../fixtures/es-modules/json-with-comments/regular.json', [false, false]],
+  ['../fixtures/es-modules/json-with-comments/comments.json', [true, false]],
+  ['../fixtures/es-modules/json-with-comments/comments.jsonc', [true, false]],
+];
+
+for (const [file, [throwWithTypeJSON, throwWithTypeJSONC]] of jsonFiles) {
+
+  assert[throwWithTypeJSON ? 'rejects' : 'doesNotReject'](async () => {
+    await import(file, { with: { type: 'json' } });
+  });
+
+  assert[throwWithTypeJSONC ? 'rejects' : 'doesNotReject'](async () => {
+    await import(file, { with: { type: 'jsonc' } });
+  });
+}

test/fixtures/es-modules/json-with-comments/comments.json

@@ -0,0 +1,6 @@
+{
+    "key": "value"
+    // Some comment
+    /* Also
+    a comment */
+}

test/fixtures/es-modules/json-with-comments/comments.jsonc

@@ -0,0 +1,6 @@
+{
+    "key": "value"
+    // Some comment
+    /* Also
+    a comment */
+}

test/fixtures/es-modules/json-with-comments/regular.json

@@ -0,0 +1,3 @@
+{
+    "key": "value"
+}