From 5407df5aa3bcd1ebc16e1dc6c70d6c4fe64b0ca5 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 22 Feb 2024 10:48:57 -0500 Subject: [PATCH 1/9] feat(v8/core): remove void from transport return Use Promise instead of Promise --- dev-packages/node-integration-tests/src/index.ts | 2 +- packages/core/src/baseclient.ts | 7 ++++--- packages/core/src/transports/base.ts | 10 +++++----- packages/core/src/transports/multiplexed.ts | 4 ++-- packages/core/src/transports/offline.ts | 2 +- packages/feedback/src/util/sendFeedbackRequest.ts | 14 +++++--------- packages/replay/src/util/sendReplayRequest.ts | 14 +++++--------- packages/types/src/transport.ts | 3 +-- 8 files changed, 24 insertions(+), 32 deletions(-) diff --git a/dev-packages/node-integration-tests/src/index.ts b/dev-packages/node-integration-tests/src/index.ts index aba4e76caa7e..46c6e98401ae 100644 --- a/dev-packages/node-integration-tests/src/index.ts +++ b/dev-packages/node-integration-tests/src/index.ts @@ -7,7 +7,7 @@ import type { Express } from 'express'; */ export function loggingTransport(_options: BaseTransportOptions): Transport { return { - send(request: Envelope): Promise { + send(request: Envelope): Promise { // eslint-disable-next-line no-console console.log(JSON.stringify(request)); return Promise.resolve({ statusCode: 200 }); diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index 0f0e92dacd4d..f1b1685b12ce 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -518,16 +518,17 @@ export abstract class BaseClient implements Client { /** * @inheritdoc */ - public sendEnvelope(envelope: Envelope): PromiseLike | void { + public sendEnvelope(envelope: Envelope): PromiseLike | 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 {}; }); - } else { - DEBUG_BUILD && logger.error('Transport disabled'); } + + DEBUG_BUILD && logger.error('Transport disabled'); } /* eslint-enable @typescript-eslint/unified-signatures */ diff --git a/packages/core/src/transports/base.ts b/packages/core/src/transports/base.ts index 2121ea6751f7..2fbeb27e0ed5 100644 --- a/packages/core/src/transports/base.ts +++ b/packages/core/src/transports/base.ts @@ -37,14 +37,14 @@ export const DEFAULT_TRANSPORT_BUFFER_SIZE = 30; export function createTransport( options: InternalBaseTransportOptions, makeRequest: TransportRequestExecutor, - buffer: PromiseBuffer = makePromiseBuffer( + buffer: PromiseBuffer = makePromiseBuffer( options.bufferSize || DEFAULT_TRANSPORT_BUFFER_SIZE, ), ): Transport { let rateLimits: RateLimits = {}; const flush = (timeout?: number): PromiseLike => buffer.drain(timeout); - function send(envelope: Envelope): PromiseLike { + function send(envelope: Envelope): PromiseLike { 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 => + const requestTask = (): PromiseLike => makeRequest({ body: serializeEnvelope(filteredEnvelope) }).then( response => { // We don't want to throw on NOK responses, but we want to at least log them @@ -97,7 +97,7 @@ 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; } diff --git a/packages/core/src/transports/multiplexed.ts b/packages/core/src/transports/multiplexed.ts index a496a8adcd6f..faeb5a0fbd81 100644 --- a/packages/core/src/transports/multiplexed.ts +++ b/packages/core/src/transports/multiplexed.ts @@ -57,7 +57,7 @@ function makeOverrideReleaseTransport( const transport = createTransport(options); return { - send: async (envelope: Envelope): Promise => { + send: async (envelope: Envelope): Promise => { const event = eventFromEnvelope(envelope, ['event', 'transaction', 'profile', 'replay_event']); if (event) { @@ -101,7 +101,7 @@ export function makeMultiplexedTransport( return otherTransports[key]; } - async function send(envelope: Envelope): Promise { + async function send(envelope: Envelope): Promise { function getEvent(types?: EnvelopeItemType[]): Event | undefined { const eventTypes: EnvelopeItemType[] = types && types.length ? types : ['event']; return eventFromEnvelope(envelope, eventTypes); diff --git a/packages/core/src/transports/offline.ts b/packages/core/src/transports/offline.ts index c628e538a071..2e0db450ddfd 100644 --- a/packages/core/src/transports/offline.ts +++ b/packages/core/src/transports/offline.ts @@ -113,7 +113,7 @@ export function makeOfflineTransport( retryDelay = Math.min(retryDelay * 2, MAX_DELAY); } - async function send(envelope: Envelope): Promise { + async function send(envelope: Envelope): Promise { try { const result = await transport.send(envelope); diff --git a/packages/feedback/src/util/sendFeedbackRequest.ts b/packages/feedback/src/util/sendFeedbackRequest.ts index d8f3a880ba2c..341611d94085 100644 --- a/packages/feedback/src/util/sendFeedbackRequest.ts +++ b/packages/feedback/src/util/sendFeedbackRequest.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 { +): Promise { 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'); diff --git a/packages/replay/src/util/sendReplayRequest.ts b/packages/replay/src/util/sendReplayRequest.ts index c030ea1f8c2f..03945bb479af 100644 --- a/packages/replay/src/util/sendReplayRequest.ts +++ b/packages/replay/src/util/sendReplayRequest.ts @@ -1,6 +1,7 @@ import { getClient, getCurrentScope } from '@sentry/core'; import type { ReplayEvent, TransportMakeRequestResponse } from '@sentry/types'; import type { RateLimits } from '@sentry/utils'; +import { resolvedSyncPromise } from '@sentry/utils'; import { isRateLimited, updateRateLimits } from '@sentry/utils'; import { REPLAY_EVENT_NAME, UNABLE_TO_SEND_REPLAY } from '../constants'; @@ -20,7 +21,7 @@ export async function sendReplayRequest({ eventContext, timestamp, session, -}: SendReplayData): Promise { +}: SendReplayData): Promise { const preparedRecordingData = prepareRecordingData({ recordingData, headers: { @@ -36,7 +37,7 @@ export async function sendReplayRequest({ const dsn = client && client.getDsn(); if (!client || !transport || !dsn || !session.sampled) { - return; + return resolvedSyncPromise({}); } const baseEvent: ReplayEvent = { @@ -57,7 +58,7 @@ export async function sendReplayRequest({ // Taken from baseclient's `_processEvent` method, where this is handled for errors/transactions client.recordDroppedEvent('event_processor', 'replay', baseEvent); logInfo('An event processor returned `null`, will not send event.'); - return; + return resolvedSyncPromise({}); } /* @@ -102,7 +103,7 @@ export async function sendReplayRequest({ const envelope = createReplayEnvelope(replayEvent, preparedRecordingData, dsn, client.getOptions().tunnel); - let response: void | TransportMakeRequestResponse; + let response: TransportMakeRequestResponse; try { response = await transport.send(envelope); @@ -119,11 +120,6 @@ export async function sendReplayRequest({ throw error; } - // TODO (v8): we can remove this guard once transport.send's type signature doesn't include void anymore - if (!response) { - return response; - } - // If the status code is invalid, we want to immediately stop & not retry if (typeof response.statusCode === 'number' && (response.statusCode < 200 || response.statusCode >= 300)) { throw new TransportStatusCodeError(response.statusCode); diff --git a/packages/types/src/transport.ts b/packages/types/src/transport.ts index 6866f8d19032..07186b141420 100644 --- a/packages/types/src/transport.ts +++ b/packages/types/src/transport.ts @@ -27,8 +27,7 @@ export interface BaseTransportOptions extends InternalBaseTransportOptions { } export interface Transport { - // TODO (v8) Remove void from return as it was only retained to avoid a breaking change - send(request: Envelope): PromiseLike; + send(request: Envelope): PromiseLike; flush(timeout?: number): PromiseLike; } From c33e655ec6e5f8b3f028e6ff8b706f63580fd81b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 22 Feb 2024 10:56:18 -0500 Subject: [PATCH 2/9] remove more instances --- .../errors/errorModeCustomTransport/init.js | 6 +--- packages/core/src/baseclient.ts | 7 ++--- packages/core/src/transports/base.ts | 4 --- .../feedback/src/util/handleFeedbackSubmit.ts | 8 +++-- packages/feedback/test/utils/mockSdk.ts | 7 +---- .../src/coreHandlers/handleAfterSendEvent.ts | 29 +++---------------- .../coreHandlers/handleAfterSendEvent.test.ts | 4 --- packages/replay/test/mocks/mockSdk.ts | 7 +---- packages/types/src/client.ts | 4 +-- 9 files changed, 16 insertions(+), 60 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/init.js b/dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/init.js index 934c6f5bca6a..e25d99258844 100644 --- a/dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/init.js +++ b/dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/init.js @@ -14,10 +14,6 @@ Sentry.init({ replaysOnErrorSampleRate: 1.0, integrations: [window.Replay], transport: options => { - const transport = new Sentry.makeFetchTransport(options); - - delete transport.send.__sentry__baseTransport__; - - return transport; + return new Sentry.makeFetchTransport(options); }, }); diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index f1b1685b12ce..d82e585f4be6 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -432,10 +432,7 @@ export abstract class BaseClient implements Client { 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 implements Client { 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; diff --git a/packages/core/src/transports/base.ts b/packages/core/src/transports/base.ts index 2fbeb27e0ed5..b942266457f9 100644 --- a/packages/core/src/transports/base.ts +++ b/packages/core/src/transports/base.ts @@ -105,10 +105,6 @@ export function createTransport( ); } - // 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, diff --git a/packages/feedback/src/util/handleFeedbackSubmit.ts b/packages/feedback/src/util/handleFeedbackSubmit.ts index abb3aeb1368d..a3bf57fcbfa0 100644 --- a/packages/feedback/src/util/handleFeedbackSubmit.ts +++ b/packages/feedback/src/util/handleFeedbackSubmit.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 { +): Promise { 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({}); } diff --git a/packages/feedback/test/utils/mockSdk.ts b/packages/feedback/test/utils/mockSdk.ts index 0dbc30c02cb6..c6afbc018317 100644 --- a/packages/feedback/test/utils/mockSdk.ts +++ b/packages/feedback/test/utils/mockSdk.ts @@ -11,16 +11,11 @@ class MockTransport implements Transport { send: (request: Envelope) => PromiseLike; constructor() { - const send: ((request: Envelope) => PromiseLike) & { - __sentry__baseTransport__?: boolean; - } = jest.fn(async () => { + this.send = jest.fn(async () => { return { statusCode: 200, }; }); - - send.__sentry__baseTransport__ = true; - this.send = send; } async flush() { diff --git a/packages/replay/src/coreHandlers/handleAfterSendEvent.ts b/packages/replay/src/coreHandlers/handleAfterSendEvent.ts index 28e112e736f0..14e92d404fc7 100644 --- a/packages/replay/src/coreHandlers/handleAfterSendEvent.ts +++ b/packages/replay/src/coreHandlers/handleAfterSendEvent.ts @@ -1,20 +1,15 @@ -import { getClient } from '@sentry/core'; -import type { ErrorEvent, Event, TransactionEvent, Transport, TransportMakeRequestResponse } from '@sentry/types'; +import type { ErrorEvent, Event, TransactionEvent, TransportMakeRequestResponse } from '@sentry/types'; import type { ReplayContainer } from '../types'; import { isErrorEvent, isTransactionEvent } from '../util/eventUtils'; -type AfterSendEventCallback = (event: Event, sendResponse: TransportMakeRequestResponse | void) => void; +type AfterSendEventCallback = (event: Event, sendResponse: TransportMakeRequestResponse) => void; /** * Returns a listener to be added to `client.on('afterSendErrorEvent, listener)`. */ export function handleAfterSendEvent(replay: ReplayContainer): AfterSendEventCallback { - // Custom transports may still be returning `Promise`, which means we cannot expect the status code to be available there - // TODO (v8): remove this check as it will no longer be necessary - const enforceStatusCode = isBaseTransportSend(); - - return (event: Event, sendResponse: TransportMakeRequestResponse | void) => { + return (event: Event, sendResponse: TransportMakeRequestResponse) => { if (!replay.isEnabled() || (!isErrorEvent(event) && !isTransactionEvent(event))) { return; } @@ -24,7 +19,7 @@ export function handleAfterSendEvent(replay: ReplayContainer): AfterSendEventCal // We only want to do stuff on successful error sending, otherwise you get error replays without errors attached // If not using the base transport, we allow `undefined` response (as a custom transport may not implement this correctly yet) // If we do use the base transport, we skip if we encountered an non-OK status code - if (enforceStatusCode && (!statusCode || statusCode < 200 || statusCode >= 300)) { + if (!statusCode || statusCode < 200 || statusCode >= 300) { return; } @@ -79,19 +74,3 @@ function handleErrorEvent(replay: ReplayContainer, event: ErrorEvent): void { replay.sendBufferedReplayOrFlush(); }); } - -function isBaseTransportSend(): boolean { - const client = getClient(); - if (!client) { - return false; - } - - const transport = client.getTransport(); - if (!transport) { - return false; - } - - return ( - (transport.send as Transport['send'] & { __sentry__baseTransport__?: true }).__sentry__baseTransport__ || false - ); -} diff --git a/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts b/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts index 3cb668a83a67..22b572b1e422 100644 --- a/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts +++ b/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts @@ -152,10 +152,6 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => { }, })); - const client = getClient()!; - // @ts-expect-error make sure to remove this - delete client.getTransport()!.send.__sentry__baseTransport__; - const error1 = Error({ event_id: 'err1' }); const error2 = Error({ event_id: 'err2' }); const error3 = Error({ event_id: 'err3' }); diff --git a/packages/replay/test/mocks/mockSdk.ts b/packages/replay/test/mocks/mockSdk.ts index ccf11dabb6ad..2ada7cec506f 100644 --- a/packages/replay/test/mocks/mockSdk.ts +++ b/packages/replay/test/mocks/mockSdk.ts @@ -16,16 +16,11 @@ class MockTransport implements Transport { send: (request: Envelope) => PromiseLike; constructor() { - const send: ((request: Envelope) => PromiseLike) & { - __sentry__baseTransport__?: boolean; - } = jest.fn(async () => { + this.send = jest.fn(async () => { return { statusCode: 200, }; }); - - send.__sentry__baseTransport__ = true; - this.send = send; } async flush() { diff --git a/packages/types/src/client.ts b/packages/types/src/client.ts index 25d2fe0b54ed..83b48ac24d9c 100644 --- a/packages/types/src/client.ts +++ b/packages/types/src/client.ts @@ -215,7 +215,7 @@ export interface Client { /** * Register a callback for when an event has been sent. */ - on(hook: 'afterSendEvent', callback: (event: Event, sendResponse: TransportMakeRequestResponse | void) => void): void; + on(hook: 'afterSendEvent', callback: (event: Event, sendResponse: TransportMakeRequestResponse) => void): void; /** * Register a callback before a breadcrumb is added. @@ -292,7 +292,7 @@ export interface Client { * Fire a hook event after sending an event. Expects to be given an Event as the * second argument. */ - emit(hook: 'afterSendEvent', event: Event, sendResponse: TransportMakeRequestResponse | void): void; + emit(hook: 'afterSendEvent', event: Event, sendResponse: TransportMakeRequestResponse): void; /** * Fire a hook for when a breadcrumb is added. Expects the breadcrumb as second argument. From 33ff70149b7fe3eda3a8657621973a720e890af3 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 22 Feb 2024 11:00:16 -0500 Subject: [PATCH 3/9] build fix --- packages/core/src/baseclient.ts | 2 +- packages/vercel-edge/src/transports/index.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts index d82e585f4be6..8d36844a96bf 100644 --- a/packages/core/src/baseclient.ts +++ b/packages/core/src/baseclient.ts @@ -521,7 +521,7 @@ export abstract class BaseClient implements Client { if (this._isEnabled() && this._transport) { return this._transport.send(envelope).then(null, reason => { DEBUG_BUILD && logger.error('Error while sending event:', reason); - return {}; + return reason; }); } diff --git a/packages/vercel-edge/src/transports/index.ts b/packages/vercel-edge/src/transports/index.ts index b2b899ecdb42..4e8a35ac7c39 100644 --- a/packages/vercel-edge/src/transports/index.ts +++ b/packages/vercel-edge/src/transports/index.ts @@ -37,13 +37,13 @@ export class IsolatedPromiseBuffer { /** * @inheritdoc */ - public add(taskProducer: () => PromiseLike): PromiseLike { + public add(taskProducer: () => PromiseLike): PromiseLike { if (this._taskProducers.length >= this._bufferSize) { return Promise.reject(new SentryError('Not adding Promise because buffer limit was reached.')); } this._taskProducers.push(taskProducer); - return Promise.resolve(); + return Promise.resolve({}); } /** From 9b482d7d4fbe5584dc30eb78cc6e1ab3d5c45aed Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 22 Feb 2024 11:51:37 -0500 Subject: [PATCH 4/9] fix tests --- packages/core/test/lib/base.test.ts | 2 +- packages/core/test/lib/transports/base.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/test/lib/base.test.ts b/packages/core/test/lib/base.test.ts index b48d3b0e5eec..85f624e3107b 100644 --- a/packages/core/test/lib/base.test.ts +++ b/packages/core/test/lib/base.test.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 () => { diff --git a/packages/core/test/lib/transports/base.test.ts b/packages/core/test/lib/transports/base.test.ts index 01cc8bf59c9b..1425c55c45a7 100644 --- a/packages/core/test/lib/transports/base.test.ts +++ b/packages/core/test/lib/transports/base.test.ts @@ -35,7 +35,7 @@ const transportOptions = { describe('createTransport', () => { it('flushes the buffer', async () => { - const mockBuffer: PromiseBuffer = { + const mockBuffer: PromiseBuffer = { $: [], add: jest.fn(), drain: jest.fn(), From d78d48e4650e378970e551a79aeea7e0474b8e1c Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 22 Feb 2024 12:14:28 -0500 Subject: [PATCH 5/9] fix replay test --- .../coreHandlers/handleAfterSendEvent.test.ts | 40 +++---------------- .../vercel-edge/test/wintercg-fetch.test.ts | 2 +- 2 files changed, 6 insertions(+), 36 deletions(-) diff --git a/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts b/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts index 22b572b1e422..03f4c236b81d 100644 --- a/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts +++ b/packages/replay/test/integration/coreHandlers/handleAfterSendEvent.test.ts @@ -35,8 +35,8 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => { const handler = handleAfterSendEvent(replay); - // With undefined response: Don't capture - handler(error1, undefined); + // With empty response: Don't capture + handler(error1, {}); // With "successful" response: Capture handler(error2, { statusCode: 200 }); // With "unsuccessful" response: Don't capture @@ -66,8 +66,8 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => { const handler = handleAfterSendEvent(replay); - // With undefined response: Don't capture - handler(transaction1, undefined); + // With empty response: Don't capture + handler(transaction1, {}); // With "successful" response: Capture handler(transaction2, { statusCode: 200 }); // With "unsuccessful" response: Don't capture @@ -141,36 +141,6 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => { ); }); - it('allows undefined send response when using custom transport', async () => { - ({ replay } = await resetSdkMock({ - replayOptions: { - stickySession: false, - }, - sentryOptions: { - replaysSessionSampleRate: 1.0, - replaysOnErrorSampleRate: 0.0, - }, - })); - - const error1 = Error({ event_id: 'err1' }); - const error2 = Error({ event_id: 'err2' }); - const error3 = Error({ event_id: 'err3' }); - const error4 = Error({ event_id: 'err4' }); - - const handler = handleAfterSendEvent(replay); - - // With undefined response: Capture - handler(error1, undefined); - // With "successful" response: Capture - handler(error2, { statusCode: 200 }); - // With "unsuccessful" response: Capture - handler(error3, { statusCode: 0 }); - // With no statusCode response: Capture - handler(error4, { statusCode: undefined }); - - expect(Array.from(replay.getContext().errorIds)).toEqual(['err1', 'err2', 'err3', 'err4']); - }); - it('flushes when in buffer mode', async () => { ({ replay } = await resetSdkMock({ replayOptions: { @@ -298,7 +268,7 @@ describe('Integration | coreHandlers | handleAfterSendEvent', () => { expect(replay.recordingMode).toBe('buffer'); - handler(error1, undefined); + handler(error1, {}); expect(Array.from(replay.getContext().errorIds)).toEqual([]); diff --git a/packages/vercel-edge/test/wintercg-fetch.test.ts b/packages/vercel-edge/test/wintercg-fetch.test.ts index f9e1c3ca8aed..6439795870f7 100644 --- a/packages/vercel-edge/test/wintercg-fetch.test.ts +++ b/packages/vercel-edge/test/wintercg-fetch.test.ts @@ -29,7 +29,7 @@ describe('WinterCGFetch instrumentation', () => { tracesSampleRate: 1, integrations: [], transport: () => ({ - send: () => Promise.resolve(undefined), + send: () => Promise.resolve({}), flush: () => Promise.resolve(true), }), tracePropagationTargets: ['http://my-website.com/'], From 90d64327b872eaed4c992dfbf12f8cef7ed2505d Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 27 Feb 2024 15:27:33 -0500 Subject: [PATCH 6/9] fix tests --- .../errors/errorModeCustomTransport/init.js | 19 --------- .../errors/errorModeCustomTransport/test.ts | 42 ------------------- packages/feedback/src/widget/createWidget.ts | 2 +- packages/profiling-node/test/index.test.ts | 2 +- 4 files changed, 2 insertions(+), 63 deletions(-) delete mode 100644 dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/init.js delete mode 100644 dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/test.ts diff --git a/dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/init.js b/dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/init.js deleted file mode 100644 index e25d99258844..000000000000 --- a/dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/init.js +++ /dev/null @@ -1,19 +0,0 @@ -import * as Sentry from '@sentry/browser'; - -window.Sentry = Sentry; -window.Replay = new Sentry.Replay({ - flushMinDelay: 200, - flushMaxDelay: 200, - minReplayDuration: 0, -}); - -Sentry.init({ - dsn: 'https://public@dsn.ingest.sentry.io/1337', - sampleRate: 1, - replaysSessionSampleRate: 0.0, - replaysOnErrorSampleRate: 1.0, - integrations: [window.Replay], - transport: options => { - return new Sentry.makeFetchTransport(options); - }, -}); diff --git a/dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/test.ts b/dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/test.ts deleted file mode 100644 index 81ff9f06cc78..000000000000 --- a/dev-packages/browser-integration-tests/suites/replay/errors/errorModeCustomTransport/test.ts +++ /dev/null @@ -1,42 +0,0 @@ -import { expect } from '@playwright/test'; - -import { sentryTest } from '../../../../utils/fixtures'; -import { getReplaySnapshot, shouldSkipReplayTest, waitForReplayRequest } from '../../../../utils/replayHelpers'; - -sentryTest( - '[error-mode] should handle errors with custom transport', - async ({ getLocalTestPath, page, forceFlushReplay }) => { - if (shouldSkipReplayTest()) { - sentryTest.skip(); - } - - const promiseReq0 = waitForReplayRequest(page, 0); - const promiseReq1 = waitForReplayRequest(page, 1); - - let callsToSentry = 0; - - await page.route('https://dsn.ingest.sentry.io/**/*', route => { - callsToSentry++; - - return route.fulfill({ - // Only error out for error, then succeed - status: callsToSentry === 1 ? 422 : 200, - }); - }); - - const url = await getLocalTestPath({ testDir: __dirname }); - - await page.goto(url); - await forceFlushReplay(); - expect(callsToSentry).toEqual(0); - - await page.locator('#error').click(); - await promiseReq0; - - await forceFlushReplay(); - await promiseReq1; - - const replay = await getReplaySnapshot(page); - expect(replay.recordingMode).toBe('session'); - }, -); diff --git a/packages/feedback/src/widget/createWidget.ts b/packages/feedback/src/widget/createWidget.ts index 05b52ca64725..1f6ed91bf04e 100644 --- a/packages/feedback/src/widget/createWidget.ts +++ b/packages/feedback/src/widget/createWidget.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(); } diff --git a/packages/profiling-node/test/index.test.ts b/packages/profiling-node/test/index.test.ts index 3a59d4bd53f4..0a074226f692 100644 --- a/packages/profiling-node/test/index.test.ts +++ b/packages/profiling-node/test/index.test.ts @@ -14,7 +14,7 @@ function makeStaticTransport(): MockTransport { events: [] as any[], send: function (...args: any[]) { this.events.push(args); - return Promise.resolve(); + return Promise.resolve({}); }, flush: function () { return Promise.resolve(true); From 837059c1b74ac250442b51f542236d8004afd469 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 27 Feb 2024 16:10:04 -0500 Subject: [PATCH 7/9] fix tests again --- .../feedback/test/widget/createWidget.test.ts | 4 ++-- .../test/hubextensions.hub.test.ts | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/feedback/test/widget/createWidget.test.ts b/packages/feedback/test/widget/createWidget.test.ts index c2a5f50daebc..0fba6f452084 100644 --- a/packages/feedback/test/widget/createWidget.test.ts +++ b/packages/feedback/test/widget/createWidget.test.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')); diff --git a/packages/profiling-node/test/hubextensions.hub.test.ts b/packages/profiling-node/test/hubextensions.hub.test.ts index 755923f62af7..2266e5cec610 100644 --- a/packages/profiling-node/test/hubextensions.hub.test.ts +++ b/packages/profiling-node/test/hubextensions.hub.test.ts @@ -98,7 +98,7 @@ describe('hubextensions', () => { // eslint-disable-next-line deprecation/deprecation hub.bindClient(client); - const transportSpy = jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve()); + const transportSpy = jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve({})); // eslint-disable-next-line deprecation/deprecation const transaction = Sentry.getCurrentHub().startTransaction({ name: 'profile_hub' }); @@ -135,7 +135,7 @@ describe('hubextensions', () => { }; }); - jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve()); + jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve({})); // eslint-disable-next-line deprecation/deprecation const transaction = Sentry.getCurrentHub().startTransaction({ name: 'profile_hub' }); @@ -183,7 +183,7 @@ describe('hubextensions', () => { }; }); - jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve()); + jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve({})); // eslint-disable-next-line deprecation/deprecation const transaction = Sentry.getCurrentHub().startTransaction({ name: 'profile_hub', traceId: 'boop' }); @@ -205,7 +205,7 @@ describe('hubextensions', () => { const startProfilingSpy = jest.spyOn(CpuProfilerBindings, 'startProfiling'); const stopProfilingSpy = jest.spyOn(CpuProfilerBindings, 'stopProfiling'); - jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve()); + jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve({})); // eslint-disable-next-line deprecation/deprecation const transaction = hub.startTransaction({ name: 'profile_hub' }); @@ -225,7 +225,7 @@ describe('hubextensions', () => { // eslint-disable-next-line deprecation/deprecation hub.bindClient(client); - const transportSpy = jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve()); + const transportSpy = jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve({})); // eslint-disable-next-line deprecation/deprecation const transaction = hub.startTransaction({ name: 'profile_hub' }); @@ -276,7 +276,7 @@ describe('hubextensions', () => { // Emit is sync, so we can just assert that we got here const transportSpy = jest.spyOn(transport, 'send').mockImplementation(() => { // Do nothing so we don't send events to Sentry - return Promise.resolve(); + return Promise.resolve({}); }); // eslint-disable-next-line deprecation/deprecation @@ -348,7 +348,7 @@ describe('hubextensions', () => { const transportSpy = jest.spyOn(transport, 'send').mockImplementation(() => { // Do nothing so we don't send events to Sentry - return Promise.resolve(); + return Promise.resolve({}); }); // eslint-disable-next-line deprecation/deprecation @@ -441,7 +441,7 @@ describe('hubextensions', () => { // eslint-disable-next-line deprecation/deprecation hub.bindClient(client); - const transportSpy = jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve()); + const transportSpy = jest.spyOn(transport, 'send').mockReturnValue(Promise.resolve({})); // eslint-disable-next-line deprecation/deprecation const transaction = hub.startTransaction({ name: 'profile_hub' }); From 3aed07c86d6f388499ac1a9fc3aa224a9908c836 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 4 Mar 2024 12:42:02 -0500 Subject: [PATCH 8/9] update migration guide --- MIGRATION.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/MIGRATION.md b/MIGRATION.md index aae2f3ee4292..e3ec5abec9ee 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -435,6 +435,22 @@ 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; +} + +// After (v8) +interface Transport { + send(event: Event): Promise; +} +``` + ### Browser SDK (Browser, React, Vue, Angular, Ember, etc.) Removed top-level exports: `Offline`, `makeXHRTransport`, `BrowserTracing` From b5182829b422ddae7955700a62f979fddbdeb5b8 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 4 Mar 2024 12:50:09 -0500 Subject: [PATCH 9/9] yarn fix --- MIGRATION.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/MIGRATION.md b/MIGRATION.md index e3ec5abec9ee..ccbd4e6db9bc 100644 --- a/MIGRATION.md +++ b/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()` @@ -437,7 +438,8 @@ The `lastEventId` function has been removed. See [below](./MIGRATION.md#deprecat #### 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. +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)