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

[trentm]

This implements "option 1" of #238

Using the got module as an example, the issue was that the following
two hooks would have a different name value in the callback when the
got main file (".../node_modules/got/dist/source/index.js") was
loaded:

Hook(['got'], (exports, name) => { ... })
Hook(['got'], {internals: true}, (exports, name) => { ... })

In the first case name === 'got',
in the second case name === 'got/dist/source/index.js.
This differed from RITM behaviour where name === 'got' in both cases.

This also fixes an issue with IITM not properly hooking an absolute
path: when the absolute path being imported happened to be in a
"node_modules" subdir.

Closes: #238

Checklist

• This builds on fix: fix a couple issues with duplicate entries and specifier (submodule) matching #237 and needs that PR to merge first.

Details on the absolute-path import fixes

Consider this can-iitm-match-abs-paths.mjs which attempts to hook four absolute paths, with slightly different cases:

import { register } from 'node:module'
import { Hook } from './index.js'

register('./hook.mjs', import.meta.url)

const CWD = process.cwd()
Hook([
  `${CWD}/test/fixtures/index.js`,
  `${CWD}/test/fixtures/something.js`,
  `${CWD}/test/fixtures/node_modules/some-external-module/index.mjs`,
  `${CWD}/test/fixtures/node_modules/some-external-module/sub.mjs`
], (mod, name, baseDir) => {
  console.log(`IITM Hook: name=${name}, baseDir=${baseDir}`)
})

// 1. Import an absolute path.
await import(`${CWD}/test/fixtures/index.js`)

// 2. Import a relative path.
await import('./test/fixtures/something.js')

// 3. Absolute path, note the file happens to be in a `node_modules` dir.
await import(`${CWD}/test/fixtures/node_modules/some-external-module/index.mjs`)

// 4. Relative path, note the file happens to be in a `node_modules` dir.
await import('./test/fixtures/node_modules/some-external-module/sub.mjs')

With current IITM:

• Case 3 matches with a misleading (and inconsistent with other abspath matches) baseDir in the callback.
• Case 4 doesn't match at all.

% node can-iitm-match-abs-paths.mjs
IITM Hook: name=/Users/trentm/tm/import-in-the-middle6/test/fixtures/index.js, baseDir=undefined
IITM Hook: name=/Users/trentm/tm/import-in-the-middle6/test/fixtures/something.js, baseDir=undefined
IITM Hook: name=/Users/trentm/tm/import-in-the-middle6/test/fixtures/node_modules/some-external-module/index.mjs, baseDir=/Users/trentm/tm/import-in-the-middle6/test/fixtures/node_modules/some-external-module

The reason for both issues is that the absolute files happen to be under a .../node_modules/... tree. Case 3 accidentally matches using the matchArg === specifier branch, rather than the intended match branch for absolute paths.

With the changes in this PR:

% node can-iitm-match-abs-paths.mjs
IITM Hook: name=/Users/trentm/tm/import-in-the-middle6/test/fixtures/index.js, baseDir=undefined
IITM Hook: name=/Users/trentm/tm/import-in-the-middle6/test/fixtures/something.js, baseDir=undefined
IITM Hook: name=/Users/trentm/tm/import-in-the-middle6/test/fixtures/node_modules/some-external-module/index.mjs, baseDir=undefined
IITM Hook: name=/Users/trentm/tm/import-in-the-middle6/test/fixtures/node_modules/some-external-module/sub.mjs, baseDir=undefined

Comparing to RITM

Using the equivalent test script for RITM:

can-ritm-match-abs-paths.cjs

const { Hook } = require('require-in-the-middle')

const CWD = process.cwd()
Hook([
  `${CWD}/test/fixtures/index.js`,
  `${CWD}/test/fixtures/something.js`,
  `${CWD}/test/fixtures/node_modules/some-external-cjs-module/index.js`,
  `${CWD}/test/fixtures/node_modules/some-external-cjs-module/sub.js`
], (exports, name, baseDir) => {
  console.log(`IITM Hook: name=${name}, baseDir=${baseDir}`)
  return exports
})

// Import an absolute path.
require(`${CWD}/test/fixtures/index.js`)

// Import a relative path.
require('./test/fixtures/something.js')

// Absolute path, note the file happens to be in a `node_modules` dir.
require(`${CWD}/test/fixtures/node_modules/some-external-cjs-module/index.js`)

// Relative path, note the file happens to be in a `node_modules` dir.
require('./test/fixtures/node_modules/some-external-cjs-module/sub.js')

the result is:

% node can-ritm-match-abs-paths.cjs
IITM Hook: name=index, baseDir=/Users/trentm/tm/import-in-the-middle6/test/fixtures
IITM Hook: name=something, baseDir=/Users/trentm/tm/import-in-the-middle6/test/fixtures
IITM Hook: name=index, baseDir=/Users/trentm/tm/import-in-the-middle6/test/fixtures/node_modules/some-external-cjs-module
IITM Hook: name=sub, baseDir=/Users/trentm/tm/import-in-the-middle6/test/fixtures/node_modules/some-external-cjs-module

All four match.
(Aside: the name and baseDir values are different between IITM and RITM, but that is a topic for a separate PR.)

[trentm]

Add some notes for reviewers.

[trentm]

Reviewer note: The name change to isNodeUrl is to avoid the name isBuiltin for the actual isBuiltin function from the node:module module in a separate PR (https://github.com/nodejs/import-in-the-middle/pull/240/files).

[trentm]

Reviewer note: This section changes to only do the moduleDetailsFromPath() parsing if we have a file:// URL that was successfully converted to a path.

[trentm]

Reviewer note: This part of the change (that moves the } else if (internals) {-block to be after the else if (baseDir.endsWith(specifier..-block) is for the first/primary fix in this PR: the fix the name in the callback to match what RITM does.

The rest of the changes in "index.js" are for the absolute-paths fix.

[jsumners-nr]

👍

[trentm]

PTAL, merging from main with a merge conflict dismissed reviews. Thanks.

[BridgeAR]

I did not get the chance to fully review this before it landed and I believe this change is actually not what we want, no matter that it deviates from ritm.

The reason is: if we declare internals: true, I really want to be on the safe side to always match the internal loaded file, no matter that the user loads it with the module name. That way it is safe that the correct code in fact always wrapped as expected. Otherwise, we would could up ending with a hook for the module name but not in case it is loaded directly by the user as internal file and I would have to declare a second hook that may only run in case the other one did not run and the other way around, if I am not mistaken (I did not yet verify this with a test case).

@trentm instead of aligning with ritm, I think we should consider changing ritm to also load the internal file when being loaded with the module name.

[trentm]

@BridgeAR I moved this to a new issue for discussion. #244