Conversation
4a6b099 to
ea43514
Compare
| 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?
|
Will refactor after DefinitelyTyped/DefinitelyTyped#64879 gets merged in |
ffe8383 to
c9cb2e9
Compare
|
Validated with Testing this is such a pain, I'm going to do it in a follow-up PR so we can just ship this for now. Unit tests seem impossible, so integration tests have to be the way. I may even have to make this a e2e test... |
|
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