fix: ensure the callback 'name' arg is the module name when matching the module main file, even when 'internals: true' option is used

index.js

@@ -3,7 +3,7 @@
 // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc.
 
 const path = require('path')
-const parse = require('module-details-from-path')
+const moduleDetailsFromPath = require('module-details-from-path')
 const { fileURLToPath } = require('url')
 const { MessageChannel } = require('worker_threads')
 
@@ -128,9 +128,9 @@ function Hook (modules, options, hookFn) {
   }
 
   this._iitmHook = (name, namespace, specifier) => {
-    const filename = name
-    const isNodeUrl = name.startsWith('node:')
-    let baseDir
+    const loadUrl = name
+    const isNodeUrl = loadUrl.startsWith('node:')
+    let filePath, baseDir
 
     if (isNodeUrl) {
       // Normalize builtin module name to *not* have 'node:' prefix, unless
@@ -140,38 +140,43 @@ function Hook (modules, options, hookFn) {
       if (isBuiltin(unprefixed)) {
         name = unprefixed
       }
-    } else {
-      if (name.startsWith('file://')) {
-        const stackTraceLimit = Error.stackTraceLimit
-        Error.stackTraceLimit = 0
-        try {
-          name = fileURLToPath(name)
-        } catch (e) {}
-        Error.stackTraceLimit = stackTraceLimit
-      }
-      const details = parse(name)
-      if (details) {
-        name = details.name
-        baseDir = details.basedir
+    } else if (loadUrl.startsWith('file://')) {
+      const stackTraceLimit = Error.stackTraceLimit
+      Error.stackTraceLimit = 0
+      try {
+        filePath = fileURLToPath(name)
+        name = filePath
+      } catch (e) {}
+      Error.stackTraceLimit = stackTraceLimit
+
+      if (filePath) {
+        const details = moduleDetailsFromPath(filePath)
+        if (details) {
+          name = details.name
+          baseDir = details.basedir
+        }
       }
     }
 
     if (modules) {
       for (const matchArg of modules) {
-        if (matchArg === name) {
+        if (filePath && matchArg === filePath) {
+          // abspath match
+          callHookFn(hookFn, namespace, filePath, undefined)
+        } else if (matchArg === name) {
           if (!baseDir) {
             // built-in module (or unexpected non file:// name?)
             callHookFn(hookFn, namespace, name, baseDir)
-          } else if (internals) {
-            const internalPath = name + path.sep + path.relative(baseDir, fileURLToPath(filename))
-            callHookFn(hookFn, namespace, internalPath, baseDir)
-          } else if (baseDir.endsWith(specifiers.get(filename))) {
+          } else if (baseDir.endsWith(specifiers.get(loadUrl))) {
             // An import of the top-level module (e.g. `import 'ioredis'`).
             // Note: Slight behaviour difference from RITM. RITM uses
-            // `require.resolve(name)` to see if `filename` is the module
+            // `require.resolve(name)` to see if filename is the module
             // main file, which will catch `require('ioredis/built/index.js')`.
             // The check here will not catch `import 'ioredis/built/index.js'`.
             callHookFn(hookFn, namespace, name, baseDir)
+          } else if (internals) {
+            const internalPath = name + path.sep + path.relative(baseDir, filePath)
+            callHookFn(hookFn, namespace, internalPath, baseDir)
           }
         } else if (matchArg === specifier) {
           callHookFn(hookFn, namespace, specifier, baseDir)

test/hook/absolute-paths.mjs

@@ -0,0 +1,39 @@
+import path from 'path'
+import { fileURLToPath } from 'url'
+import { deepStrictEqual } from 'assert'
+import Hook from '../../index.js'
+
+const hooked = []
+const TOP = path.resolve(fileURLToPath(import.meta.url), '../../../')
+Hook([
+  `${TOP}/test/fixtures/index.js`,
+  `${TOP}/test/fixtures/something.js`,
+  `${TOP}/test/fixtures/node_modules/some-external-module/index.mjs`,
+  `${TOP}/test/fixtures/node_modules/some-external-module/sub.mjs`
+], (_, name, baseDir) => {
+  hooked.push([name, baseDir])
+})
+
+;(async () => {
+  // Import an absolute path.
+  await import(`${TOP}/test/fixtures/index.js`)
+
+  // Import a relative path.
+  await import('../fixtures/something.js')
+
+  // Absolute path, note the file happens to be in a `node_modules` dir.
+  await import(`${TOP}/test/fixtures/node_modules/some-external-module/index.mjs`)
+
+  // Relative path, note the file happens to be in a `node_modules` dir.
+  await import('../fixtures/node_modules/some-external-module/sub.mjs')
+
+  deepStrictEqual(
+    hooked,
+    [
+      [`${TOP}/test/fixtures/index.js`, undefined],
+      [`${TOP}/test/fixtures/something.js`, undefined],
+      [`${TOP}/test/fixtures/node_modules/some-external-module/index.mjs`, undefined],
+      [`${TOP}/test/fixtures/node_modules/some-external-module/sub.mjs`, undefined]
+    ]
+  )
+})()

test/hook/v22-duplicate-entries.mjs

@@ -12,6 +12,6 @@ await import('../fixtures/load-external-modules.mjs')
 deepStrictEqual(hits, [
   'some-external-module/sub.mjs',
   'some-external-module/sub.mjs',
-  'some-external-module/index.mjs',
-  'some-external-module/index.mjs'
+  'some-external-module',
+  'some-external-module'
 ])

test/hook/v22-hook-module-and-specifier.mjs

@@ -11,5 +11,5 @@ await import('../fixtures/load-external-modules.mjs')
 deepStrictEqual(hits, [
   'some-external-module/sub.mjs', // module name match, internals:true
   'some-external-module/sub', // specifier match
-  'some-external-module/index.mjs' // module name match, internals:true
+  'some-external-module' // module name match, internals:true
 ])

test/low-level/v22-esm-internal-spec-match.mjs

@@ -25,22 +25,22 @@ const hookModuleURL = String(pathToFileURL(
 register(hookModuleURL, import.meta.url, registerOptions)
 
 Hook(['some-external-module'], { internals: true }, (exported, name) => {
-  equal(name, 'some-external-module/index.mjs')
+  equal(name, 'some-external-module')
   exported.foo += '-mutated'
 })
 
 Hook(['some-external-cjs-module'], { internals: true }, (exported, name) => {
-  equal(name, 'some-external-cjs-module/index.js')
+  equal(name, 'some-external-cjs-module')
   exported.foo += '-mutated (cjs)'
 })
 
 Hook(['@scope/some-scoped-module'], { internals: true }, (exported, name) => {
-  equal(name, '@scope/some-scoped-module/index.mjs')
+  equal(name, '@scope/some-scoped-module')
   exported.foo += '-mutated'
 })
 
 Hook(['@scope/some-scoped-cjs-module'], { internals: true }, (exported, name) => {
-  equal(name, '@scope/some-scoped-cjs-module/index.js')
+  equal(name, '@scope/some-scoped-cjs-module')
   exported.foo += '-mutated (cjs)'
 })