Skip to content

Commit 3bc8532

Browse files
committed
ref: Avoid some usage of SyncPromise where not needed
We make quite heavy use of it in event processing, not touching this for now, but in other places it should not be needed IMHO. fix transport using async refactor some more sync promise stuff fix node client fix some flushing stuff fix check oops small fixees fix tests fix lint fix to promise like small ref avoid special casing of no fetch implementation fetch stuff
1 parent 660d533 commit 3bc8532

File tree

10 files changed

+65
-82
lines changed

10 files changed

+65
-82
lines changed

packages/browser/test/profiling/integration.test.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
* @vitest-environment jsdom
33
*/
44

5-
import type { BrowserClient } from '@sentry/browser';
65
import * as Sentry from '@sentry/browser';
76
import { describe, expect, it, vi } from 'vitest';
87
import type { JSSelfProfile } from '../../src/profiling/jsSelfProfiling';
@@ -36,7 +35,7 @@ describe('BrowserProfilingIntegration', () => {
3635

3736
const flush = vi.fn().mockImplementation(() => Promise.resolve(true));
3837
const send = vi.fn().mockImplementation(() => Promise.resolve());
39-
Sentry.init({
38+
const client = Sentry.init({
4039
tracesSampleRate: 1,
4140
profilesSampleRate: 1,
4241
environment: 'test-environment',
@@ -50,13 +49,11 @@ describe('BrowserProfilingIntegration', () => {
5049
integrations: [Sentry.browserTracingIntegration(), Sentry.browserProfilingIntegration()],
5150
});
5251

53-
const client = Sentry.getClient<BrowserClient>();
54-
5552
const currentTransaction = Sentry.getActiveSpan();
5653
expect(currentTransaction).toBeDefined();
5754
expect(Sentry.spanToJSON(currentTransaction!).op).toBe('pageload');
5855
currentTransaction?.end();
59-
await client?.flush(1000);
56+
await client!.flush(1000);
6057

6158
expect(send).toHaveBeenCalledTimes(1);
6259

packages/bun/src/transports/index.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type { BaseTransportOptions, Transport, TransportMakeRequestResponse, TransportRequest } from '@sentry/core';
2-
import { createTransport, rejectedSyncPromise, suppressTracing } from '@sentry/core';
2+
import { createTransport, suppressTracing } from '@sentry/core';
33

44
export interface BunTransportOptions extends BaseTransportOptions {
55
/** Custom headers for the transport. Used by the XHRTransport and FetchTransport */
@@ -30,7 +30,7 @@ export function makeFetchTransport(options: BunTransportOptions): Transport {
3030
});
3131
});
3232
} catch (e) {
33-
return rejectedSyncPromise(e);
33+
return Promise.reject(e);
3434
}
3535
}
3636

packages/core/src/client.ts

Lines changed: 38 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ import { parseSampleRate } from './utils/parseSampleRate';
4444
import { prepareEvent } from './utils/prepareEvent';
4545
import { reparentChildSpans, shouldIgnoreSpan } from './utils/should-ignore-span';
4646
import { getActiveSpan, showSpanDropWarning, spanToTraceContext } from './utils/spanUtils';
47-
import { rejectedSyncPromise, resolvedSyncPromise, SyncPromise } from './utils/syncpromise';
47+
import { rejectedSyncPromise } from './utils/syncpromise';
4848
import { convertSpanJsonToTransactionEvent, convertTransactionEventToSpanJson } from './utils/transactionEvent';
4949

5050
const ALREADY_SEEN_ERROR = "Not capturing exception because it's already been captured.";
@@ -316,16 +316,19 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
316316
* @returns A promise that will resolve with `true` if all events are sent before the timeout, or `false` if there are
317317
* still events in the queue when the timeout is reached.
318318
*/
319-
public flush(timeout?: number): PromiseLike<boolean> {
319+
// @ts-expect-error - PromiseLike is a subset of Promise
320+
public async flush(timeout?: number): PromiseLike<boolean> {
320321
const transport = this._transport;
321-
if (transport) {
322-
this.emit('flush');
323-
return this._isClientDoneProcessing(timeout).then(clientFinished => {
324-
return transport.flush(timeout).then(transportFlushed => clientFinished && transportFlushed);
325-
});
326-
} else {
327-
return resolvedSyncPromise(true);
322+
if (!transport) {
323+
return Promise.resolve(true);
328324
}
325+
326+
this.emit('flush');
327+
328+
const clientFinished = await this._isClientDoneProcessing(timeout);
329+
const transportFlushed = await transport.flush(timeout);
330+
331+
return clientFinished && transportFlushed;
329332
}
330333

331334
/**
@@ -336,12 +339,12 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
336339
* @returns {Promise<boolean>} A promise which resolves to `true` if the flush completes successfully before the timeout, or `false` if
337340
* it doesn't.
338341
*/
339-
public close(timeout?: number): PromiseLike<boolean> {
340-
return this.flush(timeout).then(result => {
341-
this.getOptions().enabled = false;
342-
this.emit('close');
343-
return result;
344-
});
342+
// @ts-expect-error - PromiseLike is a subset of Promise
343+
public async close(timeout?: number): PromiseLike<boolean> {
344+
const result = await this.flush(timeout);
345+
this.getOptions().enabled = false;
346+
this.emit('close');
347+
return result;
345348
}
346349

347350
/**
@@ -872,18 +875,21 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
872875
/**
873876
* Send an envelope to Sentry.
874877
*/
875-
public sendEnvelope(envelope: Envelope): PromiseLike<TransportMakeRequestResponse> {
878+
// @ts-expect-error - PromiseLike is a subset of Promise
879+
public async sendEnvelope(envelope: Envelope): PromiseLike<TransportMakeRequestResponse> {
876880
this.emit('beforeEnvelope', envelope);
877881

878882
if (this._isEnabled() && this._transport) {
879-
return this._transport.send(envelope).then(null, reason => {
883+
try {
884+
return await this._transport.send(envelope);
885+
} catch (reason) {
880886
DEBUG_BUILD && debug.error('Error while sending envelope:', reason);
881887
return {};
882-
});
888+
}
883889
}
884890

885891
DEBUG_BUILD && debug.error('Transport disabled');
886-
return resolvedSyncPromise({});
892+
return {};
887893
}
888894

889895
/* eslint-enable @typescript-eslint/unified-signatures */
@@ -938,24 +944,20 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
938944
* @returns A promise which will resolve to `true` if processing is already done or finishes before the timeout, and
939945
* `false` otherwise
940946
*/
941-
protected _isClientDoneProcessing(timeout?: number): PromiseLike<boolean> {
942-
return new SyncPromise(resolve => {
943-
let ticked: number = 0;
944-
const tick: number = 1;
947+
protected async _isClientDoneProcessing(timeout?: number): Promise<boolean> {
948+
let ticked = 0;
945949

946-
const interval = setInterval(() => {
947-
if (this._numProcessing == 0) {
948-
clearInterval(interval);
949-
resolve(true);
950-
} else {
951-
ticked += tick;
952-
if (timeout && ticked >= timeout) {
953-
clearInterval(interval);
954-
resolve(false);
955-
}
956-
}
957-
}, tick);
958-
});
950+
// if no timeout is provided, we wait "forever" until everything is processed
951+
while (!timeout || ticked < timeout) {
952+
await new Promise(resolve => setTimeout(resolve, 1));
953+
954+
if (!this._numProcessing) {
955+
return true;
956+
}
957+
ticked++;
958+
}
959+
960+
return false;
959961
}
960962

961963
/** Determines whether this SDK is enabled and a transport is present. */

packages/core/src/transports/base.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import {
1616
} from '../utils/envelope';
1717
import { type PromiseBuffer, makePromiseBuffer, SENTRY_BUFFER_FULL_ERROR } from '../utils/promisebuffer';
1818
import { type RateLimits, isRateLimited, updateRateLimits } from '../utils/ratelimit';
19-
import { resolvedSyncPromise } from '../utils/syncpromise';
2019

2120
export const DEFAULT_TRANSPORT_BUFFER_SIZE = 64;
2221

@@ -51,7 +50,7 @@ export function createTransport(
5150

5251
// Skip sending if envelope is empty after filtering out rate limited events
5352
if (filteredEnvelopeItems.length === 0) {
54-
return resolvedSyncPromise({});
53+
return Promise.resolve({});
5554
}
5655

5756
const filteredEnvelope: Envelope = createEnvelope(envelope[0], filteredEnvelopeItems as (typeof envelope)[1]);
@@ -87,7 +86,7 @@ export function createTransport(
8786
if (error === SENTRY_BUFFER_FULL_ERROR) {
8887
DEBUG_BUILD && debug.error('Skipped sending event because buffer is full.');
8988
recordEnvelopeLoss('queue_overflow');
90-
return resolvedSyncPromise({});
89+
return Promise.resolve({});
9190
} else {
9291
throw error;
9392
}

packages/core/test/lib/client.test.ts

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2200,11 +2200,7 @@ describe('Client', () => {
22002200
client.on('afterSendEvent', callback);
22012201

22022202
client.sendEvent(errorEvent);
2203-
vi.runAllTimers();
2204-
// Wait for two ticks
2205-
// note that for whatever reason, await new Promise(resolve => setTimeout(resolve, 0)) causes the test to hang
2206-
await undefined;
2207-
await undefined;
2203+
await vi.runAllTimersAsync();
22082204

22092205
expect(mockSend).toBeCalledTimes(1);
22102206
expect(callback).toBeCalledTimes(1);
@@ -2228,11 +2224,7 @@ describe('Client', () => {
22282224
client.on('afterSendEvent', callback);
22292225

22302226
client.sendEvent(transactionEvent);
2231-
vi.runAllTimers();
2232-
// Wait for two ticks
2233-
// note that for whatever reason, await new Promise(resolve => setTimeout(resolve, 0)) causes the test to hang
2234-
await undefined;
2235-
await undefined;
2227+
await vi.runAllTimersAsync();
22362228

22372229
expect(mockSend).toBeCalledTimes(1);
22382230
expect(callback).toBeCalledTimes(1);
@@ -2260,11 +2252,7 @@ describe('Client', () => {
22602252
client.on('afterSendEvent', callback);
22612253

22622254
client.sendEvent(errorEvent);
2263-
vi.runAllTimers();
2264-
// Wait for two ticks
2265-
// note that for whatever reason, await new Promise(resolve => setTimeout(resolve, 0)) causes the test to hang
2266-
await undefined;
2267-
await undefined;
2255+
await vi.runAllTimersAsync();
22682256

22692257
expect(mockSend).toBeCalledTimes(1);
22702258
expect(callback).toBeCalledTimes(1);

packages/deno/src/integrations/deno-cron.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,11 @@ const _denoCronIntegration = (() => {
1515
return {
1616
name: INTEGRATION_NAME,
1717
setupOnce() {
18-
// eslint-disable-next-line deprecation/deprecation
1918
if (!Deno.cron) {
2019
// The cron API is not available in this Deno version use --unstable flag!
2120
return;
2221
}
2322

24-
// eslint-disable-next-line deprecation/deprecation
2523
Deno.cron = new Proxy(Deno.cron, {
2624
apply(target, thisArg, argArray: CronParams) {
2725
const [monitorSlug, schedule, opt1, opt2] = argArray;

packages/deno/src/transports/index.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type { BaseTransportOptions, Transport, TransportMakeRequestResponse, TransportRequest } from '@sentry/core';
2-
import { consoleSandbox, createTransport, debug, rejectedSyncPromise, suppressTracing } from '@sentry/core';
2+
import { consoleSandbox, createTransport, debug, suppressTracing } from '@sentry/core';
33

44
export interface DenoTransportOptions extends BaseTransportOptions {
55
/** Custom headers for the transport. Used by the XHRTransport and FetchTransport */
@@ -48,7 +48,7 @@ export function makeFetchTransport(options: DenoTransportOptions): Transport {
4848
});
4949
});
5050
} catch (e) {
51-
return rejectedSyncPromise(e);
51+
return Promise.reject(e);
5252
}
5353
}
5454

packages/node-core/src/sdk/client.ts

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -77,9 +77,9 @@ export class NodeClient extends ServerRuntimeClient<NodeClientOptions> {
7777
return tracer;
7878
}
7979

80-
// Eslint ignore explanation: This is already documented in super.
81-
// eslint-disable-next-line jsdoc/require-jsdoc
82-
public async flush(timeout?: number): Promise<boolean> {
80+
/** @inheritDoc */
81+
// @ts-expect-error - PromiseLike is a subset of Promise
82+
public async flush(timeout?: number): PromiseLike<boolean> {
8383
await this.traceProvider?.forceFlush();
8484

8585
if (this.getOptions().sendClientReports) {
@@ -89,9 +89,9 @@ export class NodeClient extends ServerRuntimeClient<NodeClientOptions> {
8989
return super.flush(timeout);
9090
}
9191

92-
// Eslint ignore explanation: This is already documented in super.
93-
// eslint-disable-next-line jsdoc/require-jsdoc
94-
public close(timeout?: number | undefined): PromiseLike<boolean> {
92+
/** @inheritDoc */
93+
// @ts-expect-error - PromiseLike is a subset of Promise
94+
public async close(timeout?: number | undefined): PromiseLike<boolean> {
9595
if (this._clientReportInterval) {
9696
clearInterval(this._clientReportInterval);
9797
}
@@ -104,11 +104,12 @@ export class NodeClient extends ServerRuntimeClient<NodeClientOptions> {
104104
process.off('beforeExit', this._logOnExitFlushListener);
105105
}
106106

107-
return super
108-
.close(timeout)
109-
.then(allEventsSent =>
110-
this.traceProvider ? this.traceProvider.shutdown().then(() => allEventsSent) : allEventsSent,
111-
);
107+
const allEventsSent = await super.close(timeout);
108+
if (this.traceProvider) {
109+
await this.traceProvider.shutdown();
110+
}
111+
112+
return allEventsSent;
112113
}
113114

114115
/**

packages/node-core/test/sdk/client.test.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -362,9 +362,7 @@ describe('NodeClient', () => {
362362

363363
expect(result).toBe(true);
364364

365-
// once call directly in close to stop client reports,
366-
// the other in core client `_isClientDoneProcessing`
367-
expect(clearIntervalSpy).toHaveBeenCalledTimes(2);
365+
expect(clearIntervalSpy).toHaveBeenCalledTimes(1);
368366

369367
// removes `_clientReportOnExitFlushListener`
370368
expect(processOffSpy).toHaveBeenNthCalledWith(1, 'beforeExit', expect.any(Function));

packages/replay-internal/src/util/sendReplayRequest.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type { RateLimits, ReplayEvent, TransportMakeRequestResponse } from '@sentry/core';
2-
import { getClient, getCurrentScope, isRateLimited, resolvedSyncPromise, updateRateLimits } from '@sentry/core';
2+
import { getClient, getCurrentScope, isRateLimited, updateRateLimits } from '@sentry/core';
33
import { REPLAY_EVENT_NAME, UNABLE_TO_SEND_REPLAY } from '../constants';
44
import { DEBUG_BUILD } from '../debug-build';
55
import type { SendReplayData } from '../types';
@@ -34,7 +34,7 @@ export async function sendReplayRequest({
3434
const dsn = client?.getDsn();
3535

3636
if (!client || !transport || !dsn || !session.sampled) {
37-
return resolvedSyncPromise({});
37+
return Promise.resolve({});
3838
}
3939

4040
const baseEvent: ReplayEvent = {
@@ -55,7 +55,7 @@ export async function sendReplayRequest({
5555
// Taken from baseclient's `_processEvent` method, where this is handled for errors/transactions
5656
client.recordDroppedEvent('event_processor', 'replay');
5757
DEBUG_BUILD && debug.log('An event processor returned `null`, will not send event.');
58-
return resolvedSyncPromise({});
58+
return Promise.resolve({});
5959
}
6060

6161
/*

0 commit comments

Comments
 (0)