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(integrations): BaseClient reports integrations added after init #7011

Merged
merged 10 commits into from Feb 2, 2023

Conversation

krystofwoldrich
Copy link
Member

In RN SDK we would like to add some integrations using the client.addIntegration method, but these integrations are not being reported in the event.sdk.integrations.

I assume that was not intentional.

Copy link
Member

@mydea mydea left a comment

Choose a reason for hiding this comment

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

Yes, def. not intentional! Thanks for finding & fixing this 🎉

@github-actions
Copy link
Contributor

github-actions bot commented Feb 1, 2023

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.91 KB (+0.18% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 61.72 KB (+0.24% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.59 KB (+0.26% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 55.02 KB (+0.26% 🔺)
@sentry/browser - Webpack (gzipped + minified) 20.32 KB (+0.14% 🔺)
@sentry/browser - Webpack (minified) 66.48 KB (+0.17% 🔺)
@sentry/react - Webpack (gzipped + minified) 20.35 KB (+0.16% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 47.64 KB (+0.08% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.82 KB (+0.1% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 25.12 KB (+0.17% 🔺)
@sentry/replay ES6 CDN Bundle (gzipped + minified) 44.21 KB (+0.12% 🔺)
@sentry/replay - Webpack (gzipped + minified) 38.93 KB (+0.12% 🔺)
@sentry/browser + @sentry/tracing + @sentry/replay - ES6 CDN Bundle (gzipped + minified) 61.57 KB (+0.12% 🔺)

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.

Good catch, thanks!

packages/core/test/mocks/integration.ts Outdated Show resolved Hide resolved
@krystofwoldrich
Copy link
Member Author

krystofwoldrich commented Feb 1, 2023

@Lms24 @mydea I've noticed a slight problem, should I use the installedIntegrations or client._integrations as when a client gets refreshed these won't match.

I'll just reset the installedIntegrations in the tests.

@krystofwoldrich
Copy link
Member Author

How do you debug these?
https://github.com/getsentry/sentry-javascript/actions/runs/4063851636/jobs/6996721341
I've run it locally and it passes, but I guess I've missed something.

@krystofwoldrich
Copy link
Member Author

Still need to wait for the CI, but should be fixed now.

It's not pretty but it works. Maybe in the future when we decide to remove installedIntegrations we can talk about adding getIntegrations to Client API?

@Lms24
Copy link
Member

Lms24 commented Feb 1, 2023

It's not pretty but it works. Maybe in the future when we decide to remove installedIntegrations we can talk about adding getIntegrations to Client API?

Yes, definitely in favour of that. However, adding methods to the client is a breaking change, so we could only do it with an optional method at this time or only do it in v8. Tracking the removal in #7028

@Lms24
Copy link
Member

Lms24 commented Feb 2, 2023

I'm gonna merge this in as we're cutting a release.

@Lms24 Lms24 merged commit e0a75f5 into develop Feb 2, 2023
@Lms24 Lms24 deleted the report-ad-hoc-integrations branch February 2, 2023 09:05
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.

None yet

3 participants