Fix duplicate default exports (#53)

Ref: newrelic/node-newrelic#1920 <strike>👋 my last "fix" (#43) broke in a new way thanks to the helpful ESM allowance of exporting `default` multiple times. This PR resolves the issue. I suspect there could be further problems with the exports since I have no idea how exporting `default` multiple times is meant to be interpreted, but I suspect any such issues will be an extreme edge case. The only other thing I can think of to do here is to somehow track that we already have a `default` export and munge the names of any subsequent ones.</strike> ----- I have since learned that ESM doesn't actually allow exporting `default` multiple times. Transitive `default` exports get mapped to some other name for the module that has imported them, and only the directly imported `default` gets exported. Still, we have to handle figuring out which `default` is the right one and construct our shim namespace accordingly. ----- 2023-01-03: this can only be supported on Node versions where we parse the modules with an AST parser and interpret the code manually. So I have updated the test suite for this feature to only run on such versions. --------- Co-authored-by: James Sumners <[email protected]>
nodejs · Jan 8, 2024 · 3a3cd49 · 3a3cd49
1 parent 8066c39
commit 3a3cd49
Show file tree

Hide file tree

Showing 12 changed files with 279 additions and 37 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -38,15 +38,22 @@ jobs:
           node-version: ${{ matrix.node-version }}
       - run: npm install
       - run: npm test
-      - run: npm run test:ts
-        if: (matrix.node-version != '12.x' && matrix.node-version != '14.x' && matrix.node-version != '16.10.0')
+      - name: Rename coverage file
+        run: >
+          mv coverage/lcov.info coverage/${{ matrix.node-version }}_${{ matrix.os }}_lcov.info
       - name: Archive code coverage results
         if: success()
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: coverage_${{ matrix.os }}_${{ matrix.node-version}}
           if-no-files-found: ignore
-          path: coverage/lcov.info
+          path: coverage/${{ matrix.node-version }}_${{ matrix.os }}_lcov.info
+
+      # This will clobber any coverage generated by the previous `npm test`.
+      # We are opting to omit TS coverage and stick to pass or fail only for TS.
+      - run: npm run test:ts
+        if: (matrix.node-version != '12.x' && matrix.node-version != '14.x' && matrix.node-version != '16.10.0')
+
 
   coverage:
     runs-on: ubuntu-latest
@@ -55,7 +62,7 @@ jobs:
       # We need to check out the source in order for genhtml to work
       - uses: actions/checkout@v4
       - name: Download reports' artifacts
-        uses: actions/download-artifact@v3
+        uses: actions/download-artifact@v4
         with:
           path: downloaded_artifacts
       - name: Install lcov
@@ -65,10 +72,9 @@ jobs:
       - name: Combine all coverage data
         run: |
           find . -type f -name '*.info' -exec echo -a {} \; | xargs --verbose lcov -o all_lcov.info
-      - name: Report
+      - name: Generate Coverage Report
         run: >
           lcov --summary all_lcov.info
-          lcov --summary all_lcov.info | grep lines | cut -d' ' -f 4 | cut -d% -f 1 | xargs node -e "x=process.argv[1];console.log(x);assert(+x >= 90)"
       - name: Generate HTML report
         run: |
           mkdir html_report
@@ -79,3 +85,6 @@ jobs:
           name: 00_html_coverage_report
           if-no-files-found: ignore
           path: html_report/
+      - name: Verify Minimum Coverage Is Met
+        run: >
+          lcov --summary all_lcov.info | grep lines | cut -d' ' -f 4 | cut -d% -f 1 | xargs node -e "x=process.argv[1];console.log(x);assert(+x >= 90)"
diff --git a/hook.js b/hook.js
@@ -19,7 +19,7 @@ let getExports
 if (NODE_MAJOR >= 20 || (NODE_MAJOR == 18 && NODE_MINOR >= 19)) {
   getExports = require('./lib/get-exports.js')
 } else {
-  getExports = (url) => import(url).then(Object.keys)
+  getExports = ({ url }) => import(url).then(Object.keys)
 }
 
 function hasIitm (url) {
@@ -98,8 +98,10 @@ function isStarExportLine(line) {
  * @property {string[]} namespaces A set of identifiers representing the
  * modules in `imports`, e.g. for `import * as foo from 'bar'`, "foo" will be
  * present in this array.
- * @property {string[]} setters The shimmed setters for all the exports
- * from the module and any transitive export all modules.
+ * @property {Map<string, string>} setters The shimmed setters for all the
+ * exports from the module and any transitive export all modules. The key is
+ * used to deduplicate conflicting exports, assigning a priority to `default`
+ * exports.
  */
 
 /**
@@ -111,33 +113,86 @@ function isStarExportLine(line) {
  * @param {object} params
  * @param {string} params.srcUrl The full URL to the module to process.
  * @param {object} params.context Provided by the loaders API.
- * @param {function} parentGetSource Provides the source code for the parent
- * module.
+ * @param {Function} params.parentGetSource Provides the source code for the
+ * parent module.
+ * @param {string} [params.ns='namespace'] A string identifier that will be
+ * used as the namespace for the identifiers exported by the module.
+ * @param {string} [params.defaultAs='default'] The name to give the default
+ * identifier exported by the module (if one exists). This is really only
+ * useful in a recursive situation where a transitive module's default export
+ * needs to be renamed to the name of the module.
+ *
  * @returns {Promise<ProcessedModule>}
  */
-async function processModule({ srcUrl, context, parentGetSource }) {
-  const exportNames = await getExports(srcUrl, context, parentGetSource)
-  const imports = [`import * as namespace from ${JSON.stringify(srcUrl)}`]
-  const namespaces = ['namespace']
-  const setters = []
+async function processModule({
+  srcUrl,
+  context,
+  parentGetSource,
+  ns = 'namespace',
+  defaultAs = 'default'
+}) {
+  const exportNames = await getExports({
+    url: srcUrl,
+    context,
+    parentLoad: parentGetSource,
+    defaultAs
+  })
+  const imports = [`import * as ${ns} from ${JSON.stringify(srcUrl)}`]
+  const namespaces = [ns]
+
+  // As we iterate found module exports we will add setter code blocks
+  // to this map that will eventually be inserted into the shim module's
+  // source code. We utilize a map in order to prevent duplicate exports.
+  // As a consequence of default renaming, it is possible that a file named
+  // `foo.mjs` which has `export function foo() {}` and `export default foo`
+  // exports will result in the "foo" export being defined twice in our shim.
+  // The map allows us to avoid this situation at the cost of losing the
+  // named export in favor of the default export.
+  const setters = new Map()
 
   for (const n of exportNames) {
     if (isStarExportLine(n) === true) {
       const [_, modFile] = n.split('* from ')
+      const normalizedModName = normalizeModName(modFile)
       const modUrl = new URL(modFile, srcUrl).toString()
       const modName = Buffer.from(modFile, 'hex') + Date.now() + randomBytes(4).toString('hex')
 
-      imports.push(`import * as $${modName} from ${JSON.stringify(modUrl)}`)
-      namespaces.push(`$${modName}`)
+      const data = await processModule({
+        srcUrl: modUrl,
+        context,
+        parentGetSource,
+        ns: `$${modName}`,
+        defaultAs: normalizedModName
+      })
+      Array.prototype.push.apply(imports, data.imports)
+      Array.prototype.push.apply(namespaces, data.namespaces)
+      for (const [k, v] of data.setters.entries()) {
+        setters.set(k, v)
+      }
 
-      const data = await processModule({ srcUrl: modUrl, context, parentGetSource })
-      Array.prototype.push.apply(setters, data.setters)
+      continue
+    }
 
+    const matches = /^rename (.+) as (.+)$/.exec(n)
+    if (matches !== null) {
+      // Transitive modules that export a default identifier need to have
+      // that identifier renamed to the name of module. And our shim setter
+      // needs to utilize that new name while being initialized from the
+      // corresponding origin namespace.
+      const renamedExport = matches[2]
+      setters.set(`$${renamedExport}${ns}`, `
+      let $${renamedExport} = ${ns}.default
+      export { $${renamedExport} as ${renamedExport} }
+      set.${renamedExport} = (v) => {
+        $${renamedExport} = v
+        return true
+      }
+      `)
       continue
     }
 
-    setters.push(`
-    let $${n} = _.${n}
+    setters.set(`$${n}`+ns, `
+    let $${n} = ${ns}.${n}
     export { $${n} as ${n} }
     set.${n} = (v) => {
       $${n} = v
@@ -149,6 +204,24 @@ async function processModule({ srcUrl, context, parentGetSource }) {
   return { imports, namespaces, setters }
 }
 
+/**
+ * Given a module name, e.g. 'foo-bar' or './foo-bar.js', normalize it to a
+ * string that is a valid JavaScript identifier, e.g. `fooBar`. Normalization
+ * means converting kebab-case to camelCase while removing any path tokens and
+ * file extensions.
+ *
+ * @param {string} name The module name to normalize.
+ *
+ * @returns {string} The normalized identifier.
+ */
+function normalizeModName(name) {
+  return name
+    .split('\/')
+    .pop()
+    .replace(/(.+)\.(?:js|mjs)$/, '$1')
+    .replaceAll(/(-.)/g, x => x[1].toUpperCase())
+}
+
 function addIitm (url) {
   const urlObj = new URL(url)
   urlObj.searchParams.set('iitm', 'true')
@@ -194,21 +267,60 @@ function createHook (meta) {
   async function getSource (url, context, parentGetSource) {
     if (hasIitm(url)) {
       const realUrl = deleteIitm(url)
-      const { imports, namespaces, setters } = await processModule({
+      const { imports, namespaces, setters: mapSetters } = await processModule({
           srcUrl: realUrl,
           context,
           parentGetSource
       })
-
+      const setters = Array.from(mapSetters.values())
+
+      // When we encounter modules that re-export all identifiers from other
+      // modules, it is possible that the transitive modules export a default
+      // identifier. Due to us having to merge all transitive modules into a
+      // single common namespace, we need to recognize these default exports
+      // and remap them to a name based on the module name. This prevents us
+      // from overriding the top-level module's (the one actually being imported
+      // by some source code) default export when we merge the namespaces.
+      const renamedDefaults = setters
+        .map(s => {
+          const matches = /let \$(.+) = (\$.+)\.default/.exec(s)
+          if (matches === null) return
+          return `_['${matches[1]}'] = ${matches[2]}.default`
+        })
+        .filter(s => s)
+
+      // The for loops are how we merge namespaces into a common namespace that
+      // can be proxied. We can't use a simple `Object.assign` style merging
+      // because transitive modules can export a default identifier that would
+      // override the desired default identifier. So we need to do manual
+      // merging with some logic around default identifiers.
+      //
+      // Additionally, we need to make sure any renamed default exports in
+      // transitive dependencies are added to the common namespace. This is
+      // accomplished through the `renamedDefaults` array.
       return {
         source: `
 import { register } from '${iitmURL}'
 ${imports.join('\n')}
 
-const _ = Object.assign({}, ...[${namespaces.join(', ')}])
+const namespaces = [${namespaces.join(', ')}]
+const _ = {}
 const set = {}
 
+const primary = namespaces.shift()
+for (const [k, v] of Object.entries(primary)) {
+  _[k] = v
+}
+for (const ns of namespaces) {
+  for (const [k, v] of Object.entries(ns)) {
+    if (k === 'default') continue
+    _[k] = v
+  }
+}
+
 ${setters.join('\n')}
+${renamedDefaults.join('\n')}
+
 register(${JSON.stringify(realUrl)}, _, set, ${JSON.stringify(specifiers.get(realUrl))})
 `
       }

diff --git a/lib/get-esm-exports.js b/lib/get-esm-exports.js
@@ -14,9 +14,36 @@ function warn (txt) {
   process.emitWarning(txt, 'get-esm-exports')
 }
 
-function getEsmExports (moduleStr) {
+/**
+ * Utilizes an AST parser to interpret ESM source code and build a list of
+ * exported identifiers. In the baseline case, the list of identifiers will be
+ * the simple identifier names as written in the source code of the module.
+ * However, there are some special cases:
+ *
+ * 1. When an `export * from './foo.js'` line is encountered it is rewritten
+ * as `* from ./foo.js`. This allows the interpreting code to recognize a
+ * transitive export and recursively parse the indicated module. The returned
+ * identifier list will have "* from ./foo.js" as an item.
+ *
+ * 2. When `defaultAs` has a value other than 'default', the export line will
+ * be rewritten as `rename <identifier> as <defaultAsValue>`. This rename string
+ * will be an item in the returned identifier list.
+ *
+ * @param {object} params
+ * @param {string} params.moduleSource The source code of the module to parse
+ * and interpret.
+ * @param {string} [defaultAs='default'] When anything other than 'default' any
+ * `export default` lines will be rewritten utilizing the value provided. For
+ * example, if a module 'foo-bar.js' has the line `export default foo` and the
+ * value of this parameter is 'baz', then the export will be rewritten to
+ * `rename foo as baz`.
+ *
+ * @returns {string[]} The identifiers exported by the module along with any
+ * custom directives.
+ */
+function getEsmExports ({ moduleSource, defaultAs = 'default' }) {
   const exportedNames = new Set()
-  const tree = parser.parse(moduleStr, acornOpts)
+  const tree = parser.parse(moduleSource, acornOpts)
   for (const node of tree.body) {
     if (!node.type.startsWith('Export')) continue
     switch (node.type) {
@@ -27,9 +54,24 @@ function getEsmExports (moduleStr) {
           parseSpecifiers(node, exportedNames)
         }
         break
-      case 'ExportDefaultDeclaration':
-        exportedNames.add('default')
+
+      case 'ExportDefaultDeclaration': {
+        if (defaultAs === 'default') {
+          exportedNames.add('default')
+          break
+        }
+
+        if (node.declaration.type.toLowerCase() === 'identifier') {
+          // e.g. `export default foo`
+          exportedNames.add(`rename ${node.declaration.name} as ${defaultAs}`)
+        } else {
+          // e.g. `export function foo () {}
+          exportedNames.add(`rename ${node.declaration.id.name} as ${defaultAs}`)
+        }
+
         break
+      }
+
       case 'ExportAllDeclaration':
         if (node.exported) {
           exportedNames.add(node.exported.name)

diff --git a/lib/get-exports.js b/lib/get-exports.js
@@ -9,7 +9,30 @@ function addDefault(arr) {
   return Array.from(new Set(['default', ...arr]))
 }
 
-async function getExports (url, context, parentLoad) {
+/**
+ * Inspects a module for its type (commonjs or module), attempts to get the
+ * source code for said module from the loader API, and parses the result
+ * for the entities exported from that module.
+ *
+ * @param {object} params
+ * @param {string} params.url A file URL string pointing to the module that
+ * we should get the exports of.
+ * @param {object} params.context Context object as provided by the `load`
+ * hook from the loaders API.
+ * @param {Function} params.parentLoad Next hook function in the loaders API
+ * hook chain.
+ * @param {string} [defaultAs='default'] When anything other than 'default',
+ * will trigger remapping of default exports in ESM source files to the
+ * provided name. For example, if a submodule has `export default foo` and
+ * 'myFoo' is provided for this parameter, the export line will be rewritten
+ * to `rename foo as myFoo`. This is key to being able to support
+ * `export * from 'something'` exports.
+ *
+ * @returns {Promise<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.
+ */
+async function getExports ({ url, context, parentLoad, defaultAs = 'default' }) {
   // `parentLoad` gives us the possibility of getting the source
   // from an upstream loader. This doesn't always work though,
   // so later on we fall back to reading it from disk.
@@ -30,15 +53,15 @@ async function getExports (url, context, parentLoad) {
   }
 
   if (format === 'module') {
-    return getEsmExports(source)
+    return getEsmExports({ moduleSource: source, defaultAs })
   }
   if (format === 'commonjs') {
     return addDefault(getCjsExports(source).exports)
   }
 
   // At this point our `format` is either undefined or not known by us. Fall
   // back to parsing as ESM/CJS.
-  const esmExports = getEsmExports(source)
+  const esmExports = getEsmExports({ moduleSource: source, defaultAs })
   if (!esmExports.length) {
     // TODO(bengl) it's might be possible to get here if somehow the format
     // isn't set at first and yet we have an ESM module with no exports.

diff --git a/package.json b/package.json
@@ -4,7 +4,7 @@
   "description": "Intercept imports in Node.js",
   "main": "index.js",
   "scripts": {
-    "test": "c8 --reporter lcov --check-coverage --lines 70 imhotap --runner 'node test/runtest' --files test/{hook,low-level,other,get-esm-exports}/*",
+    "test": "c8 --reporter lcov --check-coverage --lines 50 imhotap --runner 'node test/runtest' --files test/{hook,low-level,other,get-esm-exports}/*",
     "test:ts": "c8 --reporter lcov imhotap --runner 'node test/runtest' --files test/typescript/*.test.mts",
     "coverage": "c8 --reporter html imhotap --runner 'node test/runtest' --files test/{hook,low-level,other,get-esm-exports}/* && echo '\nNow open coverage/index.html\n'"
   },

diff --git a/test/fixtures/default-class.mjs b/test/fixtures/default-class.mjs
@@ -0,0 +1,3 @@
+export default class DefaultClass {
+  value = 'DefaultClass'
+}