-
Notifications
You must be signed in to change notification settings - Fork 832
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
wrong import for "import-in-the-middle" #3954
Comments
We're having the same problem over at https://github.com/highlight/highlight It just kicked up when we started consuming the CJS version instead of the ESM. |
I made a quick edit to I changed the import to It worked great. |
@deltaepsilon @matthias-pichler-warrify I think the latest release should have resolved this |
Awesome! I'm working on this now. I'll update and report back. |
@dyladan Any idea on when |
Oh I'm sorry I conflated this with a different issue. @trentm do you have any idea what's going on here? I vaguely remember a similar problem with |
I'll make some guesses, but I'm not that confident with either esbuild bundling, or with the "build/esm/..." output of OTel packages.
I'm not sure that is necessarily the same issue as here. If it is the same issue, then I think you would be broken if If someone hitting this error created a full repro with the specific commands to run, that would help debug this. |
Oh, actually this looks more similar: #3701 The fix/workaround then was to:
To be honest I get a little lost in the "TypeScript import syntax" -> "commonjs and/or esm built code" -> "use of that code in |
@trentm I feel your pain regarding ESM/CJS/Whatever-else-they've-come-up-with. I solved the problem for my local environment by editing the import deep in down in That was all the bundler needed to be happy again. Of course, the moment I send this to our CI pipeline it'll install the package from scratch. |
Running into this same issue trying to use opentelemetry in a project built using Would be awesome to have a "real" fix and not require the hackaround |
@pichlermarc - Is there a reason why this: Line 28 in 20182d8
Was not updated to this: import { ImportInTheMiddle } from 'import-in-the-middle'; In the latest release? Because at runtime this will absolutely lead to an error being thrown for ESM based applications. |
Already posted in a proposed fix PR but I wanted to put this here too to get more eyes on it: We're having a hard time validating this fix because we don't have a reproducer. Does anyone have a repro we can use? I'm also not convinced that after we fix this error your problems will be solved. To my knowledge, bundlers work by removing import/require statements and inlining code. Our instrumentations rely on the functioning of those statements so it may turn out that once this problem is fixed the instrumentations may not work anyway.
This is 100% what I would prefer. We also talked about changing the web instrumentations to not use the same instrumentation base class as node instrumentations because they work differently (no require/import capture), which would also resolve this issue at least in web. |
What happened?
Steps to Reproduce
Import from
"@opentelemetry/instrumentation"
and bundle using esbuild with format "cjs"Expected Result
The build should proceed without warning and not crash during runtime
Actual Result
The build produces an error and crashes during runtime
Additional Details
During build time the following output is shown
OpenTelemetry Setup Code
package.json
Relevant log output
Our Lambda functions are instrumented with datadog, hence the stack trace shows datadog files all the way
The text was updated successfully, but these errors were encountered: