-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: Always assemble Envelopes #9101
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 📦
|
@@ -297,8 +292,8 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> { | |||
/** | |||
* Sets up the integrations | |||
*/ | |||
public setupIntegrations(): void { | |||
if (this._isEnabled() && !this._integrationsInitialized) { | |||
public setupIntegrations(forceInitialize?: boolean): void { |
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.
l: Maybe it would be nicer to split this into to methods, than to have this argument:
public setupIntegrations(): void {
if (this._isEnabled() && !this._integrationsInitialized) {
this.initializeIntegrations(); // <-- naming TBD??
}
}
public initializeIntegrations(): void {
this._integrations = setupIntegrations(this, this._options.integrations);
this._integrationsInitialized = true;
}
I guess the idea is that e.g. spotlight would initialize sentry integrations even if not enabled? So that could then just call client.initializeIntegration()
?
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.
will change it, like your proposal
actually, this might be worth doing when we rethink how integrations work
the problem is, I still want to make sure that calling this more than once doesn't setup integrations more than once
with this change, it's not guaranteed that an integration will only be called once
This PR changes a few things:
if no transport is defined, the client does nothing. The default behavior is no DSN = no transport, but if someone manually adds a transport that could do anything with the events, we shouldn't fully disable the SDK.
see https://github.com/getsentry/sentry-javascript/pull/9101/files#diff-9b99586572e171cbe9f43e400ce8c631dfc032990c52e95632fc488dde6b1cf4L536
forceInitialize
optional boolean tosetupIntegrations
to be able to initialize global integrations externally.Note: Integrations aren't initialized by default so the overhead added with this change, eventho we assemble Envelopes now, should be minimal/non-existent if someone disabled the SDK.
This is to enable Spotlight to work:
https://github.com/getsentry/spotlight/pull/3/files#diff-0b5adbfe7b36e4ae2f479291e20152e33e940f7f265162d77f40f6bdb5da7405R75