-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
size-limit report 📦
|
43e431d
to
f1682cf
Compare
There was a problem hiding this 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', () => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
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: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