feat(v8/core): remove void from transport return (#10794)

Use `Promise<TransportMakeRequestResponse>` instead of `Promise<void |
TransportMakeRequestResponse>`. As a result, remove
`__sentry__baseTransport__` field from transports.

Author: AbhiPrasad

MIGRATION.md

@@ -272,6 +272,7 @@ Removed top-level exports: `tracingOrigins`, `MetricsAggregator`, `metricsAggreg
 - [Removal of `spanStatusfromHttpCode` in favour of `getSpanStatusFromHttpCode`](./MIGRATION.md#removal-of-spanstatusfromhttpcode-in-favour-of-getspanstatusfromhttpcode)
 - [Removal of `addGlobalEventProcessor` in favour of `addEventProcessor`](./MIGRATION.md#removal-of-addglobaleventprocessor-in-favour-of-addeventprocessor)
 - [Removal of `lastEventId()` method](./MIGRATION.md#deprecate-lasteventid)
+- [Remove `void` from transport return types](./MIGRATION.md#remove-void-from-transport-return-types)
 
 #### Deprecation of `Hub` and `getCurrentHub()`
 
@@ -435,6 +436,23 @@ addEventProcessor(event => {
 
 The `lastEventId` function has been removed. See [below](./MIGRATION.md#deprecate-lasteventid) for more details.
 
+#### Remove `void` from transport return types
+
+The `send` method on the `Transport` interface now always requires a `TransportMakeRequestResponse` to be returned in
+the promise. This means that the `void` return type is no longer allowed.
+
+```ts
+// Before (v7)
+interface Transport {
+  send(event: Event): Promise<void | TransportMakeRequestResponse>;
+}
+
+// After (v8)
+interface Transport {
+  send(event: Event): Promise<TransportMakeRequestResponse>;
+}
+```
+
 ### Browser SDK (Browser, React, Vue, Angular, Ember, etc.)
 
 Removed top-level exports: `Offline`, `makeXHRTransport`, `BrowserTracing`

dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/init.js

@@ -7,7 +7,7 @@ import type { Express } from 'express';
  */
 export function loggingTransport(_options: BaseTransportOptions): Transport {
   return {
-    send(request: Envelope): Promise<void | TransportMakeRequestResponse> {
+    send(request: Envelope): Promise<TransportMakeRequestResponse> {
       // eslint-disable-next-line no-console
       console.log(JSON.stringify(request));
       return Promise.resolve({ statusCode: 200 });

dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/test.ts

@@ -432,10 +432,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   public on(hook: 'preprocessEvent', callback: (event: Event, hint?: EventHint) => void): void;
 
   /** @inheritdoc */
-  public on(
-    hook: 'afterSendEvent',
-    callback: (event: Event, sendResponse: TransportMakeRequestResponse | void) => void,
-  ): void;
+  public on(hook: 'afterSendEvent', callback: (event: Event, sendResponse: TransportMakeRequestResponse) => void): void;
 
   /** @inheritdoc */
   public on(hook: 'beforeAddBreadcrumb', callback: (breadcrumb: Breadcrumb, hint?: BreadcrumbHint) => void): void;
@@ -485,7 +482,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   public emit(hook: 'preprocessEvent', event: Event, hint?: EventHint): void;
 
   /** @inheritdoc */
-  public emit(hook: 'afterSendEvent', event: Event, sendResponse: TransportMakeRequestResponse | void): void;
+  public emit(hook: 'afterSendEvent', event: Event, sendResponse: TransportMakeRequestResponse): void;
 
   /** @inheritdoc */
   public emit(hook: 'beforeAddBreadcrumb', breadcrumb: Breadcrumb, hint?: BreadcrumbHint): void;
@@ -518,16 +515,17 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   /**
    * @inheritdoc
    */
-  public sendEnvelope(envelope: Envelope): PromiseLike<void | TransportMakeRequestResponse> | void {
+  public sendEnvelope(envelope: Envelope): PromiseLike<TransportMakeRequestResponse> | void {
     this.emit('beforeEnvelope', envelope);
 
     if (this._isEnabled() && this._transport) {
       return this._transport.send(envelope).then(null, reason => {
         DEBUG_BUILD && logger.error('Error while sending event:', reason);
+        return reason;
       });
-    } else {
-      DEBUG_BUILD && logger.error('Transport disabled');
     }
+
+    DEBUG_BUILD && logger.error('Transport disabled');
   }
 
   /* eslint-enable @typescript-eslint/unified-signatures */

dev-packages/node-integration-tests/src/index.ts

@@ -37,14 +37,14 @@ export const DEFAULT_TRANSPORT_BUFFER_SIZE = 30;
 export function createTransport(
   options: InternalBaseTransportOptions,
   makeRequest: TransportRequestExecutor,
-  buffer: PromiseBuffer<void | TransportMakeRequestResponse> = makePromiseBuffer(
+  buffer: PromiseBuffer<TransportMakeRequestResponse> = makePromiseBuffer(
     options.bufferSize || DEFAULT_TRANSPORT_BUFFER_SIZE,
   ),
 ): Transport {
   let rateLimits: RateLimits = {};
   const flush = (timeout?: number): PromiseLike<boolean> => buffer.drain(timeout);
 
-  function send(envelope: Envelope): PromiseLike<void | TransportMakeRequestResponse> {
+  function send(envelope: Envelope): PromiseLike<TransportMakeRequestResponse> {
     const filteredEnvelopeItems: EnvelopeItem[] = [];
 
     // Drop rate limited items from envelope
@@ -60,7 +60,7 @@ export function createTransport(
 
     // Skip sending if envelope is empty after filtering out rate limited events
     if (filteredEnvelopeItems.length === 0) {
-      return resolvedSyncPromise();
+      return resolvedSyncPromise({});
     }
 
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -74,7 +74,7 @@ export function createTransport(
       });
     };
 
-    const requestTask = (): PromiseLike<void | TransportMakeRequestResponse> =>
+    const requestTask = (): PromiseLike<TransportMakeRequestResponse> =>
       makeRequest({ body: serializeEnvelope(filteredEnvelope) }).then(
         response => {
           // We don't want to throw on NOK responses, but we want to at least log them
@@ -97,18 +97,14 @@ export function createTransport(
         if (error instanceof SentryError) {
           DEBUG_BUILD && logger.error('Skipped sending event because buffer is full.');
           recordEnvelopeLoss('queue_overflow');
-          return resolvedSyncPromise();
+          return resolvedSyncPromise({});
         } else {
           throw error;
         }
       },
     );
   }
 
-  // We use this to identifify if the transport is the base transport
-  // TODO (v8): Remove this again as we'll no longer need it
-  send.__sentry__baseTransport__ = true;
-
   return {
     send,
     flush,

packages/core/src/baseclient.ts

@@ -57,7 +57,7 @@ function makeOverrideReleaseTransport<TO extends BaseTransportOptions>(
     const transport = createTransport(options);
 
     return {
-      send: async (envelope: Envelope): Promise<void | TransportMakeRequestResponse> => {
+      send: async (envelope: Envelope): Promise<TransportMakeRequestResponse> => {
         const event = eventFromEnvelope(envelope, ['event', 'transaction', 'profile', 'replay_event']);
 
         if (event) {
@@ -101,7 +101,7 @@ export function makeMultiplexedTransport<TO extends BaseTransportOptions>(
       return otherTransports[key];
     }
 
-    async function send(envelope: Envelope): Promise<void | TransportMakeRequestResponse> {
+    async function send(envelope: Envelope): Promise<TransportMakeRequestResponse> {
       function getEvent(types?: EnvelopeItemType[]): Event | undefined {
         const eventTypes: EnvelopeItemType[] = types && types.length ? types : ['event'];
         return eventFromEnvelope(envelope, eventTypes);

packages/core/src/transports/base.ts

@@ -113,7 +113,7 @@ export function makeOfflineTransport<TO>(
       retryDelay = Math.min(retryDelay * 2, MAX_DELAY);
     }
 
-    async function send(envelope: Envelope): Promise<void | TransportMakeRequestResponse> {
+    async function send(envelope: Envelope): Promise<TransportMakeRequestResponse> {
       try {
         const result = await transport.send(envelope);
 

packages/core/src/transports/multiplexed.ts

@@ -1804,7 +1804,7 @@ describe('BaseClient', () => {
 
       expect(mockSend).toBeCalledTimes(1);
       expect(callback).toBeCalledTimes(1);
-      expect(callback).toBeCalledWith(errorEvent, undefined);
+      expect(callback).toBeCalledWith(errorEvent, 'send error');
     });
 
     it('passes the response to the hook', async () => {

packages/core/src/transports/offline.ts

@@ -35,7 +35,7 @@ const transportOptions = {
 
 describe('createTransport', () => {
   it('flushes the buffer', async () => {
-    const mockBuffer: PromiseBuffer<void> = {
+    const mockBuffer: PromiseBuffer<TransportMakeRequestResponse> = {
       $: [],
       add: jest.fn(),
       drain: jest.fn(),

packages/core/test/lib/base.test.ts

@@ -1,5 +1,5 @@
 import type { TransportMakeRequestResponse } from '@sentry/types';
-import { logger } from '@sentry/utils';
+import { logger, resolvedSyncPromise } from '@sentry/utils';
 
 import { FEEDBACK_WIDGET_SOURCE } from '../constants';
 import { DEBUG_BUILD } from '../debug-build';
@@ -15,10 +15,10 @@ export async function handleFeedbackSubmit(
   dialog: DialogComponent | null,
   feedback: FeedbackFormData,
   options?: SendFeedbackOptions,
-): Promise<TransportMakeRequestResponse | void> {
+): Promise<TransportMakeRequestResponse> {
   if (!dialog) {
     // Not sure when this would happen
-    return;
+    return resolvedSyncPromise({});
   }
 
   const showFetchError = (): void => {
@@ -39,4 +39,6 @@ export async function handleFeedbackSubmit(
     DEBUG_BUILD && logger.error(err);
     showFetchError();
   }
+
+  return resolvedSyncPromise({});
 }

packages/core/test/lib/transports/base.test.ts

@@ -1,5 +1,6 @@
 import { createEventEnvelope, getClient, withScope } from '@sentry/core';
 import type { FeedbackEvent, TransportMakeRequestResponse } from '@sentry/types';
+import { resolvedSyncPromise } from '@sentry/utils';
 
 import { FEEDBACK_API_SOURCE, FEEDBACK_WIDGET_SOURCE } from '../constants';
 import type { SendFeedbackData, SendFeedbackOptions } from '../types';
@@ -11,13 +12,13 @@ import { prepareFeedbackEvent } from './prepareFeedbackEvent';
 export async function sendFeedbackRequest(
   { feedback: { message, email, name, source, url } }: SendFeedbackData,
   { includeReplay = true }: SendFeedbackOptions = {},
-): Promise<void | TransportMakeRequestResponse> {
+): Promise<TransportMakeRequestResponse> {
   const client = getClient();
   const transport = client && client.getTransport();
   const dsn = client && client.getDsn();
 
   if (!client || !transport || !dsn) {
-    return;
+    return resolvedSyncPromise({});
   }
 
   const baseEvent: FeedbackEvent = {
@@ -48,14 +49,14 @@ export async function sendFeedbackRequest(
     });
 
     if (!feedbackEvent) {
-      return;
+      return resolvedSyncPromise({});
     }
 
     client.emit('beforeSendFeedback', feedbackEvent, { includeReplay: Boolean(includeReplay) });
 
     const envelope = createEventEnvelope(feedbackEvent, dsn, client.getOptions()._metadata, client.getOptions().tunnel);
 
-    let response: void | TransportMakeRequestResponse;
+    let response: TransportMakeRequestResponse;
 
     try {
       response = await transport.send(envelope);
@@ -72,11 +73,6 @@ export async function sendFeedbackRequest(
       throw error;
     }
 
-    // TODO (v8): we can remove this guard once transport.send's type signature doesn't include void anymore
-    if (!response) {
-      return;
-    }
-
     // Require valid status codes, otherwise can assume feedback was not sent successfully
     if (typeof response.statusCode === 'number' && (response.statusCode < 200 || response.statusCode >= 300)) {
       throw new Error('Unable to send Feedback');

packages/feedback/src/util/handleFeedbackSubmit.ts

@@ -109,7 +109,7 @@ export function createWidget({
     const result = await handleFeedbackSubmit(dialog, feedback);
 
     // Error submitting feedback
-    if (!result) {
+    if (!result || Object.keys(result).length === 0) {
       if (options.onSubmitError) {
         options.onSubmitError();
       }

packages/feedback/src/util/sendFeedbackRequest.ts

@@ -11,16 +11,11 @@ class MockTransport implements Transport {
   send: (request: Envelope) => PromiseLike<TransportMakeRequestResponse>;
 
   constructor() {
-    const send: ((request: Envelope) => PromiseLike<TransportMakeRequestResponse>) & {
-      __sentry__baseTransport__?: boolean;
-    } = jest.fn(async () => {
+    this.send = jest.fn(async () => {
       return {
         statusCode: 200,
       };
     });
-
-    send.__sentry__baseTransport__ = true;
-    this.send = send;
   }
 
   async flush() {

packages/feedback/src/widget/createWidget.ts

@@ -130,7 +130,7 @@ describe('createWidget', () => {
     });
 
     (sendFeedbackRequest as jest.Mock).mockImplementation(() => {
-      return Promise.resolve(true);
+      return Promise.resolve({ statusCode: 200 });
     });
     widget.actor?.el?.dispatchEvent(new Event('click'));
 
@@ -180,7 +180,7 @@ describe('createWidget', () => {
     });
 
     (sendFeedbackRequest as jest.Mock).mockImplementation(() => {
-      return true;
+      return Promise.resolve({ statusCode: 200 });
     });
     widget.actor?.el?.dispatchEvent(new Event('click'));