Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Ensure we never mutate options passed to init #9162

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

mydea
Copy link
Member

@mydea mydea commented Oct 3, 2023

In various places we mutate the options passed to init(), which is a) not a great pattern and b) may break if users pass in frozen or similar objects (for whatever reason).

I also normalized this to ensure a passed in _metadata.sdk always takes precedent (we had this in most places, but not all).

Closes #9155

In various places we mutate the `options` passed to `init()`, which is a) not a great pattern and b) may break if users pass in frozen or similar objects (for whatever reason).
@mydea mydea requested review from Lms24 and AbhiPrasad October 3, 2023 08:17
@mydea mydea self-assigned this Oct 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2023

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 84.24 KB (-0.03% 🔽)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 31.41 KB (-0.02% 🔽)
@sentry/browser - Webpack (gzipped) 22 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 78.69 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 28.52 KB (-0.01% 🔽)
@sentry/browser - ES6 CDN Bundle (gzipped) 20.59 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 254.14 KB (0%)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 86.42 KB (0%)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 61.23 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 31.38 KB (-0.02% 🔽)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 84.27 KB (-0.01% 🔽)
@sentry/react - Webpack (gzipped) 22.05 KB (+0.06% 🔺)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 102.25 KB (+0.05% 🔺)
@sentry/nextjs Client - Webpack (gzipped) 51 KB (+0.1% 🔺)

@mydea mydea merged commit 25f1c49 into develop Oct 3, 2023
56 checks passed
@mydea mydea deleted the fn/avoid-mutate-options branch October 3, 2023 13:36
applyTunnelRouteOption(options);
buildMetadata(options, ['nextjs', 'react']);
const opts = {
environment: getVercelEnv(true) || process.env.NODE_ENV,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, @mydea !
Maybe I don't fully understand how it's supposed to work, but this breaks our app on update, cause we used to pass environment in options in sentry.client.config.ts, and now it seems to be ignored? It falls back to getVercelEnv in our case, which calls process, which doesn't exist in browser. I am not sure if it's a cause or maybe we're using it wrong and this code should never reach client?

We're getting Uncaught ReferenceError: process is not defined here:
image

Downgrading to 7.73.0, before this change, fixes it for us.
Any advise?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We object spread in the options, so an explicitly set environment should override what the sdk attempts to set.

const opts = {
  environment: getVercelEnv(true) || process.env.NODE_ENV,
  // environment defined on passed in options should override above
  ...options
}

Could you please open a GH issue with a minimal reproduction so we can help debug further? Thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it does override it potentially, but it fails before cause process doesn't exist in client. I am on the go now, will try to create an issue later. Thank you!

Sign up for free to 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.

Avoid mutating defaultIntegrations on options object
3 participants