Skip to content

chore(node-core): Move isCjs to its own file to prevent sideEffects - #21856

Merged
JPeer264 merged 2 commits into
developfrom
jp/side-effects
Jul 2, 2026
Merged

chore(node-core): Move isCjs to its own file to prevent sideEffects#21856
JPeer264 merged 2 commits into
developfrom
jp/side-effects

Conversation

@JPeer264

@JPeer264JPeer264 commented Jun 30, 2026

Copy link
Copy Markdown
Member

It seems that node-core has ../nodeVersion.js as a sideEffect: https://unpkg.com/@sentry/node-core@10.62.0/build/esm/integrations/modules.js

Actually nodeVersion doesn't do anything, so it is not really a side effect. But I also couldn't get rid of that import with the PURE comments - so if someone has a better idea, I'm up for it

This came up while implementing a test for Cloudflare, where I used the Prisma integration within Cloudflare:

Screenshot 2026-06-30 at 10 54 39

@JPeer264
JPeer264 requested review from Lms24 and timfishJune 30, 2026 08:55
@JPeer264JPeer264 self-assigned this Jun 30, 2026
@JPeer264
JPeer264 requested a review from a team as a code ownerJune 30, 2026 08:55
@JPeer264
JPeer264 requested review from andreiborza and mydea and removed request for a teamJune 30, 2026 08:55
@mydea

Copy link
Copy Markdown
Member

IMHO we should find a way to properly fix this. likely this is not possible here, so this is OK for now, but I'd vote to add an actual side-effect free (e.g. a getNodeVersion() method that memoizes/caches the parsed version on call) to use as a replacement and then remove the NODE_VERSION constant in v11 🤔

JPeer264 added a commit that referenced this pull request Jun 30, 2026
## Background
same as with #21856 I came across this with adding the Prisma
integration to Cloudflare.
<img width="822" height="428" alt="Screenshot 2026-06-30 at 11 23 57"
src="https://github.com/user-attachments/assets/5eff699b-6275-4b99-8627-9062a0ee7e5d"
/>
## What has changed
The `@sentry/opentelemetry` SDK has some side effect imports, which
don't do anything:
https://unpkg.com/@sentry/opentelemetry@10.62.0/build/esm/index.js
those are the imports
```js
import '@sentry/conventions/attributes';
import '@opentelemetry/core';
import '@opentelemetry/sdk-trace-base';
```
`hoistTransitiveImports` has been added to get rid of the side effect
imports. However, I'm confused with this option, as [the
docs](https://rollupjs.org/configuration-options/#output-hoisttransitiveimports)
say that this option is actually ignored when `preserveModules` have
been set (but it still has an effect).
Also here, if someone has a better idea - I'm all ears.
@JPeer264JPeer264 changed the title chore(node-core): Add nodeVersion.js in sideEffect arraychore(node-core): Move isCjs to its own file to prevent sideEffectsJul 2, 2026
@JPeer264

Copy link
Copy Markdown
MemberAuthor

I changed the logic. sideEffects stays false and I moved isCjs in its own file, as the import of ../nodeVersion always created a sideEffect. With that the side-effect import is gone from the build output

@JPeer264
JPeer264 merged commit c73aec4 into developJul 2, 2026
206 checks passed
@JPeer264
JPeer264 deleted the jp/side-effects branch July 2, 2026 13:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@JPeer264@mydea@timfish@Lms24@andreiborza