ref(core): Make on and emit required on client (#10603)

mydea · web-flow · commit cb6912a56106 · 2024-02-12T11:58:19.000+01:00
This allows us to remove a bunch of code around fallbacks etc. Esp. in replay this also allows us to remove a bunch of code around breadcrumbs etc. note @JonasBa I did not remove this in profiling-node yet because the test rely on this quite a bunch, can you take a look at this when you've got some time?
diff --git a/packages/angular/src/errorhandler.ts b/packages/angular/src/errorhandler.ts
@@ -118,7 +118,7 @@ class SentryErrorHandler implements AngularErrorHandler {
     if (this._options.showDialog) {
       const client = Sentry.getClient();
 
-      if (client && client.on && !this._registeredAfterSendEventHandler) {
+      if (client && !this._registeredAfterSendEventHandler) {
         client.on('afterSendEvent', (event: Event) => {
           if (!event.type) {
             // eslint-disable-next-line deprecation/deprecation
@@ -128,7 +128,7 @@ class SentryErrorHandler implements AngularErrorHandler {
 
         // We only want to register this hook once in the lifetime of the error handler
         this._registeredAfterSendEventHandler = true;
-      } else if (!client || !client.on) {
+      } else if (!client) {
         Sentry.showReportDialog({ ...this._options.dialogOptions, eventId });
       }
     }
diff --git a/packages/browser/src/integrations/breadcrumbs.ts b/packages/browser/src/integrations/breadcrumbs.ts
@@ -88,7 +88,7 @@ const _breadcrumbsIntegration = ((options: Partial<BreadcrumbsOptions> = {}) =>
       if (_options.history) {
         addHistoryInstrumentationHandler(_getHistoryBreadcrumbHandler(client));
       }
-      if (_options.sentry && client.on) {
+      if (_options.sentry) {
         client.on('beforeSendEvent', _getSentryBreadcrumbHandler(client));
       }
     },
diff --git a/packages/browser/src/profiling/integration.ts b/packages/browser/src/profiling/integration.ts
@@ -35,11 +35,6 @@ const _browserProfilingIntegration = (() => {
         }
       }
 
-      if (typeof client.on !== 'function') {
-        logger.warn('[Profiling] Client does not support hooks, profiling will be disabled');
-        return;
-      }
-
       client.on('startTransaction', (transaction: Transaction) => {
         if (shouldProfileTransaction(transaction)) {
           startProfileForTransaction(transaction);
diff --git a/packages/core/src/hub.ts b/packages/core/src/hub.ts
@@ -419,9 +419,7 @@ export class Hub implements HubInterface {
 
     if (finalBreadcrumb === null) return;
 
-    if (client.emit) {
-      client.emit('beforeAddBreadcrumb', finalBreadcrumb, hint);
-    }
+    client.emit('beforeAddBreadcrumb', finalBreadcrumb, hint);
 
     // TODO(v8): I know this comment doesn't make much sense because the hub will be deprecated but I still wanted to
     // write it down. In theory, we would have to add the breadcrumbs to the isolation scope here, however, that would
diff --git a/packages/core/src/integration.ts b/packages/core/src/integration.ts
@@ -140,7 +140,7 @@ export function setupIntegration(client: Client, integration: Integration, integ
     integration.setup(client);
   }
 
-  if (client.on && typeof integration.preprocessEvent === 'function') {
+  if (typeof integration.preprocessEvent === 'function') {
     const callback = integration.preprocessEvent.bind(integration) as typeof integration.preprocessEvent;
     client.on('preprocessEvent', (event, hint) => callback(event, hint, client));
   }
diff --git a/packages/core/src/integrations/metadata.ts b/packages/core/src/integrations/metadata.ts
@@ -12,10 +12,6 @@ const _moduleMetadataIntegration = (() => {
     // TODO v8: Remove this
     setupOnce() {}, // eslint-disable-line @typescript-eslint/no-empty-function
     setup(client) {
-      if (typeof client.on !== 'function') {
-        return;
-      }
-
       // We need to strip metadata from stack frames before sending them to Sentry since these are client side only.
       client.on('beforeEnvelope', envelope => {
         forEachEnvelopeItem(envelope, (item, type) => {
diff --git a/packages/core/src/tracing/dynamicSamplingContext.ts b/packages/core/src/tracing/dynamicSamplingContext.ts
@@ -23,7 +23,7 @@ export function getDynamicSamplingContextFromClient(trace_id: string, client: Cl
     trace_id,
   }) as DynamicSamplingContext;
 
-  client.emit && client.emit('createDsc', dsc);
+  client.emit('createDsc', dsc);
 
   return dsc;
 }
@@ -81,7 +81,7 @@ export function getDynamicSamplingContextFromSpan(span: Span): Readonly<Partial<
 
   dsc.sampled = String(spanIsSampled(txn));
 
-  client.emit && client.emit('createDsc', dsc);
+  client.emit('createDsc', dsc);
 
   return dsc;
 }
diff --git a/packages/core/src/tracing/hubextensions.ts b/packages/core/src/tracing/hubextensions.ts
@@ -78,7 +78,7 @@ The transaction will not be sampled. Please use the ${configInstrumenter} instru
   if (transaction.isRecording()) {
     transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number));
   }
-  if (client && client.emit) {
+  if (client) {
     client.emit('startTransaction', transaction);
   }
   return transaction;
@@ -125,7 +125,7 @@ export function startIdleTransaction(
   if (transaction.isRecording()) {
     transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number));
   }
-  if (client && client.emit) {
+  if (client) {
     client.emit('startTransaction', transaction);
   }
   return transaction;
diff --git a/packages/core/src/tracing/transaction.ts b/packages/core/src/tracing/transaction.ts
@@ -276,7 +276,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
 
     // eslint-disable-next-line deprecation/deprecation
     const client = this._hub.getClient();
-    if (client && client.emit) {
+    if (client) {
       client.emit('finishTransaction', this);
     }
 
diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts
@@ -1957,11 +1957,11 @@ describe('BaseClient', () => {
           traceId: '86f39e84263a4de99c326acab3bfe3bd',
         } as Transaction;
 
-        client.on?.('startTransaction', transaction => {
+        client.on('startTransaction', transaction => {
           expect(transaction).toEqual(mockTransaction);
         });
 
-        client.emit?.('startTransaction', mockTransaction);
+        client.emit('startTransaction', mockTransaction);
       });
 
       it('should call a beforeEnvelope hook', () => {
@@ -1974,11 +1974,11 @@ describe('BaseClient', () => {
           {},
         ] as Envelope;
 
-        client.on?.('beforeEnvelope', envelope => {
+        client.on('beforeEnvelope', envelope => {
           expect(envelope).toEqual(mockEnvelope);
         });
 
-        client.emit?.('beforeEnvelope', mockEnvelope);
+        client.emit('beforeEnvelope', mockEnvelope);
       });
     });
   });
diff --git a/packages/feedback/src/util/prepareFeedbackEvent.ts b/packages/feedback/src/util/prepareFeedbackEvent.ts
@@ -17,9 +17,7 @@ export async function prepareFeedbackEvent({
   event,
 }: PrepareFeedbackEventParams): Promise<FeedbackEvent | null> {
   const eventHint = {};
-  if (client.emit) {
-    client.emit('preprocessEvent', event, eventHint);
-  }
+  client.emit('preprocessEvent', event, eventHint);
 
   const preparedEvent = (await prepareEvent(
     client.getOptions(),
diff --git a/packages/feedback/src/util/sendFeedbackRequest.ts b/packages/feedback/src/util/sendFeedbackRequest.ts
@@ -51,9 +51,7 @@ export async function sendFeedbackRequest(
       return;
     }
 
-    if (client.emit) {
-      client.emit('beforeSendFeedback', feedbackEvent, { includeReplay: Boolean(includeReplay) });
-    }
+    client.emit('beforeSendFeedback', feedbackEvent, { includeReplay: Boolean(includeReplay) });
 
     const envelope = createEventEnvelope(feedbackEvent, dsn, client.getOptions()._metadata, client.getOptions().tunnel);
 
diff --git a/packages/integrations/src/debug.ts b/packages/integrations/src/debug.ts
@@ -23,10 +23,6 @@ const _debugIntegration = ((options: DebugOptions = {}) => {
     // TODO v8: Remove this
     setupOnce() {}, // eslint-disable-line @typescript-eslint/no-empty-function
     setup(client) {
-      if (!client.on) {
-        return;
-      }
-
       client.on('beforeSendEvent', (event: Event, hint?: EventHint) => {
         if (_options.debugger) {
           // eslint-disable-next-line no-debugger
diff --git a/packages/node-experimental/src/sdk/api.ts b/packages/node-experimental/src/sdk/api.ts
@@ -134,9 +134,7 @@ export function addBreadcrumb(breadcrumb: Breadcrumb, hint?: BreadcrumbHint): vo
 
   if (finalBreadcrumb === null) return;
 
-  if (client.emit) {
-    client.emit('beforeAddBreadcrumb', finalBreadcrumb, hint);
-  }
+  client.emit('beforeAddBreadcrumb', finalBreadcrumb, hint);
 
   getIsolationScope().addBreadcrumb(finalBreadcrumb, maxBreadcrumbs);
 }
diff --git a/packages/node/src/integrations/spotlight.ts b/packages/node/src/integrations/spotlight.ts
@@ -65,11 +65,6 @@ function connectToSpotlight(client: Client, options: Required<SpotlightConnectio
 
   let failedRequests = 0;
 
-  if (typeof client.on !== 'function') {
-    logger.warn('[Spotlight] Cannot connect to spotlight due to missing method on SDK client (`client.on`)');
-    return;
-  }
-
   client.on('beforeEnvelope', (envelope: Envelope) => {
     if (failedRequests > 3) {
       logger.warn('[Spotlight] Disabled Sentry -> Spotlight integration due to too many failed requests');
diff --git a/packages/node/test/integrations/spotlight.test.ts b/packages/node/test/integrations/spotlight.test.ts
@@ -120,16 +120,6 @@ describe('Spotlight', () => {
       integration.setup!(client);
       expect(loggerSpy).toHaveBeenCalledWith(expect.stringContaining('Invalid sidecar URL: invalid-url'));
     });
-
-    it("the client doesn't support life cycle hooks", () => {
-      const integration = spotlightIntegration({ sidecarUrl: 'http://mylocalhost:8969' });
-      const clientWithoutHooks = { ...client };
-      // @ts-expect-error - this is fine in tests
-      delete client.on;
-      // @ts-expect-error - this is fine in tests
-      integration.setup(clientWithoutHooks);
-      expect(loggerSpy).toHaveBeenCalledWith(expect.stringContaining(' missing method on SDK client (`client.on`)'));
-    });
   });
 
   it('warns if the NODE_ENV variable doesn\'t equal "development"', () => {
diff --git a/packages/opentelemetry-node/src/spanprocessor.ts b/packages/opentelemetry-node/src/spanprocessor.ts
@@ -114,7 +114,7 @@ export class SentrySpanProcessor implements OtelSpanProcessor {
     const client = getClient();
 
     const mutableOptions = { drop: false };
-    client && client.emit && client?.emit('otelSpanEnd', otelSpan, mutableOptions);
+    client && client.emit('otelSpanEnd', otelSpan, mutableOptions);
 
     if (mutableOptions.drop) {
       clearSpan(otelSpanId);
diff --git a/packages/opentelemetry-node/test/propagator.test.ts b/packages/opentelemetry-node/test/propagator.test.ts
@@ -45,6 +45,7 @@ describe('SentryPropagator', () => {
         getDsn: () => ({
           publicKey: 'abc',
         }),
+        emit: () => {},
       };
       // @ts-expect-error Use mock client for unit tests
       // eslint-disable-next-line deprecation/deprecation
diff --git a/packages/opentelemetry/src/custom/transaction.ts b/packages/opentelemetry/src/custom/transaction.ts
@@ -17,7 +17,7 @@ export function startTransaction(hub: HubInterface, transactionContext: Transact
   // Any sampling decision happens in OpenTelemetry's sampler
   transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number));
 
-  if (client && client.emit) {
+  if (client) {
     client.emit('startTransaction', transaction);
   }
   return transaction;
diff --git a/packages/opentelemetry/test/propagator.test.ts b/packages/opentelemetry/test/propagator.test.ts
@@ -39,6 +39,7 @@ describe('SentryPropagator', () => {
       getDsn: () => ({
         publicKey: 'abc',
       }),
+      emit: () => {},
     };
 
     // @ts-expect-error Use mock client for unit tests
diff --git a/packages/react/src/errorboundary.tsx b/packages/react/src/errorboundary.tsx
@@ -108,7 +108,7 @@ class ErrorBoundary extends React.Component<ErrorBoundaryProps, ErrorBoundarySta
     this._openFallbackReportDialog = true;
 
     const client = getClient();
-    if (client && client.on && props.showDialog) {
+    if (client && props.showDialog) {
       this._openFallbackReportDialog = false;
       client.on('afterSendEvent', event => {
         if (!event.type && this._lastEventId && event.event_id === this._lastEventId) {
diff --git a/packages/replay/src/coreHandlers/handleBreadcrumbs.ts b/packages/replay/src/coreHandlers/handleBreadcrumbs.ts
@@ -16,7 +16,7 @@ type BreadcrumbWithCategory = Required<Pick<Breadcrumb, 'category'>>;
 export function handleBreadcrumbs(replay: ReplayContainer): void {
   const client = getClient();
 
-  if (!client || !client.on) {
+  if (!client) {
     return;
   }
 
diff --git a/packages/replay/src/coreHandlers/handleFetch.ts b/packages/replay/src/coreHandlers/handleFetch.ts
diff --git a/packages/replay/src/coreHandlers/handleGlobalEvent.ts b/packages/replay/src/coreHandlers/handleGlobalEvent.ts
@@ -5,19 +5,13 @@ import { DEBUG_BUILD } from '../debug-build';
 import type { ReplayContainer } from '../types';
 import { isErrorEvent, isFeedbackEvent, isReplayEvent, isTransactionEvent } from '../util/eventUtils';
 import { isRrwebError } from '../util/isRrwebError';
-import { handleAfterSendEvent } from './handleAfterSendEvent';
 import { addFeedbackBreadcrumb } from './util/addFeedbackBreadcrumb';
 import { shouldSampleForBufferEvent } from './util/shouldSampleForBufferEvent';
 
 /**
  * Returns a listener to be added to `addEventProcessor(listener)`.
  */
-export function handleGlobalEventListener(
-  replay: ReplayContainer,
-  includeAfterSendEventHandling = false,
-): (event: Event, hint: EventHint) => Event | null {
-  const afterSendHandler = includeAfterSendEventHandling ? handleAfterSendEvent(replay) : undefined;
-
+export function handleGlobalEventListener(replay: ReplayContainer): (event: Event, hint: EventHint) => Event | null {
   return Object.assign(
     (event: Event, hint: EventHint) => {
       // Do nothing if replay has been disabled
@@ -73,13 +67,6 @@ export function handleGlobalEventListener(
         event.tags = { ...event.tags, replayId: replay.getSessionId() };
       }
 
-      // In cases where a custom client is used that does not support the new hooks (yet),
-      // we manually call this hook method here
-      if (afterSendHandler) {
-        // Pretend the error had a 200 response so we always capture it
-        afterSendHandler(event, { statusCode: 200 });
-      }
-
       return event;
     },
     { id: 'Replay' },
diff --git a/packages/replay/src/coreHandlers/handleNetworkBreadcrumbs.ts b/packages/replay/src/coreHandlers/handleNetworkBreadcrumbs.ts
@@ -6,12 +6,10 @@ import type {
   TextEncoderInternal,
   XhrBreadcrumbData,
 } from '@sentry/types';
-import { addFetchInstrumentationHandler, addXhrInstrumentationHandler, logger } from '@sentry/utils';
+import { logger } from '@sentry/utils';
 
 import { DEBUG_BUILD } from '../debug-build';
 import type { FetchHint, ReplayContainer, ReplayNetworkOptions, XhrHint } from '../types';
-import { handleFetchSpanListener } from './handleFetch';
-import { handleXhrSpanListener } from './handleXhr';
 import { captureFetchBreadcrumbToReplay, enrichFetchBreadcrumb } from './util/fetchUtils';
 import { captureXhrBreadcrumbToReplay, enrichXhrBreadcrumb } from './util/xhrUtils';
 
@@ -50,12 +48,8 @@ export function handleNetworkBreadcrumbs(replay: ReplayContainer): void {
       networkResponseHeaders,
     };
 
-    if (client && client.on) {
+    if (client) {
       client.on('beforeAddBreadcrumb', (breadcrumb, hint) => beforeAddNetworkBreadcrumb(options, breadcrumb, hint));
-    } else {
-      // Fallback behavior
-      addFetchInstrumentationHandler(handleFetchSpanListener(replay));
-      addXhrInstrumentationHandler(handleXhrSpanListener(replay));
     }
   } catch {
     // Do nothing
diff --git a/packages/replay/src/coreHandlers/handleXhr.ts b/packages/replay/src/coreHandlers/handleXhr.ts
diff --git a/packages/replay/src/util/addGlobalListeners.ts b/packages/replay/src/util/addGlobalListeners.ts
diff --git a/packages/replay/src/util/prepareReplayEvent.ts b/packages/replay/src/util/prepareReplayEvent.ts
diff --git a/packages/replay/test/integration/coreHandlers/handleGlobalEvent.test.ts b/packages/replay/test/integration/coreHandlers/handleGlobalEvent.test.ts
diff --git a/packages/replay/test/unit/coreHandlers/handleFetch.test.ts b/packages/replay/test/unit/coreHandlers/handleFetch.test.ts
diff --git a/packages/replay/test/unit/coreHandlers/handleXhr.test.ts b/packages/replay/test/unit/coreHandlers/handleXhr.test.ts
diff --git a/packages/sveltekit/test/client/browserTracingIntegration.test.ts b/packages/sveltekit/test/client/browserTracingIntegration.test.ts
diff --git a/packages/tracing-internal/src/browser/browserTracingIntegration.ts b/packages/tracing-internal/src/browser/browserTracingIntegration.ts
diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts

Original file line number	Diff line number	Diff line change
`@@ -88,7 +88,7 @@ const _breadcrumbsIntegration = ((options: Partial<BreadcrumbsOptions> = {}) =>`
`88`	`88`	`if (_options.history) {`
`89`	`89`	`addHistoryInstrumentationHandler(_getHistoryBreadcrumbHandler(client));`
`90`	`90`	`}`
`91`		`- if (_options.sentry && client.on) {`
	`91`	`+ if (_options.sentry) {`
`92`	`92`	`client.on('beforeSendEvent', _getSentryBreadcrumbHandler(client));`
`93`	`93`	`}`
`94`	`94`	`},`
Original file line number	Diff line number	Diff line change
`@@ -35,11 +35,6 @@ const _browserProfilingIntegration = (() => {`
`35`	`35`	`}`
`36`	`36`	`}`
`37`	`37`
`38`		`- if (typeof client.on !== 'function') {`
`39`		`- logger.warn('[Profiling] Client does not support hooks, profiling will be disabled');`
`40`		`- return;`
`41`		`- }`
`42`		`-`
`43`	`38`	`client.on('startTransaction', (transaction: Transaction) => {`
`44`	`39`	`if (shouldProfileTransaction(transaction)) {`
`45`	`40`	`startProfileForTransaction(transaction);`
Original file line number	Diff line number	Diff line change
`@@ -140,7 +140,7 @@ export function setupIntegration(client: Client, integration: Integration, integ`
`140`	`140`	`integration.setup(client);`
`141`	`141`	`}`
`142`	`142`
`143`		`- if (client.on && typeof integration.preprocessEvent === 'function') {`
	`143`	`+ if (typeof integration.preprocessEvent === 'function') {`
`144`	`144`	`const callback = integration.preprocessEvent.bind(integration) as typeof integration.preprocessEvent;`
`145`	`145`	`client.on('preprocessEvent', (event, hint) => callback(event, hint, client));`
`146`	`146`	`}`
Original file line number	Diff line number	Diff line change
`@@ -23,7 +23,7 @@ export function getDynamicSamplingContextFromClient(trace_id: string, client: Cl`
`23`	`23`	`trace_id,`
`24`	`24`	`}) as DynamicSamplingContext;`
`25`	`25`
`26`		`- client.emit && client.emit('createDsc', dsc);`
	`26`	`+ client.emit('createDsc', dsc);`
`27`	`27`
`28`	`28`	`return dsc;`
`29`	`29`	`}`
`@@ -81,7 +81,7 @@ export function getDynamicSamplingContextFromSpan(span: Span): Readonly<Partial<`
`81`	`81`
`82`	`82`	`dsc.sampled = String(spanIsSampled(txn));`
`83`	`83`
`84`		`- client.emit && client.emit('createDsc', dsc);`
	`84`	`+ client.emit('createDsc', dsc);`
`85`	`85`
`86`	`86`	`return dsc;`
`87`	`87`	`}`
Original file line number	Diff line number	Diff line change
`@@ -78,7 +78,7 @@ The transaction will not be sampled. Please use the ${configInstrumenter} instru`
`78`	`78`	`if (transaction.isRecording()) {`
`79`	`79`	`transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number));`
`80`	`80`	`}`
`81`		`- if (client && client.emit) {`
	`81`	`+ if (client) {`
`82`	`82`	`client.emit('startTransaction', transaction);`
`83`	`83`	`}`
`84`	`84`	`return transaction;`
`@@ -125,7 +125,7 @@ export function startIdleTransaction(`
`125`	`125`	`if (transaction.isRecording()) {`
`126`	`126`	`transaction.initSpanRecorder(options._experiments && (options._experiments.maxSpans as number));`
`127`	`127`	`}`
`128`		`- if (client && client.emit) {`
	`128`	`+ if (client) {`
`129`	`129`	`client.emit('startTransaction', transaction);`
`130`	`130`	`}`
`131`	`131`	`return transaction;`
Original file line number	Diff line number	Diff line change
`@@ -276,7 +276,7 @@ export class Transaction extends SpanClass implements TransactionInterface {`
`276`	`276`
`277`	`277`	`// eslint-disable-next-line deprecation/deprecation`
`278`	`278`	`const client = this._hub.getClient();`
`279`		`- if (client && client.emit) {`
	`279`	`+ if (client) {`
`280`	`280`	`client.emit('finishTransaction', this);`
`281`	`281`	`}`
`282`	`282`