Skip to content

ref(core): Move sentry breadcrumb logic into integration #6195

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

Merged
merged 3 commits into from
Nov 30, 2022

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Nov 12, 2022

getIntegrationById is used to ensure that the Breadcrumbs integration is not included in the bundle when it's not used.

Moving this logic from the BaseClient into the integration should save further bundle size when the Breadcrumbs integration is not used, especially since getEventDescription will no longer be included too.

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.

Can we add a test for this method? Not sure if we had one before but I think it'd be worth to have this covered.

@AbhiPrasad AbhiPrasad self-requested a review November 21, 2022 09:11
@AbhiPrasad
Copy link
Member

@timfish anything blocking from getting this in?

@timfish
Copy link
Collaborator Author

timfish commented Nov 28, 2022

Just need to add a test. Will get on this tomorrow when I'm back at my desk!

@timfish
Copy link
Collaborator Author

timfish commented Nov 30, 2022

Added a test!

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

Thanks Tim! Looks like we need to re-run linter.

@timfish timfish force-pushed the fix/move-sentry-breadcrumb-logic branch from 4dcd9a4 to 4df43bd Compare November 30, 2022 11:23
@AbhiPrasad AbhiPrasad merged commit 329cf5a into getsentry:master Nov 30, 2022
@timfish timfish deleted the fix/move-sentry-breadcrumb-logic branch November 30, 2022 12:37
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.

3 participants