profiling-node should not shim cjs globals

[mbrevda]

(sorry, the regular templates weren't working for me - github was returning an ambiguous error).

As I try updating to v8 (8.24.0), I'm encountering an issue where profiling-node tries to shim cjs globals without first checking if they are set. This is done via the @rollup/plugin-esm-shim package in the rollup config, resulting in some cjs specific globals being injected even if they are already defined (see lines 19-21).

This is an issue because some bundlers require defining these globals manually (where they require defining the cjs globals), causing the Sentry variables to error with SyntaxError: Identifier '__filename' has already been declared

I'd expect

that Sentry should not define global variables. If these variables are crucial and there is no other way to define them, respectfully check if they are set before defining them.

var __filename = cjsUrl.fileURLToPath(import.meta.url);
    ^

SyntaxError: Identifier '__filename' has already been declared
    at compileSourceTextModule (node:internal/modules/esm/utils:337:16)
    at ModuleLoader.moduleStrategy (node:internal/modules/esm/translators:163:18)
    at callTranslator (node:internal/modules/esm/loader:430:14)
    at ModuleLoader.moduleProvider (node:internal/modules/esm/loader:436:30)

Thanks!

[andreiborza]

Hi, thanks for writing in. I've kicked this up to the respective team, but generally I'd agree we should guard against this.

We'll take a look.

[JonasBa]

@mbrevda thanks for opening the issue, the SDK should indeed handle this. I'm making the change to only conditionally apply the shims and will try and get it merged asap so it can go out with the next minor release

[mbrevda]

Awesome, looking forward. Thanks for the quick reply!

[mbrevda]

hi - how's this coming along?

[s1gr1d]

Hello @mbrevda, we currently have a PR for this: https://github.com/getsentry/sentry-javascript/pull/13267

[mbrevda]

Thanks, any idea what the hold is there?

[Lms24]

Hi, the PR is still under review. Not entirely sure about specifics there but @AbhiPrasad took a look yesterday. Feel free to subscribe to this issue as it's gonna be closed once the PR gets merged.

[JonasBa]

@mbrevda sorry I was ooo last week, let me take a look at this now.

[JonasBa]

@mbrevda PR is ready and CI is passing. We'll get this merged tomorrow and ship it with the next release, sorry for the wait!

[AbhiPrasad]

Fix released with https://github.com/getsentry/sentry-javascript/releases/tag/8.30.0 - thanks for your patience everyone!