elastic/require-in-the-middle

hooking 'sub-module/foo' does not work if 'sub-module' is also in the set of modules to hook

Closed this issue · 3 comments

trentm commented

If you add this diff to "test/sub-module.js":

diff --git a/test/sub-module.js b/test/sub-module.js
index e6f104f..bafc6fb 100644
--- a/test/sub-module.js
+++ b/test/sub-module.js
@@ -7,7 +7,7 @@ const { Hook } = require('../')
 test('require(\'sub-module/foo\') => sub-module/foo.js', function (t) {
   t.plan(3)

-  const hook = new Hook(['sub-module/foo'], function (exports, name, basedir) {
+  const hook = new Hook(['sub-module/foo', 'sub-module'], function (exports, name, basedir) {
     t.equal(name, 'sub-module/foo')
     return exports
   })

then the test fails:

% node test/sub-module.js
TAP version 13
# require('sub-module/foo') => sub-module/foo.js
not ok 1 should be equal
  ---
    operator: equal
    expected: |-
      'sub-module/foo'
    actual: |-
      'sub-module'
    at: <anonymous> (/Users/trentm/el/require-in-the-middle3/test/sub-module.js:11:7)
    stack: |-
      Error: should be equal
          at Test.assert [as _assert] (/Users/trentm/el/require-in-the-middle3/node_modules/tape/lib/test.js:273:48)
          at Test.bound [as _assert] (/Users/trentm/el/require-in-the-middle3/node_modules/tape/lib/test.js:85:17)
          at Test.equal (/Users/trentm/el/require-in-the-middle3/node_modules/tape/lib/test.js:434:7)
          at Test.bound [as equal] (/Users/trentm/el/require-in-the-middle3/node_modules/tape/lib/test.js:85:17)
          at /Users/trentm/el/require-in-the-middle3/test/sub-module.js:11:7
          at Module.Hook._require.Module.require (/Users/trentm/el/require-in-the-middle3/index.js:262:28)
          at require (node:internal/modules/cjs/helpers:119:18)
          at Test.<anonymous> (/Users/trentm/el/require-in-the-middle3/test/sub-module.js:19:11)
          at Test.bound [as _cb] (/Users/trentm/el/require-in-the-middle3/node_modules/tape/lib/test.js:85:17)
          at Test.run (/Users/trentm/el/require-in-the-middle3/node_modules/tape/lib/test.js:104:7)
  ...
ok 2 should be equal
ok 3 should be equal
# require('sub-module/bar') => sub-module/bar/index.js
ok 4 should be equal
ok 5 should be equal
ok 6 should be equal
# require('sub-module/bar/../bar') => sub-module/bar/index.js
ok 7 should be equal
ok 8 should be equal
ok 9 should be equal
# require('sub-module/conflict') => sub-module/conflict.js
ok 10 should be equal
ok 11 should be equal
ok 12 should be equal
# require('sub-module/conflict/index.js') => sub-module/conflict/index.js
ok 13 should be equal
ok 14 should be equal
ok 15 should be equal

1..15
# tests 15
# pass  14
# fail  1

The reason is that fullModulePath is not considered here if if the moduleName is in the set of modules to hook:

require-in-the-middle/index.js

Lines 227 to 231 in 9ddb8c7

if (hasWhitelist === true && modules.includes(moduleName) === false) {
if (modules.includes(fullModuleName) === false) return exports // abort if module name isn't on whitelist
// if we get to this point, it means that we're requiring a whitelisted sub-module
moduleName = fullModuleName

I think it is a bug that having the top-level module in the set of modules to hook breaks getting an onRequire callback for a sub-module.

The real world case for this is that the current elastic-apm-node is using sub-modules with its RITM usage, and in elastic/apm-agent-nodejs#3657 we wanted to hook both (a) the top-level "mongodb" module and (b) a sub-module path ("mongodb/lib/cmap/connection_pool"). For this and other reasons, elastic-apm-node is changing its usage to not use sub-modules for its built-in instrumentations -- instead we are moving to using the {internals: true} RITM option and hooking module-relative pathes, e.g. "mongodb/lib/cmap/connection_pool.js".

This changes means that this bug isn't a high priority issue for me. Please vote/comment/propose a PR if this is an important issue for you.

pgayvallet commented

@trentm FWIW, this is impacting Kibana in our attempt to work around #83

trentm commented

@pgayvallet There is a require-in-the-middle@7.2.1 release out with this now.