Uh oh!
There was an error while loading. Please reload this page.
feat(node): Undici integration - #7582
Conversation
4a6b099 to
ea43514Compare| let ds: typeof DiagnosticsChannel | undefined; | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| ds = require('diagnostics_channel') as typeof DiagnosticsChannel; |
There was a problem hiding this comment.
Can we use await import here? Because of e.g. this comment:
https://github.com/getsentry/sentry-javascript/blob/develop/packages/node/src/integrations/localvariables.ts#L33
There was a problem hiding this comment.
I don't think so, because we need to support cjs.
I think that docstring is no longer an issue - but I'll let @Lms24 confirm that.
There was a problem hiding this comment.
IIRC await import would actually be transpiled to Promise.resolve(() => require(..)) for our CJS build. The reason why we switched back to require is that only require'd modules are monkey-patchable while imported ones are not. So I'd say we probably can try await import here as we don't monkey-patch the required module.
However, we also don't have to if it the async nature of import makes things harder here. For instance, I ran into timing problems when trying to convert the LocalVariables integration to import and decided to leave it as is and revisit later.
Thanks for the comment link. I need to change the comment as I don't think it actually is true for SvelteKit anymore. I added this comment before I realized that yarn link caused these weird cjs/esm problems in SvelteKit.
There was a problem hiding this comment.
IMHO if we can still make it work with import, I'd still do it - the less require we have in our code base, the better!
Lms24
left a comment
There was a problem hiding this comment.
Nice! If this is hard to unit-test, maybe we just add integration tests?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
AbhiPrasad
commented
Mar 24, 2023
Will refactor after DefinitelyTyped/DefinitelyTyped#64879 gets merged in |
ffe8383 to
c9cb2e9CompareAbhiPrasad
commented
Mar 27, 2023
AbhiPrasad
commented
Mar 27, 2023
Extracted out follow up tasks into #7624 |

ref #6939
ref #7526
This PR adds support for Undici. This is required for us to instrument server-side fetch, introduced with Node 18+!
Requires Undici v4.7.0 and above.
SvelteKit uses Undici for fetch, and this will also help us with Next.js instrumentation.
I keep getting reminded of https://en.wikipedia.org/wiki/Jodeci whenever I work on this.
Follow up tasks: #7624