fix: CommonJs bare specifier resolution (#96)

Closes #95

I've also made the `getExports` functions return `Set<string>` instead
of `string[]` as this saves a load of unnecessary allocations.

I've also run this through the additional module tests in #93 and get
the same results.

Author: timfish

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<string>} 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) {

lib/get-exports.js

@@ -1,36 +1,60 @@
 '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])
+}
+
+// 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 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()
-  ]))
-
-  urlsBeingProcessed.delete(url)
-  return full
+  try {
+    const result = parseCjs(source)
+    const full = addDefault(result.exports)
+
+    await Promise.all(result.reexports.map(async re => {
+      if (re.startsWith('node:') || builtinModules.includes(re)) {
+        for (const each of getExportsForNodeBuiltIn(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 +69,7 @@ async function getFullCjsExports (url, context, parentLoad, source) {
  * @param {Function} parentLoad Next hook function in the loaders API
  * hook chain.
  *
- * @returns {Promise<string[]>} An array of identifiers exported by the module.
+ * @returns {Promise<Set<string>>} 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 +81,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 getExportsForNodeBuiltIn(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 +108,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)
   }
 }
 

test/fixtures/node_modules/some-external-cjs-module/index.js

@@ -0,0 +1 @@
+module.exports = require('util').deprecate

test/fixtures/node_modules/some-external-cjs-module/package.json

@@ -0,0 +1 @@
+module.exports = require('some-external-cjs-module').foo

test/fixtures/re-export-cjs-built-in.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}`)
 })

test/fixtures/re-export-cjs.js

@@ -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')