-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Crash with Vercel AI SDK when used in ESM #16137
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
Comments
Hey, thanks for writing in and for the reproduction! I'll take a look and might forward the issue to the Spotlight team, depending on results :) |
Some initial findings: I was able to further narrow this down to the following SDK config: import * as Sentry from '@sentry/node'
Sentry.init({
dsn: '_any_valid_dsn_',
defaultIntegrations: false,
integrations: [
Sentry.vercelAIIntegration(),
],
})
await import("./otherfile.js") // which imports 'ai' which means that my suspicion as of this moment is that This happens
I still have to find out why exactly this is happening but I'd bet on UPDATE: Some more questions:
|
I gotta stand corrected: The reason why both, setting So my now changed slightly: I think patching the |
Okay, so I debugged this a bit further and created a new, reproduction. One that takes the general Sentry SDK out of the equation but only has 3 important components:
Here's a new reproduction (thanks @Jaakkonen for the original one! I based mine on yours): https://github.com/Lms24/gh-sentry-javascript-16137-vercel-ai-esm I threw in a couple of logs in the IITM method where the error is thrown: set (target, name, value) {
console.trace('set');
console.log('> target', target);
console.log('> name', name);
console.log('> value', value);
console.log('> setters', setters);
console.log('> setters.get(target)', setters.get(target));
console.log('> setters.get(target)[name]', setters.get(target)[name]);
return setters.get(target)[name](value)
}, and I'm getting the following output: Log Output
So based on my limited understanding of IITM, it seems like for some reason it is trying to set a I will ask @timfish and @AbhiPrasad - could you please take a look at this? I'm not sure if this is an IITM bug or our instrumentation is causing this. |
I think it is caused by this code which reconstructs the module as an object: sentry-javascript/packages/node/src/integrations/tracing/vercelai/instrumentation.ts Lines 90 to 95 in 8dd90ae
I don't think a module can be represented with a pure object. Check out the craziness iitm does to "look" like a module in it's wrapping code: // Mimic a Module object (https://tc39.es/ecma262/#sec-module-namespace-objects).
const _ = Object.create(null, { [Symbol.toStringTag]: { value: 'Module' } }) It doesn't look like there's a need to construct a new module, you can just overwrite the functions and return the original module: for (const method of INSTRUMENTED_METHODS) {
moduleExports[method] = generatePatch(method);
}
return moduleExports; I'll test this and submit a PR! |
I was wrong! So my changes fix ESM, but CJS is broken because the exports are only getters. This explains why the original code recreated the module as an object. Without setters, there's no way to replace them on the original object! Looks like we need different hooking strategies for ESM/CJS... |
- Closes #16137 With ESM patching you need to retain the original module and just overwrite the exports you want to wrap. This usually works for CJS too but unfortunately, the CJS exports of `ai` only have getters and no setters so this route is not possible. This PR changes the patching so that it works slightly differently for ESM and CJS. The original code that outputs a newly created object is retained for CJS but for ESM we use the preferred route of replacing the required exports. To detect whether the module we're patching is an ES module we check `Object.prototype.toString.call(moduleExports) === '[object Module]'` which is documented [here](https://tc39.es/ecma262/#sec-module-namespace-objects). This PR also adds an ESM test for `ai`.
A PR closing this issue has just been released 🚀This issue was referenced by PR #16152, which was included in the 9.15.0 release. |
Thank you @timfish , @Lms24 and @Jaakkonen ! Absolute legends 👏 |
Is there an existing issue for this?
How do you use Sentry?
Sentry Saas (sentry.io)
Which SDK are you using?
@sentry/node
SDK Version
9.14.0
Framework Version
No response
Link to Sentry event
No response
Reproduction Example/SDK Setup
https://github.com/Jaakkonen/sentry-spotlight-vercel-ai-bug
Steps to Reproduce
Expected Result
Code to not crash. With spotlight disabled the code runs fine
Actual Result
The text was updated successfully, but these errors were encountered: