Skip to content

feat(utils): Kill SyncPromise.reject and SyncPromise.resolve #4329

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

Merged
merged 4 commits into from
Dec 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/browser/src/eventbuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {
isErrorEvent,
isEvent,
isPlainObject,
SyncPromise,
resolvedSyncPromise,
} from '@sentry/utils';

import { eventFromPlainObject, eventFromStacktrace, prepareFramesForEvent } from './parsers';
Expand All @@ -28,7 +28,7 @@ export function eventFromException(options: Options, exception: unknown, hint?:
if (hint && hint.event_id) {
event.event_id = hint.event_id;
}
return SyncPromise.resolve(event);
return resolvedSyncPromise(event);
}

/**
Expand All @@ -49,7 +49,7 @@ export function eventFromMessage(
if (hint && hint.event_id) {
event.event_id = hint.event_id;
}
return SyncPromise.resolve(event);
return resolvedSyncPromise(event);
}

/**
Expand Down
6 changes: 3 additions & 3 deletions packages/browser/src/sdk.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getCurrentHub, initAndBind, Integrations as CoreIntegrations } from '@sentry/core';
import { addInstrumentationHandler, getGlobalObject, logger, SyncPromise } from '@sentry/utils';
import { addInstrumentationHandler, getGlobalObject, logger, resolvedSyncPromise } from '@sentry/utils';

import { BrowserOptions } from './backend';
import { BrowserClient } from './client';
Expand Down Expand Up @@ -162,7 +162,7 @@ export function flush(timeout?: number): PromiseLike<boolean> {
return client.flush(timeout);
}
logger.warn('Cannot flush events. No client defined.');
return SyncPromise.resolve(false);
return resolvedSyncPromise(false);
}

/**
Expand All @@ -179,7 +179,7 @@ export function close(timeout?: number): PromiseLike<boolean> {
return client.close(timeout);
}
logger.warn('Cannot flush events and disable SDK. No client defined.');
return SyncPromise.resolve(false);
return resolvedSyncPromise(false);
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/browser/test/unit/mocks/simpletransport.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { eventStatusFromHttpCode, SyncPromise } from '@sentry/utils';
import { eventStatusFromHttpCode, resolvedSyncPromise } from '@sentry/utils';

import { Event, Response } from '../../../src';
import { BaseTransport } from '../../../src/transports';

export class SimpleTransport extends BaseTransport {
public sendEvent(_: Event): PromiseLike<Response> {
return this._buffer.add(() =>
SyncPromise.resolve({
resolvedSyncPromise({
status: eventStatusFromHttpCode(200),
}),
);
Expand Down
8 changes: 5 additions & 3 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import {
isThenable,
logger,
normalize,
rejectedSyncPromise,
resolvedSyncPromise,
SentryError,
SyncPromise,
truncate,
Expand Down Expand Up @@ -356,7 +358,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
}

// We prepare the result here with a resolved Event.
let result = SyncPromise.resolve<Event | null>(prepared);
let result = resolvedSyncPromise<Event | null>(prepared);

// This should be the last thing called, since we want that
// {@link Hub.addEventProcessor} gets the finished prepared event.
Expand Down Expand Up @@ -531,7 +533,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
}

if (!this._isEnabled()) {
return SyncPromise.reject(new SentryError('SDK not enabled, will not capture event.'));
return rejectedSyncPromise(new SentryError('SDK not enabled, will not capture event.'));
}

const isTransaction = event.type === 'transaction';
Expand All @@ -540,7 +542,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement
// Sampling for transaction happens somewhere else
if (!isTransaction && typeof sampleRate === 'number' && Math.random() > sampleRate) {
recordLostEvent('sample_rate', 'event');
return SyncPromise.reject(
return rejectedSyncPromise(
new SentryError(
`Discarding event because it's not included in the random sample (sampling rate = ${sampleRate})`,
),
Expand Down
6 changes: 3 additions & 3 deletions packages/core/src/transports/noop.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { Event, Response, Transport } from '@sentry/types';
import { SyncPromise } from '@sentry/utils';
import { resolvedSyncPromise } from '@sentry/utils';

/** Noop transport */
export class NoopTransport implements Transport {
/**
* @inheritDoc
*/
public sendEvent(_: Event): PromiseLike<Response> {
return SyncPromise.resolve({
return resolvedSyncPromise({
reason: `NoopTransport: Event has been skipped because no Dsn is configured.`,
status: 'skipped',
});
Expand All @@ -17,6 +17,6 @@ export class NoopTransport implements Transport {
* @inheritDoc
*/
public close(_?: number): PromiseLike<boolean> {
return SyncPromise.resolve(true);
return resolvedSyncPromise(true);
}
}
6 changes: 3 additions & 3 deletions packages/core/test/mocks/backend.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Session } from '@sentry/hub';
import { Event, Options, SeverityLevel, Transport } from '@sentry/types';
import { SyncPromise } from '@sentry/utils';
import { resolvedSyncPromise } from '@sentry/utils';

import { BaseBackend } from '../../src/basebackend';

Expand All @@ -24,7 +24,7 @@ export class TestBackend extends BaseBackend<TestOptions> {

// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
public eventFromException(exception: any): PromiseLike<Event> {
return SyncPromise.resolve({
return resolvedSyncPromise({
exception: {
values: [
{
Expand All @@ -39,7 +39,7 @@ export class TestBackend extends BaseBackend<TestOptions> {
}

public eventFromMessage(message: string, level: SeverityLevel = 'info'): PromiseLike<Event> {
return SyncPromise.resolve({ message, level });
return resolvedSyncPromise({ message, level });
}

public sendEvent(event: Event): void {
Expand Down
6 changes: 3 additions & 3 deletions packages/node/src/integrations/linkederrors.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { addGlobalEventProcessor, getCurrentHub } from '@sentry/core';
import { Event, EventHint, Exception, ExtendedError, Integration } from '@sentry/types';
import { isInstanceOf, SyncPromise } from '@sentry/utils';
import { isInstanceOf, resolvedSyncPromise, SyncPromise } from '@sentry/utils';

import { getExceptionFromError } from '../parsers';

Expand Down Expand Up @@ -56,7 +56,7 @@ export class LinkedErrors implements Integration {
*/
private _handler(event: Event, hint?: EventHint): PromiseLike<Event> {
if (!event.exception || !event.exception.values || !hint || !isInstanceOf(hint.originalException, Error)) {
return SyncPromise.resolve(event);
return resolvedSyncPromise(event);
}

return new SyncPromise<Event>(resolve => {
Expand All @@ -78,7 +78,7 @@ export class LinkedErrors implements Integration {
*/
private _walkErrorTree(error: ExtendedError, key: string, stack: Exception[] = []): PromiseLike<Exception[]> {
if (!isInstanceOf(error[key], Error) || stack.length + 1 >= this._limit) {
return SyncPromise.resolve(stack);
return resolvedSyncPromise(stack);
}
return new SyncPromise<Exception[]>((resolve, reject) => {
void getExceptionFromError(error[key])
Expand Down
8 changes: 4 additions & 4 deletions packages/node/src/parsers.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Event, Exception, ExtendedError, StackFrame } from '@sentry/types';
import { addContextToFrame, basename, dirname, SyncPromise } from '@sentry/utils';
import { addContextToFrame, basename, dirname, resolvedSyncPromise, SyncPromise } from '@sentry/utils';
import { readFile } from 'fs';
import { LRUMap } from 'lru_map';

Expand Down Expand Up @@ -71,7 +71,7 @@ function getModule(filename: string, base?: string): string {
function readSourceFiles(filenames: string[]): PromiseLike<{ [key: string]: string | null }> {
// we're relying on filenames being de-duped already
if (filenames.length === 0) {
return SyncPromise.resolve({});
return resolvedSyncPromise({});
}

return new SyncPromise<{
Expand Down Expand Up @@ -176,15 +176,15 @@ export function parseStack(stack: stacktrace.StackFrame[], options?: NodeOptions

// We do an early return if we do not want to fetch context liens
if (linesOfContext <= 0) {
return SyncPromise.resolve(frames);
return resolvedSyncPromise(frames);
}

try {
return addPrePostContext(filesToRead, frames, linesOfContext);
} catch (_) {
// This happens in electron for example where we are not able to read files from asar.
// So it's fine, we recover be just returning all frames without pre/post context.
return SyncPromise.resolve(frames);
return resolvedSyncPromise(frames);
}
}

Expand Down
44 changes: 17 additions & 27 deletions packages/utils/src/promisebuffer.ts
Original file line number Diff line number Diff line change
@@ -1,26 +1,5 @@
import { SentryError } from './error';
import { SyncPromise } from './syncpromise';

function allPromises<U = unknown>(collection: Array<U | PromiseLike<U>>): PromiseLike<U[]> {
return new SyncPromise<U[]>((resolve, reject) => {
if (collection.length === 0) {
resolve(null);
return;
}

let counter = collection.length;
collection.forEach(item => {
void SyncPromise.resolve(item)
.then(() => {
// eslint-disable-next-line no-plusplus
if (--counter === 0) {
resolve(null);
}
})
.then(null, reject);
});
});
}
import { rejectedSyncPromise, resolvedSyncPromise, SyncPromise } from './syncpromise';

export interface PromiseBuffer<T> {
length(): number;
Expand Down Expand Up @@ -66,7 +45,7 @@ export function makePromiseBuffer<T>(limit?: number): PromiseBuffer<T> {
*/
function add(taskProducer: () => PromiseLike<T>): PromiseLike<T> {
if (!isReady()) {
return SyncPromise.reject(new SentryError('Not adding Promise due to buffer limit reached.'));
return rejectedSyncPromise(new SentryError('Not adding Promise due to buffer limit reached.'));
}

// start the task and add its promise to the queue
Expand Down Expand Up @@ -97,7 +76,13 @@ export function makePromiseBuffer<T>(limit?: number): PromiseBuffer<T> {
* `false` otherwise
*/
function drain(timeout?: number): PromiseLike<boolean> {
return new SyncPromise<boolean>(resolve => {
return new SyncPromise<boolean>((resolve, reject) => {
let counter = buffer.length;

if (!counter) {
return resolve(true);
}

// wait for `timeout` ms and then resolve to `false` (if not cancelled first)
const capturedSetTimeout = setTimeout(() => {
if (timeout && timeout > 0) {
Expand All @@ -106,9 +91,14 @@ export function makePromiseBuffer<T>(limit?: number): PromiseBuffer<T> {
}, timeout);

// if all promises resolve in time, cancel the timer and resolve to `true`
void allPromises(buffer).then(() => {
clearTimeout(capturedSetTimeout);
resolve(true);
buffer.forEach(item => {
void resolvedSyncPromise(item).then(() => {
// eslint-disable-next-line no-plusplus
if (!--counter) {
clearTimeout(capturedSetTimeout);
resolve(true);
}
}, reject);
});
});
}
Expand Down
38 changes: 24 additions & 14 deletions packages/utils/src/syncpromise.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,30 @@ const enum States {
REJECTED = 2,
}

/**
* Creates a resolved sync promise.
*
* @param value the value to resolve the promise with
* @returns the resolved sync promise
*/
export function resolvedSyncPromise<T>(value: T | PromiseLike<T>): PromiseLike<T> {
return new SyncPromise(resolve => {
resolve(value);
});
}

/**
* Creates a rejected sync promise.
*
* @param value the value to reject the promise with
* @returns the rejected sync promise
*/
export function rejectedSyncPromise<T = never>(reason?: any): PromiseLike<T> {
return new SyncPromise((_, reject) => {
reject(reason);
});
}

/**
* Thenable class that behaves like a Promise and follows it's interface
* but is not async internally
Expand All @@ -33,20 +57,6 @@ class SyncPromise<T> implements PromiseLike<T> {
}
}

/** JSDoc */
public static resolve<T>(value: T | PromiseLike<T>): PromiseLike<T> {
return new SyncPromise(resolve => {
resolve(value);
});
}

/** JSDoc */
public static reject<T = never>(reason?: any): PromiseLike<T> {
return new SyncPromise((_, reject) => {
reject(reason);
});
}

/** JSDoc */
public then<TResult1 = T, TResult2 = never>(
onfulfilled?: ((value: T) => TResult1 | PromiseLike<TResult1>) | null,
Expand Down
4 changes: 2 additions & 2 deletions packages/utils/test/is.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { resolvedSyncPromise } from '../dist';
import { isDOMError, isDOMException, isError, isErrorEvent, isInstanceOf, isPrimitive, isThenable } from '../src/is';
import { supportsDOMError, supportsDOMException, supportsErrorEvent } from '../src/supports';
import { SyncPromise } from '../src/syncpromise';

class SentryError extends Error {
public name: string;
Expand Down Expand Up @@ -75,7 +75,7 @@ describe('isPrimitive()', () => {
describe('isThenable()', () => {
test('should work as advertised', () => {
expect(isThenable(Promise.resolve(true))).toEqual(true);
expect(isThenable(SyncPromise.resolve(true))).toEqual(true);
expect(isThenable(resolvedSyncPromise(true))).toEqual(true);

expect(isThenable(undefined)).toEqual(false);
expect(isThenable(null)).toEqual(false);
Expand Down
13 changes: 7 additions & 6 deletions packages/utils/test/syncpromise.test.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { rejectedSyncPromise, resolvedSyncPromise } from '../dist';
import { SyncPromise } from '../src/syncpromise';

describe('SyncPromise', () => {
Expand All @@ -17,9 +18,9 @@ describe('SyncPromise', () => {
return new SyncPromise<number>(resolve => {
resolve(42);
})
.then(_ => SyncPromise.resolve('a'))
.then(_ => SyncPromise.resolve(0.1))
.then(_ => SyncPromise.resolve(false))
.then(_ => resolvedSyncPromise('a'))
.then(_ => resolvedSyncPromise(0.1))
.then(_ => resolvedSyncPromise(false))
.then(val => {
expect(val).toBe(false);
});
Expand Down Expand Up @@ -84,7 +85,7 @@ describe('SyncPromise', () => {
return (
c
// @ts-ignore Argument of type 'PromiseLike<string>' is not assignable to parameter of type 'SyncPromise<string>'
.then(val => f(SyncPromise.resolve('x'), val))
.then(val => f(resolvedSyncPromise('x'), val))
.then(val => f(b, val))
// @ts-ignore Argument of type 'SyncPromise<string>' is not assignable to parameter of type 'string'
.then(val => f(a, val))
Expand All @@ -97,7 +98,7 @@ describe('SyncPromise', () => {
test('simple static', () => {
expect.assertions(1);

const p = SyncPromise.resolve(10);
const p = resolvedSyncPromise(10);
return p.then(val => {
expect(val).toBe(10);
});
Expand Down Expand Up @@ -260,7 +261,7 @@ describe('SyncPromise', () => {
})
.then(value => {
expect(value).toEqual(2);
return SyncPromise.reject('wat');
return rejectedSyncPromise('wat');
})
.then(null, reason => {
expect(reason).toBe('wat');
Expand Down