-
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
chore: resolves locally referenced package exports #90
Conversation
@trentm how does this look to you? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. Getting into the guts of RITM dealing with manipulating file paths always takes me a long time to get my bearings, which scares me away.
I think your test is only passing by fluke.
I added this local change to your branch to (a) add some debug prints and (b) reduce test/mapped-exports.js to just your test case:
https://gist.github.com/trentm/809410f18c1ef3b0e70f8d70621d480c
Then I ran the following:
$ node test/mapped-exports.js
TAP version 13
# handles mapped exports: picks up allow listed resolved module
XXX doWork section: attempt to map `require(id="../helpers/util")` in "/Users/trentm/el/require-in-the-middle3/node_modules/zod/lib/locales/en.js" (filename="/Users/trentm/el/require-in-the-middle3/node_modules/zod/lib/helpers/util.js", moduleName="zod", fullModuleName="zod/lib/helpers/util")
XXX doWork section: attempt to map `require(id="./helpers/util")` in "/Users/trentm/el/require-in-the-middle3/node_modules/zod/lib/ZodError.js" (filename="/Users/trentm/el/require-in-the-middle3/node_modules/zod/lib/helpers/util.js", moduleName="zod", fullModuleName="zod/lib/helpers/util")
...
Most attempts to map a given relative require('./something')
don't result in a hit.
Then, getting to the require that we care about for this test:
...
XXX doWork section: attempt to map `require(id="./callbacks/manager.cjs")` in "/Users/trentm/el/require-in-the-middle3/node_modules/@langchain/core/dist/tools.cjs" (filename="/Users/trentm/el/require-in-the-middle3/node_modules/@langchain/core/dist/callbacks/manager.cjs", moduleName="@langchain/core", fullModuleName="@langchain/core/dist/callbacks/manager.cjs")
XXX matchFound: mapped filename="/Users/trentm/el/require-in-the-middle3/node_modules/@langchain/core/dist/callbacks/manager.cjs" to allowlisted moduleName="@langchain/core/callbacks/manager"
ok 1 hook name matches
XXX exports for @langchain/core/callbacks/manager: {
parseCallbackConfigArg: [Function: parseCallbackConfigArg],
BaseCallbackManager: [class BaseCallbackManager],
CallbackManagerForRetrieverRun: [class CallbackManagerForRetrieverRun extends BaseRunManager],
CallbackManagerForLLMRun: [class CallbackManagerForLLMRun extends BaseRunManager],
CallbackManagerForChainRun: [class CallbackManagerForChainRun extends BaseRunManager],
CallbackManagerForToolRun: [class CallbackManagerForToolRun extends BaseRunManager],
CallbackManager: [class CallbackManager extends BaseCallbackManager],
ensureHandler: [Function: ensureHandler],
TraceGroup: [class TraceGroup],
traceAsGroup: [AsyncFunction: traceAsGroup]
}
- This is attempting to see if the
require("./callbacks/manager.cjs")
in ".../node_modules/@langchain/core/dist/tools.cjs" maps to a package export that is listed in the Hook args. In our case the Hook args are['@langchain/core/callbacks/manager']
. - That require id maps to the file:
.../node_modules/@langchain/core/dist/callbacks/manager.cjs
. Note that this is in the "dist/" dir. - Your change suggests that this path is a match for the
@langchain/core/callbacks/manager
export.
That is not correct. The @langchain/core/callbacks/manager
export, for a "require", maps to the path ".../node_modules/@langchain/core/callbacks/manager.cjs". Note that this is the file NOT in the "dist/" dir. It is specific to @langchain/core
that ".../node_modules/@langchain/core/callbacks/manager.cjs" happens to contain:
module.exports = require('../dist/callbacks/manager.cjs');
I think this may not be possible
Earlier in #88 (comment) I gave "implementation thoughts" that suggested what you want might be possible. I think I made a mistake there (the same "dist/" dir mistake). Let me try again.
- Your goal is to be able to hook some part of the callbacks manager code in
@langchain/core
. For discussion, let's say it is theBaseCallbackManager
class. - For CommonJS code that is implemented in ".../node_modules/@langchain/core/dist/callbacks/manager.cjs".
- It should get hooked, no matter how the user indirectly causes that module to be loaded.
- The
@langchain/core/callbacks/manager
export maps to ".../node_modules/@langchain/core/callbacks/manager.cjs". - Consider this call path. This loads the
BaseCallbackManager
implementation, but at no point is the filename for the@langchain/core/callbacks/manager
export touched.require('@langchain/core/tools')
-> ".../node_modules/@langchain/core/tools.cjs"require('./dist/tools.cjs')
-> ".../node_modules/@langchain/core/dist/tools.cjs"require("./callbacks/manager.cjs")
-> ".../node_modules/@langchain/core/dist/callbacks/manager.cjs"
My conclusion is that there is no way to hook the BaseCallbackManager
being loaded by using the @langchain/core/callbacks/manager
argument to Hook()
.
test/mapped-exports.js
Outdated
@@ -42,3 +42,27 @@ test('handles mapped exports: mapped-exports/bar', { skip: nodeSupportsExports } | |||
|
|||
hook.unhook() | |||
}) | |||
|
|||
test('handles mapped exports: picks up allow listed resolved module', { skip: nodeSupportsExports }, function (t) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skip
needs to also skip out if node < 18, because that's the min node supported by @langchain/core
being used for the test. That should get the other tests to run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue addressed.
I don't know what else I can do. Yes, it is possible that
That, and the rest of the summary, is correct.
Here's what I know:
|
If I understand you correctly,
So, if one wants to hook the langchain "callbacks manager" code, regardless of what code path loads it, one needs to give RITM a hook arg that references a module that all those code paths go through. That module, in this case, is ".../node_modules/@langchain/core/dist/callbacks/manager.cjs". As I argued at #88 (comment) the RITM hook arg for this should include the .cjs extension. So I'm suggesting: Hook(['@langchain/core/dist/callbacks/manager.cjs'], ...) which works now, IIRC. On that PR you had this comment (repeated here):
I think we got partially distracted by package "exports" for langchain. I'm guessing a bit here, but perhaps you are referring to wanting to have the same Hook arg to both RITM and IITM to be able to hook the langchain "callbacks manager" code? If so, I don't think this will be possible. The In general, there is no requirement that the ESM code and CJS code for this module have a similar path. In many packages that ship both ESM and CJS code, they are in separate directory trees. So wanting a single So, I think for CJS you'll want var {Hook} = require('require-in-the-middle');
new Hook(['@langchain/core/dist/callbacks/manager.cjs'], (mod, name, baseDir) => {
console.log('Hooked name=%j:', name, mod);
return mod;
})
var tools = require('@langchain/core/tools')
console.log('Required @langchain/core/tools:', tools); and for ESM, this seems to work for me:
import * as tools from '@langchain/core/tools';
console.log('Imported @langchain/core/tools:', tools);
Using that:
possible IITM featureAFAICT, IITM doesn't support passing a module sub-path as a hook arg. That could be a feature, so that instead of requiring (a) new Hook(['@langchain/core/dist/callbacks/manager.js'], (mod, name, baseDir) => {
console.log('Hooked name=%j: mod keys are:', name, Object.keys(mod));
return mod;
}); |
This PR supersedes #88 and resolves #82.