-
-
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? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # 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