From f2000b134e1e7dea610f4424ba8a0f10c3291219 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 1 Oct 2023 20:57:25 +0200 Subject: [PATCH 1/4] esm: bypass CJS loader in default load under `--default-type=module` This allows user to opt-out from using the monkey-patchable CJS loader, even to load CJS modules. --- doc/api/module.md | 22 +-- lib/internal/modules/esm/load.js | 4 +- test/es-module/test-esm-type-flag-errors.mjs | 126 ++++++++++++++---- .../fixtures/es-module-require-cache/echo.cjs | 1 + .../echo-require-cache.js | 1 + 5 files changed, 115 insertions(+), 39 deletions(-) create mode 100644 test/fixtures/es-module-require-cache/echo.cjs create mode 100644 test/fixtures/es-modules/package-type-commonjs/echo-require-cache.js diff --git a/doc/api/module.md b/doc/api/module.md index f4338028abe31d..88a6f030a03ec2 100644 --- a/doc/api/module.md +++ b/doc/api/module.md @@ -610,19 +610,21 @@ not possible to replace the value of a Node.js builtin (core) module. Omitting vs providing a `source` for `'commonjs'` has very different effects: -* When a `source` is provided, all `require` calls from this module will be - processed by the ESM loader with registered `resolve` and `load` hooks; all - `require.resolve` calls from this module will be processed by the ESM loader - with registered `resolve` hooks; only a subset of the CommonJS API will be - available (e.g. no `require.extensions`, no `require.cache`, no +* When a `source` is provided, or when running `node` with + `--experimental-default-type=module`, all `require` calls from this module + will be processed by the ESM loader with registered `resolve` and `load` + hooks; all `require.resolve` calls from this module will be processed by the + ESM loader with registered `resolve` hooks; only a subset of the CommonJS API + will be available (e.g. no `require.extensions`, no `require.cache`, no `require.resolve.paths`) and monkey-patching on the CommonJS module loader will not apply. -* If `source` is undefined or `null`, it will be handled by the CommonJS module - loader and `require`/`require.resolve` calls will not go through the - registered hooks. This behavior for nullish `source` is temporary — in the - future, nullish `source` will not be supported. +* If `source` is undefined or `null`, and `node` is run with + `--experimental-default-type=commonjs`, it will be handled by the CommonJS + module loader and `require`/`require.resolve` calls will not go through the + registered hooks. -The Node.js internal `load` implementation, which is the value of `next` for the +When `node` is run with `--experimental-default-type=commonjs`, the Node.js +internal `load` implementation, which is the value of `next` for the last hook in the `load` chain, returns `null` for `source` when `format` is `'commonjs'` for backward compatibility. Here is an example hook that would opt-in to using the non-default behavior: diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js index 6f9b73abd8a761..bcebc283828ad5 100644 --- a/lib/internal/modules/esm/load.js +++ b/lib/internal/modules/esm/load.js @@ -18,6 +18,8 @@ const policy = getOptionValue('--experimental-policy') ? null; const experimentalNetworkImports = getOptionValue('--experimental-network-imports'); +const defaultType = + getOptionValue('--experimental-default-type'); const { Buffer: { from: BufferFrom } } = require('buffer'); @@ -140,7 +142,7 @@ async function defaultLoad(url, context = kEmptyObject) { // 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 === 'commonjs' && contextToPass !== 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; diff --git a/test/es-module/test-esm-type-flag-errors.mjs b/test/es-module/test-esm-type-flag-errors.mjs index 6d54eff94763ef..1a4d438103f7d9 100644 --- a/test/es-module/test-esm-type-flag-errors.mjs +++ b/test/es-module/test-esm-type-flag-errors.mjs @@ -1,31 +1,101 @@ import { spawnPromisified } from '../common/index.mjs'; import * as fixtures from '../common/fixtures.mjs'; import { describe, it } from 'node:test'; -import { match, strictEqual } from 'node:assert'; - -describe('--experimental-default-type=module should not affect the interpretation of files with unknown extensions', - { concurrency: true }, () => { - it('should error on an entry point with an unknown extension', async () => { - const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ - '--experimental-default-type=module', - fixtures.path('es-modules/package-type-module/extension.unknown'), - ]); - - match(stderr, /ERR_UNKNOWN_FILE_EXTENSION/); - strictEqual(stdout, ''); - strictEqual(code, 1); - strictEqual(signal, null); - }); - - it('should error on an import with an unknown extension', async () => { - const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ - '--experimental-default-type=module', - fixtures.path('es-modules/package-type-module/imports-unknownext.mjs'), - ]); - - match(stderr, /ERR_UNKNOWN_FILE_EXTENSION/); - strictEqual(stdout, ''); - strictEqual(code, 1); - strictEqual(signal, null); - }); - }); +import { deepStrictEqual, match, strictEqual } from 'node:assert'; + +describe('--experimental-default-type=module', { concurrency: true }, () => { + describe('should not affect the interpretation of files with unknown extensions', { concurrency: true }, () => { + it('should error on an entry point with an unknown extension', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + fixtures.path('es-modules/package-type-module/extension.unknown'), + ]); + + match(stderr, /ERR_UNKNOWN_FILE_EXTENSION/); + strictEqual(stdout, ''); + strictEqual(code, 1); + strictEqual(signal, null); + }); + + it('should error on an import with an unknown extension', async () => { + const { code, signal, stdout, stderr } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + fixtures.path('es-modules/package-type-module/imports-unknownext.mjs'), + ]); + + match(stderr, /ERR_UNKNOWN_FILE_EXTENSION/); + strictEqual(stdout, ''); + strictEqual(code, 1); + strictEqual(signal, null); + }); + }); + + it('should affect CJS .js files', async () => { + const result = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + fixtures.path('es-modules/package-type-commonjs/echo-require-cache.js'), + ]); + + deepStrictEqual(result, { + code: 0, + stderr: '', + stdout: 'undefined\n', + signal: null, + }); + }); + + it('should affect .cjs files that are imported', async () => { + const result = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + '-e', + `import ${JSON.stringify(fixtures.fileURL('es-module-require-cache/echo.cjs'))}`, + ]); + + deepStrictEqual(result, { + code: 0, + stderr: '', + stdout: 'undefined\n', + signal: null, + }); + }); + + it('should not affect entry point .cjs files (with no hooks)', async () => { + const { stderr, stdout, code } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + fixtures.path('es-module-require-cache/echo.cjs'), + ]); + + strictEqual(stderr, ''); + match(stdout, /^\[Object: null prototype\] \{(\n .+)+\n\}\n$/); + strictEqual(code, 0); + }); + + it('should affect entry point .cjs files (when any hooks is registered)', async () => { + const result = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + '--import', + 'data:text/javascript,import{register}from"node:module";register("data:text/javascript,");', + fixtures.path('es-module-require-cache/echo.cjs'), + ]); + + deepStrictEqual(result, { + code: 0, + stderr: '', + stdout: 'undefined\n', + signal: null, + }); + }); + + it('should not affect CJS from input-type', async () => { + const { stderr, stdout, code } = await spawnPromisified(process.execPath, [ + '--experimental-default-type=module', + '--input-type=commonjs', + '-p', + 'require.cache', + ]); + + strictEqual(stderr, ''); + match(stdout, /^\[Object: null prototype\] \{\}\n$/); + strictEqual(code, 0); + }); +}); diff --git a/test/fixtures/es-module-require-cache/echo.cjs b/test/fixtures/es-module-require-cache/echo.cjs new file mode 100644 index 00000000000000..c00af019304dbe --- /dev/null +++ b/test/fixtures/es-module-require-cache/echo.cjs @@ -0,0 +1 @@ +console.log(require.cache); diff --git a/test/fixtures/es-modules/package-type-commonjs/echo-require-cache.js b/test/fixtures/es-modules/package-type-commonjs/echo-require-cache.js new file mode 100644 index 00000000000000..c00af019304dbe --- /dev/null +++ b/test/fixtures/es-modules/package-type-commonjs/echo-require-cache.js @@ -0,0 +1 @@ +console.log(require.cache); From bd3208aa235d20d626c21e727423e5df0edf1574 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sun, 1 Oct 2023 21:54:22 +0200 Subject: [PATCH 2/4] fixup! esm: bypass CJS loader in default load under `--default-type=module` --- doc/api/module.md | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/doc/api/module.md b/doc/api/module.md index 88a6f030a03ec2..394af95ac00b41 100644 --- a/doc/api/module.md +++ b/doc/api/module.md @@ -610,18 +610,17 @@ not possible to replace the value of a Node.js builtin (core) module. Omitting vs providing a `source` for `'commonjs'` has very different effects: -* When a `source` is provided, or when running `node` with - `--experimental-default-type=module`, all `require` calls from this module - will be processed by the ESM loader with registered `resolve` and `load` - hooks; all `require.resolve` calls from this module will be processed by the - ESM loader with registered `resolve` hooks; only a subset of the CommonJS API - will be available (e.g. no `require.extensions`, no `require.cache`, no +* When a `source` is provided, all `require` calls from this module will be + processed by the ESM loader with registered `resolve` and `load` hooks; all + `require.resolve` calls from this module will be processed by the ESM loader + with registered `resolve` hooks; only a subset of the CommonJS API will be + available (e.g. no `require.extensions`, no `require.cache`, no `require.resolve.paths`) and monkey-patching on the CommonJS module loader will not apply. -* If `source` is undefined or `null`, and `node` is run with - `--experimental-default-type=commonjs`, it will be handled by the CommonJS - module loader and `require`/`require.resolve` calls will not go through the - registered hooks. +* If `source` is undefined or `null`, it will be handled by the CommonJS module + loader and `require`/`require.resolve` calls will not go through the + registered hooks. This behavior for nullish `source` is temporary — in the + future, nullish `source` will not be supported. When `node` is run with `--experimental-default-type=commonjs`, the Node.js internal `load` implementation, which is the value of `next` for the From fadff3b300b14235ab51113a7d700f26a67f6c2d Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Sat, 21 Oct 2023 11:11:36 +0200 Subject: [PATCH 3/4] fixup! esm: bypass CJS loader in default load under `--default-type=module` --- test/es-module/test-esm-type-flag-errors.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/es-module/test-esm-type-flag-errors.mjs b/test/es-module/test-esm-type-flag-errors.mjs index 1a4d438103f7d9..9abd99258042f1 100644 --- a/test/es-module/test-esm-type-flag-errors.mjs +++ b/test/es-module/test-esm-type-flag-errors.mjs @@ -30,7 +30,7 @@ describe('--experimental-default-type=module', { concurrency: true }, () => { }); }); - it('should affect CJS .js files', async () => { + it('should affect CJS .js files (imported, required, entry points)', async () => { const result = await spawnPromisified(process.execPath, [ '--experimental-default-type=module', fixtures.path('es-modules/package-type-commonjs/echo-require-cache.js'), From 197863a27202bac91dcebeb3ace722a9737c3a03 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Mon, 23 Oct 2023 19:25:50 +0200 Subject: [PATCH 4/4] fixup! esm: bypass CJS loader in default load under `--default-type=module` --- test/es-module/test-esm-type-flag-errors.mjs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/es-module/test-esm-type-flag-errors.mjs b/test/es-module/test-esm-type-flag-errors.mjs index 9abd99258042f1..8fd06c7d562bb0 100644 --- a/test/es-module/test-esm-type-flag-errors.mjs +++ b/test/es-module/test-esm-type-flag-errors.mjs @@ -59,14 +59,14 @@ describe('--experimental-default-type=module', { concurrency: true }, () => { }); }); - it('should not affect entry point .cjs files (with no hooks)', async () => { + it('should affect entry point .cjs files (with no hooks)', async () => { const { stderr, stdout, code } = await spawnPromisified(process.execPath, [ '--experimental-default-type=module', fixtures.path('es-module-require-cache/echo.cjs'), ]); strictEqual(stderr, ''); - match(stdout, /^\[Object: null prototype\] \{(\n .+)+\n\}\n$/); + match(stdout, /^undefined\n$/); strictEqual(code, 0); });