From 26e62274df05ac92e0dc80e2b54a584d036c29e2 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 4 Jun 2024 18:54:45 +0200 Subject: [PATCH 1/3] fix: CommonJs bare specifier resolution --- lib/get-esm-exports.js | 4 +- lib/get-exports.js | 68 +++++++++++-------- .../some-external-cjs-module/index.js | 2 + .../some-external-cjs-module/package.json | 4 ++ test/fixtures/re-export-cjs-built-in.js | 1 + test/fixtures/re-export-cjs.js | 1 + test/get-esm-exports/v20-get-esm-exports.js | 2 +- test/hook/re-export-cjs.mjs | 19 ++++++ 8 files changed, 69 insertions(+), 32 deletions(-) create mode 100644 test/fixtures/node_modules/some-external-cjs-module/index.js create mode 100644 test/fixtures/node_modules/some-external-cjs-module/package.json create mode 100644 test/fixtures/re-export-cjs-built-in.js create mode 100644 test/fixtures/re-export-cjs.js create mode 100644 test/hook/re-export-cjs.mjs diff --git a/lib/get-esm-exports.js b/lib/get-esm-exports.js index 9c88ac8..32b49db 100644 --- a/lib/get-esm-exports.js +++ b/lib/get-esm-exports.js @@ -29,7 +29,7 @@ function warn (txt) { * @param {string} params.moduleSource The source code of the module to parse * and interpret. * - * @returns {string[]} The identifiers exported by the module along with any + * @returns {Set} The identifiers exported by the module along with any * custom directives. */ function getEsmExports (moduleSource) { @@ -62,7 +62,7 @@ function getEsmExports (moduleSource) { warn('unrecognized export type: ' + node.type) } } - return Array.from(exportedNames) + return exportedNames } function parseDeclaration (node, exportedNames) { diff --git a/lib/get-exports.js b/lib/get-exports.js index 7616e6c..1a42a02 100644 --- a/lib/get-exports.js +++ b/lib/get-exports.js @@ -1,36 +1,46 @@ 'use strict' const getEsmExports = require('./get-esm-exports.js') -const { parse: getCjsExports } = require('cjs-module-lexer') -const fs = require('fs') +const { parse: parseCjs } = require('cjs-module-lexer') +const { readFileSync } = require('fs') +const { builtinModules } = require('module') const { fileURLToPath, pathToFileURL } = require('url') +const { dirname } = require('path') function addDefault (arr) { - return Array.from(new Set(['default', ...arr])) + return new Set(['default', ...arr]) } const urlsBeingProcessed = new Set() // Guard against circular imports. -async function getFullCjsExports (url, context, parentLoad, source) { +async function getCjsExports (url, context, parentLoad, source) { if (urlsBeingProcessed.has(url)) { return [] } urlsBeingProcessed.add(url) - const ex = getCjsExports(source) - const full = Array.from(new Set([ - ...addDefault(ex.exports), - ...(await Promise.all(ex.reexports.map(re => getExports( - (/^(..?($|\/|\\))/).test(re) - ? pathToFileURL(require.resolve(fileURLToPath(new URL(re, url)))).toString() - : pathToFileURL(require.resolve(re)).toString(), - context, - parentLoad - )))).flat() - ])) + try { + const result = parseCjs(source) + const full = addDefault(result.exports) - urlsBeingProcessed.delete(url) - return full + for (const re of result.reexports) { + if (re.startsWith('node:') || builtinModules.includes(re)) { + for (const each of Object.keys(require(re))) { + full.add(each) + } + } else { + // Resolve the re-exported module relative to the current module. + const newUrl = pathToFileURL(require.resolve(re, { paths: [dirname(fileURLToPath(url))] })).href + for (const each of await getExports(newUrl, context, parentLoad)) { + full.add(each) + } + } + } + + return full + } finally { + urlsBeingProcessed.delete(url) + } } /** @@ -45,7 +55,7 @@ async function getFullCjsExports (url, context, parentLoad, source) { * @param {Function} parentLoad Next hook function in the loaders API * hook chain. * - * @returns {Promise} An array of identifiers exported by the module. + * @returns {Promise>} An array of identifiers exported by the module. * Please see {@link getEsmExports} for caveats on special identifiers that may * be included in the result set. */ @@ -57,23 +67,23 @@ async function getExports (url, context, parentLoad) { let source = parentCtx.source const format = parentCtx.format - // TODO support non-node/file urls somehow? - if (format === 'builtin') { - // Builtins don't give us the source property, so we're stuck - // just requiring it to get the exports. - return addDefault(Object.keys(require(url))) - } - if (!source) { - // Sometimes source is retrieved by parentLoad, sometimes it isn't. - source = fs.readFileSync(fileURLToPath(url), 'utf8') + if (format === 'builtin') { + // Builtins don't give us the source property, so we're stuck + // just requiring it to get the exports. + return addDefault(Object.keys(require(url))) + } + + // Sometimes source is retrieved by parentLoad, CommonJs isn't. + source = readFileSync(fileURLToPath(url), 'utf8') } if (format === 'module') { return getEsmExports(source) } + if (format === 'commonjs') { - return getFullCjsExports(url, context, parentLoad, source) + return getCjsExports(url, context, parentLoad, source) } // At this point our `format` is either undefined or not known by us. Fall @@ -84,7 +94,7 @@ async function getExports (url, context, parentLoad) { // isn't set at first and yet we have an ESM module with no exports. // I couldn't construct an example that would do this, so maybe it's // impossible? - return getFullCjsExports(url, context, parentLoad, source) + return getCjsExports(url, context, parentLoad, source) } } diff --git a/test/fixtures/node_modules/some-external-cjs-module/index.js b/test/fixtures/node_modules/some-external-cjs-module/index.js new file mode 100644 index 0000000..1e44d6a --- /dev/null +++ b/test/fixtures/node_modules/some-external-cjs-module/index.js @@ -0,0 +1,2 @@ +const foo = 'bar' +module.exports = { foo } diff --git a/test/fixtures/node_modules/some-external-cjs-module/package.json b/test/fixtures/node_modules/some-external-cjs-module/package.json new file mode 100644 index 0000000..b0b9b8a --- /dev/null +++ b/test/fixtures/node_modules/some-external-cjs-module/package.json @@ -0,0 +1,4 @@ +{ + "name": "some-external-cjs-module", + "main": "./index.js" +} diff --git a/test/fixtures/re-export-cjs-built-in.js b/test/fixtures/re-export-cjs-built-in.js new file mode 100644 index 0000000..cbf28ad --- /dev/null +++ b/test/fixtures/re-export-cjs-built-in.js @@ -0,0 +1 @@ +module.exports = require('util').deprecate diff --git a/test/fixtures/re-export-cjs.js b/test/fixtures/re-export-cjs.js new file mode 100644 index 0000000..e3815c5 --- /dev/null +++ b/test/fixtures/re-export-cjs.js @@ -0,0 +1 @@ +module.exports = require('some-external-cjs-module').foo diff --git a/test/get-esm-exports/v20-get-esm-exports.js b/test/get-esm-exports/v20-get-esm-exports.js index 0d03214..00f45a4 100644 --- a/test/get-esm-exports/v20-get-esm-exports.js +++ b/test/get-esm-exports/v20-get-esm-exports.js @@ -15,7 +15,7 @@ fixture.split('\n').forEach(line => { if (expectedNames[0] === '') { expectedNames.length = 0 } - const names = getEsmExports(mod) + const names = Array.from(getEsmExports(mod)) assert.deepEqual(expectedNames, names) console.log(`${mod}\n ✅ contains exports: ${testStr}`) }) diff --git a/test/hook/re-export-cjs.mjs b/test/hook/re-export-cjs.mjs new file mode 100644 index 0000000..8419ca9 --- /dev/null +++ b/test/hook/re-export-cjs.mjs @@ -0,0 +1,19 @@ +import Hook from '../../index.js' +import foo from '../fixtures/re-export-cjs-built-in.js' +import foo2 from '../fixtures/re-export-cjs.js' +import { strictEqual } from 'assert' + +Hook((exports, name) => { + if (name.endsWith('fixtures/re-export-cjs-built-in.js')) { + strictEqual(typeof exports.default, 'function') + exports.default = '1' + } + + if (name.endsWith('fixtures/re-export-cjs.js')) { + strictEqual(exports.default, 'bar') + exports.default = '2' + } +}) + +strictEqual(foo, '1') +strictEqual(foo2, '2') From 8819f3aa6eec5be2d9b8a364c873ec6ee702e5ad Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 4 Jun 2024 21:46:38 +0200 Subject: [PATCH 2/3] Use `Promise.all` --- lib/get-exports.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/get-exports.js b/lib/get-exports.js index 1a42a02..1df9159 100644 --- a/lib/get-exports.js +++ b/lib/get-exports.js @@ -23,7 +23,7 @@ async function getCjsExports (url, context, parentLoad, source) { const result = parseCjs(source) const full = addDefault(result.exports) - for (const re of result.reexports) { + await Promise.all(result.reexports.map(async re => { if (re.startsWith('node:') || builtinModules.includes(re)) { for (const each of Object.keys(require(re))) { full.add(each) @@ -35,7 +35,7 @@ async function getCjsExports (url, context, parentLoad, source) { full.add(each) } } - } + })) return full } finally { From 1f01cee4c478b72688477d251e0f69a027ae5383 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Tue, 4 Jun 2024 22:16:27 +0200 Subject: [PATCH 3/3] Cache built in exports --- lib/get-exports.js | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/lib/get-exports.js b/lib/get-exports.js index 1df9159..9a78e8c 100644 --- a/lib/get-exports.js +++ b/lib/get-exports.js @@ -11,6 +11,20 @@ function addDefault (arr) { return new Set(['default', ...arr]) } +// Cached exports for Node built-in modules +const BUILT_INS = new Map() + +function getExportsForNodeBuiltIn (name) { + let exports = BUILT_INS.get() + + if (!exports) { + exports = new Set(addDefault(Object.keys(require(name)))) + BUILT_INS.set(name, exports) + } + + return exports +} + const urlsBeingProcessed = new Set() // Guard against circular imports. async function getCjsExports (url, context, parentLoad, source) { @@ -25,7 +39,7 @@ async function getCjsExports (url, context, parentLoad, source) { await Promise.all(result.reexports.map(async re => { if (re.startsWith('node:') || builtinModules.includes(re)) { - for (const each of Object.keys(require(re))) { + for (const each of getExportsForNodeBuiltIn(re)) { full.add(each) } } else { @@ -71,7 +85,7 @@ async function getExports (url, context, parentLoad) { if (format === 'builtin') { // Builtins don't give us the source property, so we're stuck // just requiring it to get the exports. - return addDefault(Object.keys(require(url))) + return getExportsForNodeBuiltIn(url) } // Sometimes source is retrieved by parentLoad, CommonJs isn't.