Skip to content

Commit 37a6299

Browse files
authored
fix(execution): convert all promise-like results to promises (#4672)
previously, most pathways converted, but not all in particular, pathways branching off of `isTypeOf(...)` and `resolveType(...)` as well as `createSourceEventStream` could return non-native promises even though we advertised `PromiseOrValue`. There is also a micro-benchmark perf improvement even in sync paths, presumably because V8 can make a few more assumptions. <img width="907" height="827" alt="image" src="https://github.com/user-attachments/assets/8285c2de-f872-47ec-88bf-ee046e0aa063" />
1 parent afbe436 commit 37a6299

8 files changed

Lines changed: 36 additions & 36 deletions

File tree

src/execution/AsyncWorkTracker.ts

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
import { isPromise } from '../jsutils/isPromise.js';
2-
import type { PromiseOrValue } from '../jsutils/PromiseOrValue.js';
1+
import { isPromiseLike } from '../jsutils/isPromise.js';
32

43
/** @internal */
54
export class AsyncWorkTracker {
@@ -9,9 +8,9 @@ export class AsyncWorkTracker {
98
this.pendingAsyncWork = new Set<Promise<void>>();
109
}
1110

12-
add(promise: Promise<unknown>): void {
11+
add(promiseLike: PromiseLike<unknown>): void {
1312
const pendingAsyncWork = this.pendingAsyncWork;
14-
const promiseToSettle = promise.then(
13+
const promiseToSettle = Promise.resolve(promiseLike).then(
1514
() => {
1615
pendingAsyncWork.delete(promiseToSettle);
1716
},
@@ -22,9 +21,9 @@ export class AsyncWorkTracker {
2221
pendingAsyncWork.add(promiseToSettle);
2322
}
2423

25-
addValues(values: ReadonlyArray<PromiseOrValue<unknown>>): void {
24+
addValues(values: ReadonlyArray<unknown>): void {
2625
for (const value of values) {
27-
if (isPromise(value)) {
26+
if (isPromiseLike(value)) {
2827
this.add(value);
2928
}
3029
}
@@ -40,7 +39,7 @@ export class AsyncWorkTracker {
4039
}
4140

4241
promiseAllTrackOnReject<T>(
43-
values: ReadonlyArray<PromiseOrValue<T>>,
42+
values: ReadonlyArray<PromiseLike<T> | T>,
4443
): Promise<Array<T>> {
4544
const promise = Promise.all(values);
4645
promise.then(undefined, () => {

src/execution/Executor.ts

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { inspect } from '../jsutils/inspect.js';
22
import { invariant } from '../jsutils/invariant.js';
33
import { isAsyncIterable } from '../jsutils/isAsyncIterable.js';
44
import { isIterableObject } from '../jsutils/isIterableObject.js';
5-
import { isPromise } from '../jsutils/isPromise.js';
5+
import { isPromise, isPromiseLike } from '../jsutils/isPromise.js';
66
import { memoize2 } from '../jsutils/memoize2.js';
77
import { memoize3 } from '../jsutils/memoize3.js';
88
import type { ObjMap } from '../jsutils/ObjMap.js';
@@ -610,7 +610,7 @@ export class Executor<
610610
// used to represent an authenticated user, or request-specific caches.
611611
const result = resolveFn(source, args, contextValue, info);
612612

613-
if (isPromise(result)) {
613+
if (isPromiseLike(result)) {
614614
return this.completePromisedValue(
615615
returnType,
616616
fieldDetailsList,
@@ -790,7 +790,7 @@ export class Executor<
790790
fieldDetailsList: FieldDetailsList,
791791
info: GraphQLResolveInfo,
792792
path: Path,
793-
result: Promise<unknown>,
793+
result: PromiseLike<unknown>,
794794
positionContext: TPositionContext | undefined,
795795
): Promise<unknown> {
796796
try {
@@ -1052,7 +1052,7 @@ export class Executor<
10521052
itemPath: Path,
10531053
positionContext: TPositionContext | undefined,
10541054
): boolean {
1055-
if (isPromise(item)) {
1055+
if (isPromiseLike(item)) {
10561056
completedResults.push(
10571057
this.completePromisedListItemValue(
10581058
item,
@@ -1130,7 +1130,7 @@ export class Executor<
11301130
}
11311131

11321132
async completePromisedListItemValue(
1133-
item: Promise<unknown>,
1133+
item: PromiseLike<unknown>,
11341134
itemType: GraphQLOutputType,
11351135
fieldDetailsList: FieldDetailsList,
11361136
info: GraphQLResolveInfo,
@@ -1193,8 +1193,8 @@ export class Executor<
11931193
returnType.resolveType ?? validatedExecutionArgs.typeResolver;
11941194
const runtimeType = resolveTypeFn(result, contextValue, info, returnType);
11951195

1196-
if (isPromise(runtimeType)) {
1197-
return runtimeType.then((resolvedRuntimeType) => {
1196+
if (isPromiseLike(runtimeType)) {
1197+
return Promise.resolve(runtimeType).then((resolvedRuntimeType) => {
11981198
if (this.aborted) {
11991199
throw new Error('Aborted!');
12001200
}
@@ -1303,8 +1303,8 @@ export class Executor<
13031303
info,
13041304
);
13051305

1306-
if (isPromise(isTypeOf)) {
1307-
return isTypeOf.then((resolvedIsTypeOf) => {
1306+
if (isPromiseLike(isTypeOf)) {
1307+
return Promise.resolve(isTypeOf).then((resolvedIsTypeOf) => {
13081308
if (this.aborted) {
13091309
throw new Error('Aborted!');
13101310
}

src/execution/collectIteratorPromises.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
1-
import { isPromise } from '../jsutils/isPromise.js';
1+
import { isPromiseLike } from '../jsutils/isPromise.js';
22

33
/**
44
* Drain a sync iterator after abrupt completion so later promise rejections
55
* can be observed before they become unhandled.
66
*/
77
export function collectIteratorPromises(
88
iterator: Iterator<unknown>,
9-
): Array<Promise<unknown>> {
9+
): Array<unknown> {
1010
const promises = [];
1111
try {
1212
while (true) {
1313
const iteration = iterator.next();
1414
if (iteration.done) {
1515
return promises;
1616
}
17-
if (isPromise(iteration.value)) {
17+
if (isPromiseLike(iteration.value)) {
1818
promises.push(iteration.value);
1919
}
2020
}

src/execution/createSharedExecutionContext.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
import type { PromiseOrValue } from '../jsutils/PromiseOrValue.js';
2-
31
import type { GraphQLResolveInfoHelpers } from '../type/index.js';
42

53
import { AsyncWorkTracker } from './AsyncWorkTracker.js';
@@ -10,7 +8,7 @@ export interface SharedExecutionContext {
108
getAbortSignal: () => AbortSignal | undefined;
119
getAsyncHelpers: () => GraphQLResolveInfoHelpers;
1210
promiseAll: <T>(
13-
values: ReadonlyArray<PromiseOrValue<T>>,
11+
values: ReadonlyArray<PromiseLike<T> | T>,
1412
) => Promise<Array<T>>;
1513
}
1614

@@ -21,7 +19,7 @@ export function createSharedExecutionContext(
2119
let resolveInfoHelpers: GraphQLResolveInfoHelpers | undefined;
2220

2321
const promiseAll = <T>(
24-
values: ReadonlyArray<PromiseOrValue<T>>,
22+
values: ReadonlyArray<PromiseLike<T> | T>,
2523
): Promise<Array<T>> => asyncWorkTracker.promiseAllTrackOnReject(values);
2624

2725
const getAsyncHelpers = (): GraphQLResolveInfoHelpers =>

src/execution/execute.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { inspect } from '../jsutils/inspect.js';
22
import { isAsyncIterable } from '../jsutils/isAsyncIterable.js';
33
import { isObjectLike } from '../jsutils/isObjectLike.js';
4-
import { isPromise } from '../jsutils/isPromise.js';
4+
import { isPromise, isPromiseLike } from '../jsutils/isPromise.js';
55
import type { Maybe } from '../jsutils/Maybe.js';
66
import type { ObjMap } from '../jsutils/ObjMap.js';
77
import type { Path } from '../jsutils/Path.js';
@@ -445,7 +445,7 @@ export const defaultTypeResolver: GraphQLTypeResolver<unknown, unknown> =
445445

446446
// Otherwise, test each possible type.
447447
const possibleTypes = info.schema.getPossibleTypes(abstractType);
448-
const promisedIsTypeOfResults: Array<Promise<boolean>> = [];
448+
const promisedIsTypeOfResults: Array<PromiseLike<boolean>> = [];
449449

450450
try {
451451
for (let i = 0; i < possibleTypes.length; i++) {
@@ -454,7 +454,7 @@ export const defaultTypeResolver: GraphQLTypeResolver<unknown, unknown> =
454454
if (type.isTypeOf) {
455455
const isTypeOfResult = type.isTypeOf(value, contextValue, info);
456456

457-
if (isPromise(isTypeOfResult)) {
457+
if (isPromiseLike(isTypeOfResult)) {
458458
promisedIsTypeOfResults[i] = isTypeOfResult;
459459
} else if (isTypeOfResult) {
460460
if (promisedIsTypeOfResults.length) {
@@ -637,10 +637,11 @@ function executeSubscription(
637637
// used to represent an authenticated user, or request-specific caches.
638638
const result = resolveFn(rootValue, args, contextValue, info);
639639

640-
if (isPromise(result)) {
640+
if (isPromiseLike(result)) {
641+
const promisedResult = Promise.resolve(result);
641642
const promise = externalAbortSignal
642-
? cancellablePromise(result, externalAbortSignal)
643-
: result;
643+
? cancellablePromise(promisedResult, externalAbortSignal)
644+
: promisedResult;
644645
return promise
645646
.then(assertEventStream)
646647
.then(undefined, (error: unknown) => {

src/execution/incremental/IncrementalExecutor.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/* eslint-disable max-params */
22
import { invariant } from '../../jsutils/invariant.js';
3-
import { isPromise } from '../../jsutils/isPromise.js';
3+
import { isPromise, isPromiseLike } from '../../jsutils/isPromise.js';
44
import { memoize1 } from '../../jsutils/memoize1.js';
55
import { memoize2 } from '../../jsutils/memoize2.js';
66
import type { ObjMap } from '../../jsutils/ObjMap.js';
@@ -841,7 +841,7 @@ export class IncrementalExecutor<
841841
info: GraphQLResolveInfo,
842842
itemType: GraphQLOutputType,
843843
): PromiseOrValue<StreamItemResult> {
844-
if (isPromise(item)) {
844+
if (isPromiseLike(item)) {
845845
return this.completePromisedValue(
846846
itemType,
847847
fieldDetailsList,

src/jsutils/isPromise.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
1+
export function isPromise(value: unknown): value is Promise<unknown> {
2+
return value instanceof Promise;
3+
}
4+
15
/**
26
* Returns true if the value acts like a Promise, i.e. has a "then" function,
37
* otherwise returns false.
48
*/
5-
export function isPromise(value: any): value is Promise<unknown> {
9+
export function isPromiseLike(value: any): value is PromiseLike<unknown> {
610
return typeof value?.then === 'function';
711
}

src/type/definition.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1069,11 +1069,9 @@ export interface GraphQLResolveInfoHelpers {
10691069
* ]);
10701070
*/
10711071
readonly promiseAll: <T>(
1072-
values: ReadonlyArray<PromiseOrValue<T>>,
1072+
values: ReadonlyArray<PromiseLike<T> | T>,
10731073
) => Promise<Array<T>>;
1074-
readonly track: (
1075-
maybePromises: ReadonlyArray<PromiseOrValue<unknown>>,
1076-
) => void;
1074+
readonly track: (maybePromises: ReadonlyArray<unknown>) => void;
10771075
}
10781076

10791077
export interface GraphQLResolveInfo {

0 commit comments

Comments
 (0)