fix: use require for import in the middle

[dyladan]

Fixes an issue where bundlers (esbuild) fail to build because of es module interop issues.

Import in the middle does not have a default export, which means import IITM from 'import-in-the-middle'; fails. Importing as a namespace with import * as IITM from 'import-in-the-middle'; works, but causes issues with bundlers. Specifically, namespace imports are required by the ESM spec to be objects. IITM's top-level export is not an object, but a callable function (https://github.com/DataDog/import-in-the-middle/blob/c3c2c52c1915b47994af59d507c59029c1f1fae9/index.js#L91).

This draft is to consider the possibility of using require instead of import for this dependency.

Related to #4519

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.83%. Comparing base (e49c4c7) to head (6ffda27).
Report is 7 commits behind head on main.

❗ Current head 6ffda27 differs from pull request most recent head 2d58474

Please upload reports for the commit 2d58474 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4546      +/-   ##
==========================================
- Coverage   93.32%   92.83%   -0.50%     
==========================================
  Files          84      328     +244     
  Lines        1694     9486    +7792     
  Branches      349     2035    +1686     
==========================================
+ Hits         1581     8806    +7225     
- Misses        113      680     +567     

see 269 files with indirect coverage changes

[jd-carroll]

@dyladan can you change this from a draft PR to an actual PR?

A change to this effect really needs to be merged.

[dyladan]

@dyladan can you change this from a draft PR to an actual PR?

A change to this effect really needs to be merged.

Yeah i'll update it

[ericallam]

It would be great to get this merged, as trigger.dev is currently blocked from bundling user code with esbuild into ESM, as we rely on @opentelemetry/* to power all our logging for our background task platform. Building user code into CJS comes with all sorts of problems (CJS cannot import ESM only modules).

[mydea]

+1, we also have reports of this breaking users, e.g. see getsentry/sentry-javascript#12009 - would be great to get this merged and released ❤️

[pichlermarc]

I've been digging through lots of issues/PRs today that have been opened about this (#4717, #3954, #4519, #4691) but I'm still trying to grasp if this would actually solve the problem or if this would just get rid of the warning and cause us to run into different problems. All esbuild configs that I've tried to come up with fail at a way different point before I can even see this warning - I must be using different options/doing something wrong.

In reviewing I'm limited by my lack of familiarity with esbuild: Is there any somewhat minimal reproducer anywhere (that I can run locally) which I can use to verify this fix?

[mydea]

I've been digging through lots of issues/PRs today that have been opened about this (#4717, #3954, #4519, #4691) but I'm still trying to grasp if this would actually solve the problem or if this would just get rid of the warning and cause us to run into different problems. All esbuild configs that I've tried to come up with fail at a way different point before I can even see this warning - I must be using different options/doing something wrong.

In reviewing I'm limited by my lack of familiarity with esbuild: Is there any somewhat minimal reproducer anywhere (that I can run locally) which I can use to verify this fix?

Not sure if this helps, but @timfish has combined some stuff together:
getsentry/sentry-javascript#12154 (comment)

Which seems to work for some people, at least (e.g. getsentry/sentry-javascript#12154 (comment))

[timfish]

I don't think any of the existing PRs will fix this. My best guess is that esbuild closely follows the spec whereas other bundlers paper over the mess.

The obvious thing to do would be to fix the exports in import-in-the-middle and release a V2 that doesn't have invalid exports!

[dyladan]

We're having a hard time validating this fix because we don't have a reproducer. @mydea @ericallam do you 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.

The obvious thing to do would be to fix the exports in import-in-the-middle and release a V2 that doesn't have invalid exports!

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.

[pichlermarc]

Closing this as I think this has been resolved by #4745. Feel free to re-open if that's incorrect 🙂