-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
ref(browser): Don't use Breadcrumbs integration in send event flow #5024
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
Conversation
size-limit report 📦
|
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.
Nice work! LGTM (seems work similarly to what we discussed a few days ago).
* | ||
* @returns The installed integration or `undefined` if no integration with that `id` was installed. | ||
*/ | ||
public getIntegrationById(integrationId: string): Integration | undefined { |
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.
What do you think about making this should be generic over an Integration? This means the caller can define the value without needing a cast.
public getIntegrationById(integrationId: string): Integration | undefined { | |
public getIntegrationById<I extends Integration>(integrationId: string): I | undefined { |
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.
Honestly, I do not like it. I do not want to help people to disable TS checks. If you are sure that your code is right go ahead and cast it, because that is way more explicitly saying that you're doing some dangerous stuff than using a generic, which kind of hides the danger...
* Options of the breadcrumbs integration. | ||
*/ | ||
// This field is public, because we use it in the browser client to check if the `sentry` option is enabled. | ||
public readonly options: Readonly<BreadcrumbsOptions>; |
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.
This now means that options
cannot be minified - but I guess not the end of the world.
packages/browser/src/client.ts
Outdated
if (integration) { | ||
integration.addSentryBreadcrumb(event); | ||
// We only want to add the sentry event breadcrumb when the user has the breadcrumb integration installed and | ||
// activated its`sentry` option. |
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.
// activated its`sentry` option. | |
// activated it's `sentry` option. |
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.
while I do now notice that there's a space missing I think this should actually be "its" because "it is" would definitely not fit here
Avoids directly using the
Breadcrumbs
integration class in non-integration code to reduce bundle size if manually creating a client instead of usingSentry.init()
.Ref: https://getsentry.atlassian.net/browse/WEB-820