Filter out ESM-incompatible defaultIntegrations when running in ESM

[Lms24]

Problem Statement

While working on #16137 we realized that quite a lot of Otel instrumentation is currently not compatible with ESM. In some (all?) cases this goes as far as the application breaking if the instrumentation tries to patch a package but isn't compatible with the ESM exports

Solution Brainstorm

We should avoid trying to apply instrumentations known to break ESM apps. Currently I'm we're thinking of creating a denyList of defaultIntegrations to filter out. Let's only apply this to default integrations. If users specifically add an integration, this is on them.

This also involves adding more tests for our integrations in both CJS and ESM to be aware of this.

[timfish]

Part of the problem is that it's possible to start Sentry in one mode (esm/cjs) and then use code in the other. The mode you start Sentry in is just a strong hint at what you might be using most.

[Lms24]

Do you mean because of require(esm)?

[timfish]

Do you mean because of require(esm)?

The issues existed before require(esm), since you could already import(cjs), it just hasn't impacted us much.

Currently, if we detect ESM at init, we don't include the modulesIntegration. However, it is still possible to load and start this integration if we force it to CJS via import { modulesIntegration } from './modules.cjs'. It would work and list all the modules used by all the CJS code called from ESM. It would give partial, hence likely useless module information, but it's still possible to load and use it.

Users can node --require sentry.js app.mjs and we'd detect CJS at init but the app is ESM. ESM instrumentation wouldn't work.

For Next.js, all user code is transpiled to CJS, so at init we detect CJS, but Next.js core is actually ESM and loads some stuff via await import('./something'). Because we detect CJS we don't enable the ESM loader hooks so we don't instrument anything ESM. Next.js users might start using ESM only libraries via require(esm) and we don't have any hooks setup.

My main point is that we need to be careful about ESM/CJS detection at init time because with both import(cjs) and require(esm) available now, it doesn't tell us the whole story!

[Lms24]

Thanks for sharing this! What a mess 😭

So IIUC, deactivating the integration is risky because our ESM detection could be off, for example if people require the module and instrumentation wouldn't even fail (?). Or vice versa I guess in some other cases...

So maybe we should do something less drastic, something along the lines of emitting a debug log in the integration:

[Sentry] [integrationName] seems to be used in ESM-mode but is not compatible with ESM. If your process crashes, try deactivating [integrationName]

[mydea]

That... all sucks a lot xD

I am a bit torn, it still seems like it would be the better experience to not add these integrations if we detect ESM 🤔 out of the box, nothing will break at least. Users can then manually add the integrations back if they now it works (or try it out....)

So what about this concrete idea:

• If we detect ESM mode (best effort, as now) we remove integrations that are known to not work with ESM from the default integrations.
• We also log a message about this - e.g. because we detect your app to be running in ESM mode, the xxxIntegration was not enabled by default because it is known to cause issues with ESM setups. You can manually add it to your "integrations: []" array to try if it works with your setup or something along these lines?

[timfish]

Which integrations have issues with ESM?

[mydea]

I have only started to try it out, for now I identified dataloader and connect that seem to fail, see failsOnEsm: true tests in node-integration-tests (https://github.com/search?q=repo%3Agetsentry%2Fsentry-javascript%20failsOnEsm&type=code)

[timfish]

Ah I missed that option!

We might also have some instrumentation that are just missing the ESM test completely.