Skip to content

fix(browser): Move browserTracingIntegration code to setup hook #16386

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

Merged
merged 3 commits into from
May 26, 2025

Conversation

mydea
Copy link
Member

@mydea mydea commented May 26, 2025

This PR removes the warning for multiple browserTracingIntegrations being setup.
We realised this warning was actually just a symptom of us running a bunch of the integration code in the function itself, not int setup(). This lead to stuff running twice when users passed in a custom integration in addition to a default one, because the logic is basically:

const defaultIntergations = [browserTracingIntegration()];
const usedDefinedIntegrations = [browserTracingIntegration()];
// at this point, the function was already executed twice, leading to side effects even without calling `init`!

Now, we move all the logic to setup/setupAfterAll, so this should be safe. This means that the integration will be deduped anyhow (I added a test to make sure this is the case), and we no longer need the warning.

fixes #16369

@mydea mydea requested review from Lms24 and s1gr1d May 26, 2025 09:13
@mydea mydea self-assigned this May 26, 2025
Copy link
Contributor

github-actions bot commented May 26, 2025

size-limit report 📦

Path Size % Change Change
@sentry/browser 23.95 kB - -
@sentry/browser - with treeshaking flags 23.72 kB - -
@sentry/browser (incl. Tracing) 38.24 kB -0.08% -29 B 🔽
@sentry/browser (incl. Tracing, Replay) 76.37 kB -0.03% -19 B 🔽
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 69.48 kB -0.04% -22 B 🔽
@sentry/browser (incl. Tracing, Replay with Canvas) 81.14 kB -0.04% -30 B 🔽
@sentry/browser (incl. Tracing, Replay, Feedback) 93.22 kB -0.05% -38 B 🔽
@sentry/browser (incl. Feedback) 40.72 kB - -
@sentry/browser (incl. sendFeedback) 28.68 kB - -
@sentry/browser (incl. FeedbackAsync) 33.57 kB - -
@sentry/react 25.73 kB - -
@sentry/react (incl. Tracing) 40.21 kB -0.11% -41 B 🔽
@sentry/vue 28.35 kB - -
@sentry/vue (incl. Tracing) 40.05 kB -0.12% -47 B 🔽
@sentry/svelte 23.98 kB - -
CDN Bundle 25.25 kB - -
CDN Bundle (incl. Tracing) 38.38 kB -0.01% -3 B 🔽
CDN Bundle (incl. Tracing, Replay) 74.24 kB -0.03% -16 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback) 79.66 kB -0.02% -12 B 🔽
CDN Bundle - uncompressed 73.67 kB - -
CDN Bundle (incl. Tracing) - uncompressed 113.59 kB -0.1% -104 B 🔽
CDN Bundle (incl. Tracing, Replay) - uncompressed 227.56 kB -0.05% -104 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 240.38 kB -0.05% -104 B 🔽
@sentry/nextjs (client) 41.87 kB -0.1% -40 B 🔽
@sentry/sveltekit (client) 38.71 kB -0.11% -39 B 🔽
@sentry/node 149.42 kB - -
@sentry/node - without tracing 98.09 kB - -
@sentry/aws-serverless 123.43 kB - -

View base workflow run

@mydea mydea force-pushed the fn/fix-browserTracing-warning branch from 43e431d to f1682cf Compare May 26, 2025 10:31
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for making this change! Agree that it's better to have instrumentation setup in setup.

expect(integrations).toEqual([integration1]);
});

test('it uses last passed integration only', () => {
Copy link
Member

Choose a reason for hiding this comment

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

ah nice, I think this is good behaviour and probably better than taking first one. Just in case users have some kind of dynamic behaviour that builds the integrations array.

Copy link
Member Author

Choose a reason for hiding this comment

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

jup, totally, that was what we had already anyhow 👍 the test just shows that!

@mydea mydea merged commit 349d7d0 into develop May 26, 2025
161 checks passed
@mydea mydea deleted the fn/fix-browserTracing-warning branch May 26, 2025 11:18
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.

@sentry/nextjs: Multiple browserTracingIntegration instances are not supported.
2 participants