-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes allow listing mapped modules that resolve with an extension #88
Conversation
PR elastic#82 originally included code to ignore the resolved file extension. This PR restores that functionality.
f2e51bd
to
9847cf8
Compare
@trentm do you have time to take a look at this one? |
@jsumners-nr Again sorry for the delay in looking at this. I've gone back and forth on this a couple of times, and now I think the right answer here is to (a) NOT take this patch and (b) you should use this in your code (i.e. specify the const hook = new Hook(['@langchain/core/dist/callbacks/manager.cjs'], function (exports, name) {
// ...
}); Let me explain. In an earlier look at this, I thought that this should work just like the existing "sub-module/foo" test: require-in-the-middle/test/sub-module.js Lines 7 to 21 in 826dbde
This is what your patch would accomplish. It could also be accomplished with this cleaner patch to the handling that is already normalizing a diff --git a/index.js b/index.js
index 966b9c9..1b0cf68 100644
--- a/index.js
+++ b/index.js
@@ -47,7 +47,7 @@ if (Module.isBuiltin) { // Added in node v18.6.0, v16.17.0
}
// 'foo/bar.js' or 'foo/bar/index.js' => 'foo/bar'
-const normalize = /([/\\]index)?(\.js)?$/
+const normalize = /([/\\]index)?(\.(cjs|js))?$/
// Cache `onrequire`-patched exports for modules.
// This normalization is effectively saying that a resolved load of ".../some-package/some-sub-path/foo.js" should be hookable via a The only difference with your case (with However Node's
I think RITM should follow the same pattern and NOT normalize-away tl;drI think the correct RITM usage in this case is to specify the const hook = new Hook(['@langchain/core/dist/callbacks/manager.cjs'], function (exports, name) {
// ...
}); Is there a reason you aren't able to do this in your code? This should be tested and documented. I'll try to get a PR to test and doc this soon. |
@jsumners-nr #89 is my proposed test and doc update to clarify how to hook package-internal .cjs files. |
I don't think that is going to work for us. We don't have separate instrumentations for modules loaded via |
@trentm have you had a chance to think about this? |
@jsumners-nr Can we get more concrete with the hooking code that you are trying to get to work? I think I'm missing something. The IITM Hook There is support for an Do you have some other in-progress PR to IITM to support something like that? Also, I wonder if one potential point of confusion between us here is referring to an entry-point to the
That means |
I'm doing some more research based on the information you have provided. At the moment, I have only been able to refactor the test to replicate the issue I am actually trying to solve: test('handles mapped exports: picks up allow listed resolved module', { skip: nodeSupportsExports }, function (t) {
t.plan(3)
let hookHit = false
const hook = new Hook(['@langchain/core/callbacks/manager'], function (exports) {
hookHit = true
return exports
})
const { Tool } = require('@langchain/core/tools')
const MyTool = class MyTool extends Tool {
_call () {
t.pass()
}
}
const tool = new MyTool()
tool.call('foo', [() => { t.pass() }])
t.equal(hookHit, true)
hook.unhook()
}) In short:
What we will see is that the tools script is ultimately trying to import The "fix" in this PR does not solve this. But before I go deep into solving it, what are your thoughts around this @trentm? |
why it currently doesn't work for your caseTracing the chain of loaded modules: require('@langchain/core/tools')
// resolves via package.json#exports to node_modules/@langchain/core/tools.cjs, which does:
module.exports = require('./dist/tools.cjs');
// resolves to node_modules/@langchain/core/dist/tools.cjs, which does:
const manager_js_1 = require("./callbacks/manager.cjs");
// resolves to node_modules/@langchain/core/dist/callbacks/manager.cjs
So, with the current RITM code, you need one of the following Hook args to match:
RITM's matching of an entry point is only looking at the string directly passed to the
and not with whatever internal implementation thoughtsThinking about how RITM might implement what you are looking for.
I think this could work. One subtle change in behaviour, that likely doesn't practically matter, is that this case (where the Hook arg includes both the entry-point and the package sub-module path) will match with with a different Hook(['@langchain/core/callbacks/manager', '@langchain/core/dist/callbacks/manager.cjs'], (name) => {
console.log('Hook: name=%s', name)
}) I can't imagine that'll break anyone. If we're very conservative, that would be a major version bump. |
👋 @trentm checking in to see if you've had a chance to consider my finding. |
@jsumners-nr Did you see my previous comment? |
🤦 no. I failed to refresh the page before commenting and for some reason I never got a GitHub notification of an update. Your idea sounds like a great solution. Would you like me to work on it? |
If you have the bandwidth, that would be great. It feels like I won't be able to get to this soon. |
Thanks. I'll get it into my queue. |
PR #82 originally included code to ignore the resolved file extension. This PR restores that functionality.