From fa1a8d254aeefece234f12b2afffcd3f09a84762 Mon Sep 17 00:00:00 2001 From: Francesco Trotta Date: Sat, 28 Oct 2023 20:40:35 +0200 Subject: [PATCH 1/6] 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 --- lib/internal/modules/esm/load.js | 19 ++++++------- .../test-esm-dynamic-import-commonjs.js | 27 +++++++++++++++++++ .../test-esm-dynamic-import-commonjs.mjs | 16 +++++++++++ 3 files changed, 53 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 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..134e6898c71cde --- /dev/null +++ b/test/es-module/test-esm-dynamic-import-commonjs.js @@ -0,0 +1,27 @@ +'use strict'; + +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +const assert = require('node:assert'); +const fs = require('node:fs/promises'); + +tmpdir.refresh(); + +const preloadURL = tmpdir.fileURL('preload.cjs'); +const commonjsURL = tmpdir.fileURL('commonjs-module-1.cjs'); + +(async () => { + + await Promise.all([fs.writeFile(preloadURL, ''), fs.writeFile(commonjsURL, '')]); + + // 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(preloadURL); + + let tickDuringCJSImport = false; + process.nextTick(() => { tickDuringCJSImport = true; }); + await import(commonjsURL); + + 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..461afa35eb1574 --- /dev/null +++ b/test/es-module/test-esm-dynamic-import-commonjs.mjs @@ -0,0 +1,16 @@ +import '../common/index.mjs'; +import tmpdir from '../common/tmpdir.js'; +import assert from 'node:assert'; +import fs from 'node:fs/promises'; + +tmpdir.refresh(); + +const commonjsURL = tmpdir.fileURL('commonjs-module-2.cjs'); + +await fs.writeFile(commonjsURL, ''); + +let tickDuringCJSImport = false; +process.nextTick(() => { tickDuringCJSImport = true; }); +await import(commonjsURL); + +assert(!tickDuringCJSImport); From 7c420ad5ae2f1446541ff8cf60222d598739c892 Mon Sep 17 00:00:00 2001 From: Francesco Trotta Date: Sun, 29 Oct 2023 11:33:34 +0100 Subject: [PATCH 2/6] fix lint problems --- .../test-esm-dynamic-import-commonjs.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/es-module/test-esm-dynamic-import-commonjs.js b/test/es-module/test-esm-dynamic-import-commonjs.js index 134e6898c71cde..44d0f475f8e707 100644 --- a/test/es-module/test-esm-dynamic-import-commonjs.js +++ b/test/es-module/test-esm-dynamic-import-commonjs.js @@ -12,16 +12,16 @@ const commonjsURL = tmpdir.fileURL('commonjs-module-1.cjs'); (async () => { - await Promise.all([fs.writeFile(preloadURL, ''), fs.writeFile(commonjsURL, '')]); + await Promise.all([fs.writeFile(preloadURL, ''), fs.writeFile(commonjsURL, '')]); - // 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(preloadURL); + // 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(preloadURL); - let tickDuringCJSImport = false; - process.nextTick(() => { tickDuringCJSImport = true; }); - await import(commonjsURL); - - assert(!tickDuringCJSImport); + let tickDuringCJSImport = false; + process.nextTick(() => { tickDuringCJSImport = true; }); + await import(commonjsURL); + + assert(!tickDuringCJSImport); })().then(common.mustCall()); From 6dcb6688f80a444d5bff71b2b4ba6adb6fce21a4 Mon Sep 17 00:00:00 2001 From: Francesco Trotta Date: Mon, 6 Nov 2023 17:03:32 +0100 Subject: [PATCH 3/6] use fixtures in tests --- test/es-module/test-esm-dynamic-import-commonjs.js | 14 +++----------- .../es-module/test-esm-dynamic-import-commonjs.mjs | 11 ++--------- test/fixtures/empty.cjs | 0 3 files changed, 5 insertions(+), 20 deletions(-) create mode 100644 test/fixtures/empty.cjs diff --git a/test/es-module/test-esm-dynamic-import-commonjs.js b/test/es-module/test-esm-dynamic-import-commonjs.js index 44d0f475f8e707..9022b83f1fcffa 100644 --- a/test/es-module/test-esm-dynamic-import-commonjs.js +++ b/test/es-module/test-esm-dynamic-import-commonjs.js @@ -1,26 +1,18 @@ 'use strict'; const common = require('../common'); -const tmpdir = require('../common/tmpdir'); +const fixtures = require('../common/fixtures'); const assert = require('node:assert'); -const fs = require('node:fs/promises'); - -tmpdir.refresh(); - -const preloadURL = tmpdir.fileURL('preload.cjs'); -const commonjsURL = tmpdir.fileURL('commonjs-module-1.cjs'); (async () => { - await Promise.all([fs.writeFile(preloadURL, ''), fs.writeFile(commonjsURL, '')]); - // 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(preloadURL); + await import(fixtures.fileURL('empty.js')); let tickDuringCJSImport = false; process.nextTick(() => { tickDuringCJSImport = true; }); - await import(commonjsURL); + await import(fixtures.fileURL('empty.cjs')); assert(!tickDuringCJSImport); diff --git a/test/es-module/test-esm-dynamic-import-commonjs.mjs b/test/es-module/test-esm-dynamic-import-commonjs.mjs index 461afa35eb1574..6a8f2506c0eb05 100644 --- a/test/es-module/test-esm-dynamic-import-commonjs.mjs +++ b/test/es-module/test-esm-dynamic-import-commonjs.mjs @@ -1,16 +1,9 @@ import '../common/index.mjs'; -import tmpdir from '../common/tmpdir.js'; +import fixtures from '../common/fixtures.js'; import assert from 'node:assert'; -import fs from 'node:fs/promises'; - -tmpdir.refresh(); - -const commonjsURL = tmpdir.fileURL('commonjs-module-2.cjs'); - -await fs.writeFile(commonjsURL, ''); let tickDuringCJSImport = false; process.nextTick(() => { tickDuringCJSImport = true; }); -await import(commonjsURL); +await import(fixtures.fileURL('empty.cjs')); assert(!tickDuringCJSImport); diff --git a/test/fixtures/empty.cjs b/test/fixtures/empty.cjs new file mode 100644 index 00000000000000..e69de29bb2d1d6 From d21699ef5456d88133a2a1ef68f6b3fe003da119 Mon Sep 17 00:00:00 2001 From: Francesco Trotta Date: Tue, 7 Nov 2023 13:43:14 +0100 Subject: [PATCH 4/6] add test for loader-provided source --- .../es-module/test-esm-loader-with-source.mjs | 10 +++++++++ .../es-module-loaders/preset-cjs-source.mjs | 21 +++++++++++++++++++ 2 files changed, 31 insertions(+) create mode 100644 test/es-module/test-esm-loader-with-source.mjs create mode 100644 test/fixtures/es-module-loaders/preset-cjs-source.mjs 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..913e65262ba943 --- /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(fixtures.fileURL('no-such-file.cjs')); + +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/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); +} From 80d1ef16b4210b177476b80dac135c99b17ea88b Mon Sep 17 00:00:00 2001 From: Francesco Trotta Date: Wed, 8 Nov 2023 15:54:14 +0100 Subject: [PATCH 5/6] use absolute file URL in test --- test/es-module/test-esm-loader-with-source.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/es-module/test-esm-loader-with-source.mjs b/test/es-module/test-esm-loader-with-source.mjs index 913e65262ba943..a0214b208f0597 100644 --- a/test/es-module/test-esm-loader-with-source.mjs +++ b/test/es-module/test-esm-loader-with-source.mjs @@ -4,7 +4,7 @@ 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(fixtures.fileURL('no-such-file.cjs')); +const { default: noSuchFileSource } = await import('file:///no-such-file.cjs'); assert.strictEqual(existingFileSource, 'no .cjs file was read to get this source'); assert.strictEqual(noSuchFileSource, 'no .cjs file was read to get this source'); From 45c9a4af2b94b7fd6cf298623c6a3e8b5dd4fadd Mon Sep 17 00:00:00 2001 From: Francesco Trotta Date: Wed, 8 Nov 2023 21:04:53 +0100 Subject: [PATCH 6/6] use base URL in test --- test/es-module/test-esm-loader-with-source.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/es-module/test-esm-loader-with-source.mjs b/test/es-module/test-esm-loader-with-source.mjs index a0214b208f0597..41dca12dfc6bab 100644 --- a/test/es-module/test-esm-loader-with-source.mjs +++ b/test/es-module/test-esm-loader-with-source.mjs @@ -4,7 +4,7 @@ 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('file:///no-such-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');