Revert "feat: Optionally hook internal paths like require-in-the-middle (#194)"

[trentm]

This reverts commit 976d032.

Summary

This removes the experimentalPatchInternals option that was added in #194. I believe it doesn't solve the issue.

Details

If you want to play along, commit 3ed5244 on a separate feature branch (https://github.com/trentm/import-in-the-middle/tree/trentm-clearer-match-block) includes example code demonstrating the issue with experimentalPatchInternals, that I'll walk through below.

Background

The following works. A typical case of hooking top-level import of a module.

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

register('../../hook.mjs', import.meta.url);
Hook(['c8'], (exports, name, baseDir) => {
  console.log('HIT: name=%s, baseDir=%s', name, baseDir)
})
await import('c8')

Running that:

% node play.mjs
HIT: name=c8, baseDir=/Users/trentm/tm/import-in-the-middle5/node_modules/c8

The original issue

If a user imports the module "main" file by referring to its internal implementation file, then IITM doesn't hook it. Change the above example to import('c8/index.js') and IITM doesn't hook it.

register('../../hook.mjs', import.meta.url);
Hook(['c8'], (exports, name, baseDir) => {
  console.log('HIT: name=%s, baseDir=%s', name, baseDir) // <--- This isn't hit.
})
await import('c8/index.js') // <--- This changed.

Note: This is the issue that #194 was trying to solve, which I'm inferring from the added test case.
The original motivating issue #185 was a misunderstanding on internals: true usage, which was slightly different.

The workaround: experimentalPatchInternals

register('../../hook.mjs', import.meta.url, { data: { experimentalPatchInternals: true } })
Hook(['c8'], (exports, name, baseDir) => {
  console.log('HIT: name=%s, baseDir=%s', name, baseDir) // <--- This hits now, with name==`c8`.
})
await import('c8/index.js')

This looks like it works:

HIT: name=c8, baseDir=/Users/trentm/tm/import-in-the-middle5/node_modules/c8

However, c8 is a commonjs module, so we don't see the problem.

The problem with experimentalPatchInternals

If the package being hooked: (a) is type=module, (b) has multiple files, and (c) does not use "exports" (so we can import the "main" implementation file path). Then every file loaded in the package is matched by IITM, and all of them with name === "<the module name>".

Given this package:

node_modules/an-esm-module-not-using-exports-field/
  package.json
      {
        "type": "module",
        "main": "./index.js"
      }
  index.js
      export * from './lib/foo.js';
      export * from './lib/bar.js';
  lib/foo.js:
      export const fooVar = 1;
  lib/bar.js:
      export const barVar = 2;

The run this:

register('../../hook.mjs', import.meta.url, { data: { experimentalPatchInternals: true } })
Hook(['an-esm-module-not-using-exports-field'], (exports, name, baseDir) => {
  console.log('HIT: name=%s', name, exports)
})
await import('an-esm-module-not-using-exports-field/index.js')

The result is every internal file is matched, and the returned name is useless:

HIT: name=an-esm-module-not-using-exports-field [Object: null prototype] [Module] { fooVar: 1 }
HIT: name=an-esm-module-not-using-exports-field [Object: null prototype] [Module] { barVar: 2 }
HIT: name=an-esm-module-not-using-exports-field [Object: null prototype] [Module] { fooVar: 1, barVar: 2 }

Using internals: true is better

Using internals: true

register('../../hook.mjs', import.meta.url)
Hook(['an-esm-module-not-using-exports-field'], {internals: true}, (exports, name, baseDir) => {
  console.log('HIT: name=%s', name)
})
await import('an-esm-module-not-using-exports-field/index.js')

is better:

HIT: name=an-esm-module-not-using-exports-field/lib/foo.js
HIT: name=an-esm-module-not-using-exports-field/lib/bar.js
HIT: name=an-esm-module-not-using-exports-field/index.js

But still not exactly what RITM does

The equivalent with RITM results in:

% node play-ritm.cjs
HIT: name=a-cjs-module-not-using-exports-field/lib/foo.js
HIT: name=a-cjs-module-not-using-exports-field/lib/bar.js
HIT: name=a-cjs-module-not-using-exports-field

RITM does extra work (using require.resolve(moduleName)) to realize that the loaded ".../a-cjs-module-not-using-exports-field/index.js" file is the package "main" file, and then returns the module name rather than the internal file path.

I don't know how IITM could do this.
I'll open a separate issue to discuss this behaviour difference with RITM.

[BridgeAR]

LGTM, I just wonder if we should remove it as a major version bump.

[timfish]

I just wonder if we should remove it as a major version bump.

It was an experimental option and I can't find any sign of anyone using it in public code. If we can confirm this I think it's safe to remove in v2.

[trentm]

LGTM, I just wonder if we should remove it as a major version bump.

I was leaning on it explicitly being experimental with the note in the README that it could be removed in a minor.
AFAIK the feature (in the PR or the changelog) was never directly linked back to the two motivating issues (#185 and #187). The authors of those issues never responded, and we closed those two issues as misunderstandings a few days ago.

That said, I don't have a strong preference. #230 keeps support for Node.js 18 that OTel currently requires, so all good. (There might be some small pain for my work's non-OTel-based agent which currently still has support back to Node.js 14, but that doesn't need to block anything.)