From cb527a925d1ecaa93d85140837cb6e85b9e0f45b Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Sat, 23 Nov 2024 12:28:47 +0100 Subject: [PATCH] module: do not warn when require(esm) comes from node_modules As part of the standard experimental feature graduation policy, when we unflagged require(esm) we moved the experimental warning to be emitted when require() is actually used to load ESM, which previously was an error. However, some packages in the ecosystem have already being using try-catch to load require(esm) to e.g. resolve optional dependency, and emitting warning from there instead of throwing directly could break the CLI output. To reduce the disruption for releases, as a compromise, this patch skips the warning if require(esm) comes from node_modules, where users typically don't have much control over the code. This warning will be eventually removed when require(esm) becomes stable. This patch was originally intended for the LTS releases, though it seems there's appetite for it on v23.x as well so it's re-targeted to the main branch. PR-URL: https://github.com/nodejs/node/pull/55960 Backport-PR-URL: https://github.com/nodejs/node/pull/55217 Refs: https://github.com/nodejs/node/pull/55217 Refs: https://github.com/nodejs/node/issues/52697 Reviewed-By: Matteo Collina Reviewed-By: Antoine du Hamel Reviewed-By: Jacob Smith --- lib/internal/modules/cjs/loader.js | 59 ++++++++++++------- .../test-require-node-modules-warning.js | 59 +++++++++++++++++++ .../import-import-require-esm.mjs | 2 + .../test_node_modules/import-require-esm.mjs | 2 + .../node_modules/esm/index.js | 1 + .../node_modules/esm/package.json | 4 ++ .../node_modules/import-require-esm/index.js | 2 + .../import-require-esm/package.json | 4 ++ .../node_modules/require-esm/index.js | 2 + .../test_node_modules/require-esm.js | 2 + .../test_node_modules/require-require-esm.js | 2 + 11 files changed, 119 insertions(+), 20 deletions(-) create mode 100644 test/es-module/test-require-node-modules-warning.js create mode 100644 test/fixtures/es-modules/test_node_modules/import-import-require-esm.mjs create mode 100644 test/fixtures/es-modules/test_node_modules/import-require-esm.mjs create mode 100644 test/fixtures/es-modules/test_node_modules/node_modules/esm/index.js create mode 100644 test/fixtures/es-modules/test_node_modules/node_modules/esm/package.json create mode 100644 test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/index.js create mode 100644 test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/package.json create mode 100644 test/fixtures/es-modules/test_node_modules/node_modules/require-esm/index.js create mode 100644 test/fixtures/es-modules/test_node_modules/require-esm.js create mode 100644 test/fixtures/es-modules/test_node_modules/require-require-esm.js diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 5af29ef56acaac..d28a295c3af9ec 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -70,6 +70,7 @@ const { module_export_private_symbol, module_parent_private_symbol, }, + isInsideNodeModules, } = internalBinding('util'); const { kEvaluated, createRequiredModuleFacade } = internalBinding('module_wrap'); @@ -1342,6 +1343,7 @@ Module.prototype.require = function(id) { } }; +let emittedRequireModuleWarning = false; /** * Resolved path to `process.argv[1]` will be lazily placed here * (needed for setting breakpoint when called with `--inspect-brk`). @@ -1375,29 +1377,46 @@ function loadESMFromCJS(mod, filename) { setOwnProperty(process, 'mainModule', undefined); } else { const parent = mod[kModuleParent]; - let messagePrefix; - if (parent) { - // In the case of the module calling `require()`, it's more useful to know its absolute path. - let from = parent.filename || parent.id; - // In the case of the module being require()d, it's more useful to know the id passed into require(). - const to = mod.id || mod.filename; - if (from === 'internal/preload') { - from = '--require'; - } else if (from === '') { - from = 'The REPL'; - } else if (from === '.') { - from = 'The entry point'; - } else { - from &&= `CommonJS module ${from}`; + + if (!emittedRequireModuleWarning) { + let shouldEmitWarning = false; + // Check if the require() comes from node_modules. + if (parent) { + shouldEmitWarning = !isUnderNodeModules(parent.filename); + } else if (mod[kIsCachedByESMLoader]) { + // It comes from the require() built for `import cjs` and doesn't have a parent recorded + // in the CJS module instance. Inspect the stack trace to see if the require() + // comes from node_modules and reduce the noise. If there are more than 100 frames, + // just give up and assume it is under node_modules. + shouldEmitWarning = !isInsideNodeModules(100, true); } - if (from && to) { - messagePrefix = `${from} is loading ES Module ${to} using require().\n`; + if (shouldEmitWarning) { + let messagePrefix; + if (parent) { + // In the case of the module calling `require()`, it's more useful to know its absolute path. + let from = parent.filename || parent.id; + // In the case of the module being require()d, it's more useful to know the id passed into require(). + const to = mod.id || mod.filename; + if (from === 'internal/preload') { + from = '--require'; + } else if (from === '') { + from = 'The REPL'; + } else if (from === '.') { + from = 'The entry point'; + } else { + from &&= `CommonJS module ${from}`; + } + if (from && to) { + messagePrefix = `${from} is loading ES Module ${to} using require().\n`; + } + } + emitExperimentalWarning('Support for loading ES Module in require()', + messagePrefix, + undefined, + parent?.require); + emittedRequireModuleWarning = true; } } - emitExperimentalWarning('Support for loading ES Module in require()', - messagePrefix, - undefined, - parent?.require); const { wrap, namespace, diff --git a/test/es-module/test-require-node-modules-warning.js b/test/es-module/test-require-node-modules-warning.js new file mode 100644 index 00000000000000..837f174fd28950 --- /dev/null +++ b/test/es-module/test-require-node-modules-warning.js @@ -0,0 +1,59 @@ +'use strict'; + +// This checks the experimental warning for require(esm) is disabled when the +// require() comes from node_modules. +require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +const warningRE = /Support for loading ES Module in require\(\)/; + +// The fixtures are placed in a directory that includes "node_modules" in its name +// to check false negatives. + +// require() in non-node_modules -> esm in node_modules should warn. +spawnSyncAndAssert( + process.execPath, + [fixtures.path('es-modules', 'test_node_modules', 'require-esm.js')], + { + trim: true, + stderr: warningRE, + stdout: 'world', + } +); + +// require() in non-node_modules -> require() in node_modules -> esm in node_modules +// should not warn. +spawnSyncAndAssert( + process.execPath, + [fixtures.path('es-modules', 'test_node_modules', 'require-require-esm.js')], + { + trim: true, + stderr: '', + stdout: 'world', + } +); + +// Import in non-node_modules -> require() in node_modules -> esm in node_modules +// should not warn. +spawnSyncAndAssert( + process.execPath, + [fixtures.path('es-modules', 'test_node_modules', 'import-require-esm.mjs')], + { + trim: true, + stderr: '', + stdout: 'world', + } +); + +// Import in non-node_modules -> import in node_modules -> +// require() in node_modules -> esm in node_modules should not warn. +spawnSyncAndAssert( + process.execPath, + [fixtures.path('es-modules', 'test_node_modules', 'import-import-require-esm.mjs')], + { + trim: true, + stderr: '', + stdout: 'world', + } +); diff --git a/test/fixtures/es-modules/test_node_modules/import-import-require-esm.mjs b/test/fixtures/es-modules/test_node_modules/import-import-require-esm.mjs new file mode 100644 index 00000000000000..d470088c38caee --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/import-import-require-esm.mjs @@ -0,0 +1,2 @@ +import mod from 'import-require-esm'; +console.log(mod.hello); diff --git a/test/fixtures/es-modules/test_node_modules/import-require-esm.mjs b/test/fixtures/es-modules/test_node_modules/import-require-esm.mjs new file mode 100644 index 00000000000000..2ad346de9e841c --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/import-require-esm.mjs @@ -0,0 +1,2 @@ +import mod from 'require-esm'; +console.log(mod.hello); diff --git a/test/fixtures/es-modules/test_node_modules/node_modules/esm/index.js b/test/fixtures/es-modules/test_node_modules/node_modules/esm/index.js new file mode 100644 index 00000000000000..35f468bf4856d8 --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/node_modules/esm/index.js @@ -0,0 +1 @@ +export const hello = 'world'; diff --git a/test/fixtures/es-modules/test_node_modules/node_modules/esm/package.json b/test/fixtures/es-modules/test_node_modules/node_modules/esm/package.json new file mode 100644 index 00000000000000..07aec65d5a4f31 --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/node_modules/esm/package.json @@ -0,0 +1,4 @@ +{ + "type": "module", + "main": "index.js" +} diff --git a/test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/index.js b/test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/index.js new file mode 100644 index 00000000000000..918bb5d5597e34 --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/index.js @@ -0,0 +1,2 @@ +import mod from 'require-esm'; +export default mod; diff --git a/test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/package.json b/test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/package.json new file mode 100644 index 00000000000000..07aec65d5a4f31 --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/node_modules/import-require-esm/package.json @@ -0,0 +1,4 @@ +{ + "type": "module", + "main": "index.js" +} diff --git a/test/fixtures/es-modules/test_node_modules/node_modules/require-esm/index.js b/test/fixtures/es-modules/test_node_modules/node_modules/require-esm/index.js new file mode 100644 index 00000000000000..ca6f0c264ad1fc --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/node_modules/require-esm/index.js @@ -0,0 +1,2 @@ +module.exports = require('esm'); + diff --git a/test/fixtures/es-modules/test_node_modules/require-esm.js b/test/fixtures/es-modules/test_node_modules/require-esm.js new file mode 100644 index 00000000000000..60ad3f7fff60c6 --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/require-esm.js @@ -0,0 +1,2 @@ +const { hello } = require('esm'); +console.log(hello); diff --git a/test/fixtures/es-modules/test_node_modules/require-require-esm.js b/test/fixtures/es-modules/test_node_modules/require-require-esm.js new file mode 100644 index 00000000000000..9fe255dce258a6 --- /dev/null +++ b/test/fixtures/es-modules/test_node_modules/require-require-esm.js @@ -0,0 +1,2 @@ +const { hello } = require('require-esm'); +console.log(hello);