From 8feb2dbaa4d53ab2d0d9c6592ef4495b6115ddc9 Mon Sep 17 00:00:00 2001 From: Francesco Trotta Date: Wed, 29 Nov 2023 10:21:54 +0100 Subject: [PATCH] esm: do not call `getSource` when format is `commonjs` Ensure that `defaultLoad` does not uselessly access the file system to get the source of modules that are known to be in CommonJS format. This allows CommonJS imports to resolve in the current phase of the event loop. Refs: https://github.com/eslint/eslint/pull/17683 PR-URL: https://github.com/nodejs/node/pull/50465 Reviewed-By: James M Snell Reviewed-By: Antoine du Hamel --- lib/internal/modules/esm/load.js | 19 +++++++++-------- .../test-esm-dynamic-import-commonjs.js | 19 +++++++++++++++++ .../test-esm-dynamic-import-commonjs.mjs | 9 ++++++++ .../es-module/test-esm-loader-with-source.mjs | 10 +++++++++ test/fixtures/empty.cjs | 0 .../es-module-loaders/preset-cjs-source.mjs | 21 +++++++++++++++++++ 6 files changed, 69 insertions(+), 9 deletions(-) create mode 100644 test/es-module/test-esm-dynamic-import-commonjs.js create mode 100644 test/es-module/test-esm-dynamic-import-commonjs.mjs create mode 100644 test/es-module/test-esm-loader-with-source.mjs create mode 100644 test/fixtures/empty.cjs create mode 100644 test/fixtures/es-module-loaders/preset-cjs-source.mjs diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js index bcebc283828ad5..70d1b7fe83fa35 100644 --- a/lib/internal/modules/esm/load.js +++ b/lib/internal/modules/esm/load.js @@ -132,20 +132,21 @@ async function defaultLoad(url, context = kEmptyObject) { if (urlInstance.protocol === 'node:') { source = null; format ??= 'builtin'; - } else { - let contextToPass = context; + } else if (format !== 'commonjs' || defaultType === 'module') { if (source == null) { ({ responseURL, source } = await getSource(urlInstance, context)); - contextToPass = { __proto__: context, source }; + context = { __proto__: context, source }; } - // Now that we have the source for the module, run `defaultGetFormat` again in case we detect ESM syntax. - format ??= await defaultGetFormat(urlInstance, contextToPass); + if (format == null) { + // Now that we have the source for the module, run `defaultGetFormat` to detect its format. + format = await defaultGetFormat(urlInstance, context); - if (format === 'commonjs' && contextToPass !== context && defaultType !== 'module') { - // For backward compatibility reasons, we need to discard the source in - // order for the CJS loader to re-fetch it. - source = null; + if (format === 'commonjs') { + // For backward compatibility reasons, we need to discard the source in + // order for the CJS loader to re-fetch it. + source = null; + } } } diff --git a/test/es-module/test-esm-dynamic-import-commonjs.js b/test/es-module/test-esm-dynamic-import-commonjs.js new file mode 100644 index 00000000000000..9022b83f1fcffa --- /dev/null +++ b/test/es-module/test-esm-dynamic-import-commonjs.js @@ -0,0 +1,19 @@ +'use strict'; + +const common = require('../common'); +const fixtures = require('../common/fixtures'); +const assert = require('node:assert'); + +(async () => { + + // Make sure that the CommonJS module lexer has been initialized. + // See https://github.com/nodejs/node/blob/v21.1.0/lib/internal/modules/esm/translators.js#L61-L81. + await import(fixtures.fileURL('empty.js')); + + let tickDuringCJSImport = false; + process.nextTick(() => { tickDuringCJSImport = true; }); + await import(fixtures.fileURL('empty.cjs')); + + assert(!tickDuringCJSImport); + +})().then(common.mustCall()); diff --git a/test/es-module/test-esm-dynamic-import-commonjs.mjs b/test/es-module/test-esm-dynamic-import-commonjs.mjs new file mode 100644 index 00000000000000..6a8f2506c0eb05 --- /dev/null +++ b/test/es-module/test-esm-dynamic-import-commonjs.mjs @@ -0,0 +1,9 @@ +import '../common/index.mjs'; +import fixtures from '../common/fixtures.js'; +import assert from 'node:assert'; + +let tickDuringCJSImport = false; +process.nextTick(() => { tickDuringCJSImport = true; }); +await import(fixtures.fileURL('empty.cjs')); + +assert(!tickDuringCJSImport); diff --git a/test/es-module/test-esm-loader-with-source.mjs b/test/es-module/test-esm-loader-with-source.mjs new file mode 100644 index 00000000000000..41dca12dfc6bab --- /dev/null +++ b/test/es-module/test-esm-loader-with-source.mjs @@ -0,0 +1,10 @@ +// Flags: --experimental-loader ./test/fixtures/es-module-loaders/preset-cjs-source.mjs +import '../common/index.mjs'; +import * as fixtures from '../common/fixtures.mjs'; +import assert from 'assert'; + +const { default: existingFileSource } = await import(fixtures.fileURL('es-modules', 'cjs-file.cjs')); +const { default: noSuchFileSource } = await import(new URL('./no-such-file.cjs', import.meta.url)); + +assert.strictEqual(existingFileSource, 'no .cjs file was read to get this source'); +assert.strictEqual(noSuchFileSource, 'no .cjs file was read to get this source'); diff --git a/test/fixtures/empty.cjs b/test/fixtures/empty.cjs new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/test/fixtures/es-module-loaders/preset-cjs-source.mjs b/test/fixtures/es-module-loaders/preset-cjs-source.mjs new file mode 100644 index 00000000000000..2cfa851a23f889 --- /dev/null +++ b/test/fixtures/es-module-loaders/preset-cjs-source.mjs @@ -0,0 +1,21 @@ +export function resolve(specifier, context, next) { + if (specifier.endsWith('/no-such-file.cjs')) { + // Shortcut to avoid ERR_MODULE_NOT_FOUND for non-existing file, but keep the url for the load hook. + return { + shortCircuit: true, + url: specifier, + }; + } + return next(specifier); +} + +export function load(href, context, next) { + if (href.endsWith('.cjs')) { + return { + format: 'commonjs', + shortCircuit: true, + source: 'module.exports = "no .cjs file was read to get this source";', + }; + } + return next(href); +}