Skip to content

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

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

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

Conversation

@krystofwoldrich
Copy link
Copy Markdown
Contributor

@krystofwoldrich krystofwoldrich commented Feb 1, 2023

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.

mydea
mydea approved these changes Feb 1, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

github-actions bot commented Feb 1, 2023 •
edited
Loading

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

Lms24
Lms24 approved these changes Feb 1, 2023
View reviewed changes
Copy link
Copy Markdown
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!

@krystofwoldrich
Copy link
Copy Markdown
Contributor Author

krystofwoldrich commented Feb 1, 2023 •
edited
Loading

@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 krystofwoldrich mentioned this pull request Feb 1, 2023
Merged
11 tasks
@krystofwoldrich
Copy link
Copy Markdown
Contributor Author

krystofwoldrich commented Feb 1, 2023

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.

@Lms24 Lms24 mentioned this pull request Feb 1, 2023
Closed
@krystofwoldrich
Copy link
Copy Markdown
Contributor Author

krystofwoldrich commented Feb 1, 2023

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?

mydea
mydea reviewed Feb 1, 2023
View reviewed changes
mydea
mydea reviewed Feb 1, 2023
View reviewed changes
@krystofwoldrich krystofwoldrich requested a review from mydea February 1, 2023 16:15
@Lms24
Copy link
Copy Markdown
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

mydea
mydea reviewed Feb 1, 2023
View reviewed changes
@Lms24
Copy link
Copy Markdown
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

Reviewers

@mydea mydea mydea approved these changes

@Lms24 Lms24 Lms24 approved these changes

@AbhiPrasad AbhiPrasad Awaiting requested review from AbhiPrasad

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@krystofwoldrich @Lms24 @mydea