Skip to content

fix(integrations): BaseClient reports integrations added after init - #7011

Merged
Lms24 merged 10 commits into
developfrom
report-ad-hoc-integrations
Feb 2, 2023
Merged

fix(integrations): BaseClient reports integrations added after init#7011
Lms24 merged 10 commits into
developfrom
report-ad-hoc-integrations

Conversation

@krystofwoldrich

Copy link
Copy Markdown
Contributor

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.

@mydeamydea left a comment

Copy link
Copy Markdown
Member

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

github-actionsBot commented Feb 1, 2023

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@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% 🔺)

@Lms24Lms24 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch, thanks!

Comment threadpackages/core/test/mocks/integration.ts Outdated
@krystofwoldrich

krystofwoldrich commented Feb 1, 2023

Copy link
Copy Markdown
ContributorAuthor

@Lms24@mydeaI'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
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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?

Comment threadpackages/core/src/baseclient.ts Outdated
Comment threadpackages/core/src/utils/prepareEvent.ts
@Lms24

Lms24 commented Feb 1, 2023

Copy link
Copy Markdown
Member

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

Comment threadrollup/plugins/bundlePlugins.js
@Lms24

Lms24 commented Feb 2, 2023

Copy link
Copy Markdown
Member

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

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

3 participants

@krystofwoldrich@Lms24@mydea