Skip to content

Conversation

@trentm
Copy link
Contributor

@trentm trentm commented Jan 23, 2026

This clarifies the hook matching section (as discussed a bit at #187 (comment)) and fixes a couple issues when the Hook arg has duplicate entries or matches a module and a sub-module (and internals: true is being used).

Prerequisites

Details

1. Change specifier (sub-module) match to return name==specifier

Given this hook that matches sub-module imports / entry points:

Hook(['an-esm-module-not-using-exports-field/lib/foo.js', '@langchain/core/vectorstores'], (exports, name, baseDir) => {
  console.log('HIT: name=%s', name)
})
await import('an-esm-module-not-using-exports-field/lib/foo.js')
await import('@langchain/core/vectorstores')

Currently the on-import callback gets these name values:

HIT: name=an-esm-module-not-using-exports-field
HIT: name=@langchain/core

This PR changes the returned name to be the specifier:

HIT: name=an-esm-module-not-using-exports-field/lib/foo.js
HIT: name=@langchain/core/vectorstores

Which strikes me as more useful when using specifier matching. Otherwise a single hook that matches multiple entry-points in the same package couldn't differentiate with the name which was hooked.

2. Fix behaviour of internals: true when there are duplicate module entries

Given:

Hook(['@langchain/core', '@langchain/core'], (exports, name, baseDir) => {
  console.log('HIT: name=%s', name)
})
await import('@langchain/core')

Currently the Hook returns matches for each entry:

HIT: name=@langchain/core
HIT: name=@langchain/core

However, with internals:true:

Hook([
  '@langchain/core',
  '@langchain/core'
], { internals: true }, (exports, name, baseDir) => {
  console.log('HIT: name=%s', name)
})
await import('@langchain/core')

This get messed up (because name var handling in the match loop is keeping state,
assuming there will only ever be one match).

HIT: name=@langchain/core/dist/index.js
HIT: name=@langchain/core/dist/index.js/dist/index.js  <-- This is wrong.

The same issue also leads to bogus name values when a module and an entry-point in that module are being matched:

Hook([
  '@langchain/core',
  '@langchain/core/vectorstores'
], { internals: true }, (exports, name, baseDir) => {
  console.log('HIT: name=%s', name)
})
await import('@langchain/core/vectorstores')

Results in:

...
HIT: name=@langchain/core/dist/utils/zod-to-json-schema/index.js
HIT: name=@langchain/core/dist/utils/json_schema.js
HIT: name=@langchain/core/dist/runnables/graph.js
HIT: name=@langchain/core/dist/runnables/wrappers.js
HIT: name=@langchain/core/dist/runnables/iter.js
HIT: name=@langchain/core/dist/runnables/base.js
HIT: name=@langchain/core/dist/retrievers/index.js
HIT: name=@langchain/core/dist/vectorstores.js
HIT: name=@langchain/core/dist/vectorstores.js/dist/vectorstores.js   <--- This one is wrong

This PR fixes both those cases (the part using internalPath instead of mutating name).

@trentm trentm self-assigned this Jan 23, 2026
@trentm trentm changed the title fix: clarify the matching and fix a couple issues with duplicate entries and specifier (submodule) matching fix: fix a couple issues with duplicate entries and specifier (submodule) matching Jan 24, 2026
timfish
timfish previously approved these changes Jan 24, 2026
Comment on lines +157 to +163
} else if (baseDir.endsWith(specifiers.get(filename))) {
// 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
// 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think matching this with RITM would be good. I would be fine to do that as a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BridgeAR Please see #239 which discusses this. I don't know how to fix this difference very easily.

@timfish timfish merged commit fdc0b3d into nodejs:main Jan 27, 2026
51 checks passed
@trentm trentm deleted the trentm-clearer-match-block branch January 27, 2026 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants