feat: Stop passing `defaultIntegrations` as client option #15307

mydea · 2025-02-05T13:07:01Z

This PR updates the handling of defaultIntegrations to avoid our own SDKs ever setting this option. Instead, we manually do this and expose a new initWithDefaultIntegrations method which is designed for internal use. We already have something similar (which in that case is replaced by the new method) in node, to allow tree shaking of integrations for serverless SDKs.

Status Quo

Today, the way we handle default integrations is a bit non-ideal:

We have an option called defaultIntegrations which users can set to false to opt out of any default integrations.
However, that property also allows to pass Integrations[], which is not really something we want any user to do (just set defaultIntegrations: false and pass integrations: [...] if you want to do this), but more something that we use internally in the SDKs. However, this leads to a bit of an overloaded behavior there.

Especially, because we allow users to overwrite this, but need to differentiate if this has been inherited by an upstream SDK. This is because e.g. the next SDK (and many others) does this:

const opts = {
    environment: getVercelEnv(true) || process.env.NODE_ENV,
    defaultIntegrations: getDefaultIntegrations(options),
    ...options,
  } satisfies BrowserOptions;

This is actually technically false, because if a user does this: init({ defaultIntegrations: undefined }) this will result in no default integrations being set, instead of using the default ones.

We have partially fixed this in some places by specifically checking for undefined there etc, but overall this is just not an ideal design.

Additionally, it also has the problem that you cannot tree shake default integrations of upstream SDKs. E.g. the angular SDK has its own set of default integrations, skipping BrowserApiErrors. However, with the current design, that integration is still included in the bundle.

New behavior in this SDK

For example, this is the old vs new behavior:

// old
const opts = {
  defaultIntegrations: getDefaultIntegrations(),
  ...options,
};

applySdkMetadata(opts, 'angular');

checkAndSetAngularVersion();
return browserInit(opts);

// new
const opts = {
  ...options,
};

applySdkMetadata(opts, 'angular');

checkAndSetAngularVersion();

return initWithDefaultIntegrations(opts, getDefaultIntegrations);

In the places where an actual client is constructed, we pass these in directly now, like this:

const integrations =  getIntegrationsToSetup(options, defaultIntegrations);

And this function, in turn, can now properly check the passed in user options, ensuring they have precedence, and use the SDK defaultIntegrations where appropriate.

Note: initWithDefaultIntegrations accepts a getter method instead of just taking the array of integrations, because in some cases we build default integrations based on options, and we want to have the fully built options there with all defaults etc. applied, so that this is definitely correct. It has the additional benefit that it is easier to tree shake.

Related to #14950

github-actions · 2025-02-05T13:13:29Z

size-limit report 📦

⚠️ Warning: Base artifact is not the latest one, because the latest workflow run is not done yet. This may lead to incorrect results. Try to re-run all tests to get up to date results.

Path	Size	% Change	Change
@sentry/browser	22.98 KB	+0.32%	+74 B 🔺
@sentry/browser - with treeshaking flags	22.74 KB	+0.2%	+46 B 🔺
@sentry/browser (incl. Tracing)	35.85 KB	+0.21%	+75 B 🔺
@sentry/browser (incl. Tracing, Replay)	72.63 KB	+0.07%	+45 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	66.07 KB	+0.02%	+7 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas)	76.88 KB	+0.07%	+50 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback)	89.62 KB	+0.06%	+53 B 🔺
@sentry/browser (incl. Feedback)	39.92 KB	+0.17%	+68 B 🔺
@sentry/browser (incl. sendFeedback)	27.59 KB	+0.22%	+60 B 🔺
@sentry/browser (incl. FeedbackAsync)	32.38 KB	+0.2%	+64 B 🔺
@sentry/react	24.8 KB	+0.26%	+64 B 🔺
@sentry/react (incl. Tracing)	37.73 KB	+0.15%	+57 B 🔺
@sentry/vue	27.12 KB	+0.09%	+23 B 🔺
@sentry/vue (incl. Tracing)	37.56 KB	+0.22%	+81 B 🔺
@sentry/svelte	23.02 KB	+0.33%	+76 B 🔺
CDN Bundle	24.12 KB	-0.07%	-16 B 🔽
CDN Bundle (incl. Tracing)	35.84 KB	-0.03%	-9 B 🔽
CDN Bundle (incl. Tracing, Replay)	70.47 KB	-0.04%	-22 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback)	75.64 KB	+0.03%	+17 B 🔺
CDN Bundle - uncompressed	70.58 KB	-0.02%	-13 B 🔽
CDN Bundle (incl. Tracing) - uncompressed	106.48 KB	+0.01%	+8 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed	217.34 KB	+0.01%	+9 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	229.9 KB	+0.01%	+9 B 🔺
@sentry/nextjs (client)	38.69 KB	+0.13%	+48 B 🔺
@sentry/sveltekit (client)	36.27 KB	+0.18%	+64 B 🔺
@sentry/node	156.63 KB	+0.04%	+53 B 🔺
@sentry/node - without tracing	97.63 KB	+0.06%	+50 B 🔺
@sentry/aws-serverless	107.12 KB	+0.06%	+59 B 🔺

View base workflow run

codecov · 2025-02-05T13:16:41Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
700	1	699	301

View the top 1 failed test(s) by shortest run time

sessions/initial-scope/test.ts should start a new session with navigation.

Stack Traces | 0.375s run time

test.ts:19:11 should start a new session with navigation.

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

mydea · 2025-02-05T13:35:01Z

packages/browser/src/sdk.ts

@@ -59,27 +58,10 @@ export function applyDefaultOptions(optionsArg: BrowserOptions = {}): BrowserOpt

  return {
    ...defaultOptions,
-    ...dropTopLevelUndefinedKeys(optionsArg),


this should not be necessary anymore with this change!

Lms24

Initial impression: I think this is a good change and I'm looking forward to seeing the Integration[] being removed from defaultIntegrations. This should rease a potential footgun from the API surface.

I think the initWithDefaultIntegrations pattern is fine and if we can reduce bundle size with it (as in the Angular case), it justifies the added API surface.

Lms24 · 2025-02-06T10:55:41Z

packages/angular/src/sdk.ts

  checkAndSetAngularVersion();
-  return browserInit(opts);
+
+  return initWithDefaultIntegrations(opts, getDefaultIntegrations);


now that I see this: is it even correct to call checkAndSetAngularVersion before initWithDefaultIntegrations (or formerly browserInit)? Given that this calls setContext before the actual client exists...

I guess it somehow works but probably it's better to first call init, then checkSetNgVersion and then return the client

this should be fine because the scope remains valid even before the client is created (one of the particular design goals we considered with the current scope stuff 😅 )

Lms24 · 2025-02-06T11:03:09Z

packages/browser/src/sdk.ts

+/**
+ * Initialize a browser client with the provided options and default integrations getter function.
+ */
+export function initWithDefaultIntegrations(


l: can we add a test or two for this function?

packages/cloudflare/src/sdk.ts

Lms24 · 2025-02-06T11:11:46Z

packages/core/src/types-hoist/options.ts

+   * It is deprecated to pass `Integrations[]` here. This capability will be removed in v10.
+   *
+   * TODO(v10): Remove `Integration[]` support.
   */
  defaultIntegrations?: false | Integration[];


l: not sure if you already tried this but if we split up the type and union-type it again, can we somehow add @deprecated to the Integration[] type? might not work, so feel free to disregard.

I tried a few ways but could not get it to work 😬 if we do e.g. this:

/** @deprecated oh no */ type DeprecatedStuff = Integration[]; // ... interface { defaultIntegrations: false | DeprecatedStuff; }

then it complains in there that this is deprecated (so we need to add an eslint-disable comment), but for users its fine to pass this 😬

Lms24 · 2025-02-06T11:14:18Z

packages/core/src/integration.ts

+  // TODO(v10): If an array is passed, we use this - this is deprecated and will eventually be removed
+  // Else, we use the default integrations that are directly passed to this function as second argument
+  const defaultIntegrationsToUse =
+    options.defaultIntegrations === false
+      ? []
+      : Array.isArray(options.defaultIntegrations)
+        ? options.defaultIntegrations
+        : defaultIntegrations;


l: If we can't get my suggestion with deprecated to work, wdyt about adding a log here that passing an array will be removed in v10? Again, no super strong feelings but I'm fairly sure that people (mis-)use this option quite a lot which is why I'm thinking it's worth thinking about this.

Lms24 · 2025-02-06T11:16:28Z

packages/browser/src/sdk.ts

+/**
+ * Initialize a browser client with the provided options and default integrations getter function.
+ */


l: Based on your PR description:

Suggested change

/**

* Initialize a browser client with the provided options and default integrations getter function.

*/

/**

* Initialize a browser client with the provided options and default integrations getter function.

*

* @hidden

* @internal - This is only intended for internal use. Use at your own risk!

*/

mydea · 2025-02-07T11:58:04Z

I added tests here, should be good to go!

mydea self-assigned this Feb 5, 2025

mydea marked this pull request as draft February 5, 2025 13:07

mydea commented Feb 5, 2025

View reviewed changes

mydea force-pushed the fn/no-more-defaultIntegrations branch 6 times, most recently from 08ca723 to 9d11a83 Compare February 6, 2025 11:11

Lms24 reviewed Feb 6, 2025

View reviewed changes

mydea added 4 commits February 6, 2025 13:59

feat: Stop passing defaultIntegrations as client option

6ef8c8c

small ref

b78740c

fix test

7e2a589

adjustments

e507097

mydea force-pushed the fn/no-more-defaultIntegrations branch from 9d11a83 to e507097 Compare February 6, 2025 12:59

fix & add test

7fd601a

mydea marked this pull request as ready for review February 7, 2025 11:57

mydea mentioned this pull request Feb 7, 2025

feat(core): Allow to configure disableIntegrations to opt-out of default integrations #15300

Draft

Lms24 approved these changes Feb 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Stop passing `defaultIntegrations` as client option #15307

feat: Stop passing `defaultIntegrations` as client option #15307

mydea commented Feb 5, 2025

github-actions bot commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading

mydea Feb 5, 2025

Lms24 left a comment

Lms24 Feb 6, 2025

mydea Feb 6, 2025

Lms24 Feb 6, 2025

Lms24 Feb 6, 2025

mydea Feb 6, 2025

Lms24 Feb 6, 2025

Lms24 Feb 6, 2025

mydea commented Feb 7, 2025

feat: Stop passing defaultIntegrations as client option #15307

Are you sure you want to change the base?

feat: Stop passing defaultIntegrations as client option #15307

Conversation

mydea commented Feb 5, 2025

Status Quo

New behavior in this SDK

github-actions bot commented Feb 5, 2025 • edited Loading

size-limit report 📦

codecov bot commented Feb 5, 2025 • edited Loading

❌ 1 Tests Failed:

Choose a reason for hiding this comment

Lms24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mydea commented Feb 7, 2025

feat: Stop passing `defaultIntegrations` as client option #15307

feat: Stop passing `defaultIntegrations` as client option #15307

github-actions bot commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading