. in exports property of CJS file breaks dynamic imports

[MattIPv4]

Is there an existing issue for this?

• I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
• I have reviewed the documentation https://docs.sentry.io/
• I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

@sentry/node

SDK Version

8.11.0

Framework Version

No response

Link to Sentry event

No response

SDK Setup

Sentry.init();

Steps to Reproduce

package.json:

{
  "type": "module",
  "dependencies": {
    "@sentry/node": "^8.11.0"
  }
}

index.js:

import * as Sentry from '@sentry/node';

const main = async () => {
  Sentry.init();

  const other = await import(new URL('./other.cjs', import.meta.url));
  console.log(other);
};

main();

other.cjs:

exports['one.two'] = () => console.log('b');

Expected Result

It runs, outputting:

[Module: null prototype] {
  default: { 'one.two': [Function (anonymous)] },
  'one.two': [Function (anonymous)]
}

(this can be reproduced by commenting out the Sentry.init(); line)

Actual Result

It fails on a sytax error:

file://[snip]/other.cjs?iitm=true:21
      let $one.two = _.one.two
              ^

SyntaxError: Unexpected token '.'
    at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:168:18)
    at callTranslator (node:internal/modules/esm/loader:279:14)
    at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:285:30)
    at async link (node:internal/modules/esm/module_job:78:21)

Node.js v20.12.2

[MattIPv4]

For some additional context, this works fine in @sentry/node@7. This is blocking our upgrade to @sentry/node@8. In our actual code, the usage of . in exports comes from files generated by grpc-tools, which are then run through @grpc/grpc-js.

[AbhiPrasad]

Sorry for the trouble, this is def some issues with the new automatic instrumentation in 8.x.

Could you apply patch-package with the linked patch here #12485 (comment) and see if the error goes away?

[MattIPv4]

I had to install import-in-the-middle@1.8.0 as the patch failed on 1.8.1, so my package.json now looks like this (from the repro above):

{
  "type": "module",
  "scripts": {
    "postinstall": "patch-package"
  },
  "dependencies": {
    "@sentry/node": "^8.11.0",
    "import-in-the-middle": "^1.8.0",
    "patch-package": "^8.0.0"
  }
}

Running npm i shows that patch-package is applying the patch, however node index.js still results in the same syntax error as above.

[timfish]

This is currently tracked upstream by nodejs/import-in-the-middle#94

I've been thinking of some ways to work around this and should get a PR submitted this week!

[timfish]

nodejs/import-in-the-middle#115

[timfish]

v8.29.0 of the Node SDK added a new onlyIncludeInstrumentedModules option to registerEsmLoaderHooks.

import * as Sentry from '@sentry/node';

Sentry.init({
  dsn: '__PUBLIC_DSN__',
  registerEsmLoaderHooks: { onlyIncludeInstrumentedModules: true },
});

When set to true, import-in-the-middle will only wrap ESM modules that are specifically instrumented by OpenTelemetry plugins. This is useful to avoid issues where import-in-the-middle is not compatible with some dependencies.

This feature will only work if you Sentry.init() the SDK before the instrumented modules are loaded. This can be achieved via the Node --register and --import CLI flags or by loading your app code via async await import() after calling Sentry.init().

Please let me know if this helps work around the import-in-the-middle incompatibility issues!

[MattIPv4]

👀 This seems to work just fine, but I am curious what the ramifications are of setting this. What exactly stops working by no longer instrumenting these files? I assume something does, otherwise this'd be a default?

[timfish]

By default import-in-the-middle wraps every ES module (ie. every ESM file that loaded) with another module which proxies the exports which allows them to be modified after they've already been loaded. This is important for ESM because all imports are evaluated and loaded before any code executes.

import-in-the-middle means that Sentry can instrument the node:net module below even though it's loaded before the Sentry code runs.

import * as Sentry from '@sentry/node';
import * as net from 'node:net';

Sentry.init({});

There are a couple of fundamental issues with import-in-the-middle that mean it can break modules it wraps because it changes the behaviour of the exports. One example of this exported primitives. The ESM spec says that exported primitives should be treated like references which means they become internally mutable inside the module. There is no way for import-in-the-middle to wrap an exported primitive without copying it which breaks this behaviour.

If you enable the onlyIncludeInstrumentedModules option, as the OpenTelemetry instrumentations are initialised, we notify import-in-the-middle of the specific modules we would like to be wrapped so we can instrument them. This doesn't fix the issues with import-in-the-middle, it just means we only need it to work with the libraries that we intend to instrument.

The only downside with this option is that Sentry needs to be initialised before any module we want to instrument is loaded. This can be achieved by loading Sentry via --register or --import which is the recommended approach in the docs. You can also async load your app code after Sentry.init():

import * as Sentry from '@sentry/node';

Sentry.init({});

await import('./my-app');

I assume something does, otherwise this'd be a default?

If we still need to use import-in-the-middle to instrument ESM libraries when the next major version of the SDK is released, it will almost certainly become the default. It's a breaking change because the Node SDK does work in a lot of cases without this option enabled. I'm still looking at ways we can improve this and enable it automatically.