Skip to content

Commit

Permalink
deduplicate (mask) leaf values that have already been sent
Browse files Browse the repository at this point in the history
causes some additional minor changes to `hasNext` property on some payloads due to modification of returned promises resulting in an extra tick
  • Loading branch information
yaacovCR committed Jan 15, 2023
1 parent d998e79 commit 2e6796f
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 24 deletions.
13 changes: 3 additions & 10 deletions src/execution/__tests__/defer-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -650,19 +650,14 @@ describe('Execute: defer directive', () => {
},
{
data: {
deeperObject: {
bar: 'bar',
baz: 'baz',
},
deeperObject: {},
},
path: ['hero', 'nestedObject'],
},
{
data: {
nestedObject: {
deeperObject: {
bar: 'bar',
},
deeperObject: {},
},
},
path: ['hero'],
Expand Down Expand Up @@ -789,9 +784,7 @@ describe('Execute: defer directive', () => {
data: {
a: {
b: {
e: {
f: 'f',
},
e: {},
},
},
g: {
Expand Down
6 changes: 0 additions & 6 deletions src/execution/__tests__/stream-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1170,9 +1170,6 @@ describe('Execute: stream directive', () => {
],
},
],
hasNext: true,
},
{
hasNext: false,
},
]);
Expand Down Expand Up @@ -1355,9 +1352,6 @@ describe('Execute: stream directive', () => {
],
},
],
hasNext: true,
},
{
hasNext: false,
},
]);
Expand Down
55 changes: 47 additions & 8 deletions src/execution/execute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ export interface ExecutionContext {
errors: Array<GraphQLError>;
subsequentPayloads: Set<AsyncPayloadRecord>;
branches: WeakMap<GroupedFieldSet, Set<Path | undefined>>;
leaves: WeakMap<GroupedFieldSet, Set<Path | undefined>>;
}

/**
Expand Down Expand Up @@ -506,6 +507,7 @@ export function buildExecutionContext(
addPath: createPathFactory(),
subsequentPayloads: new Set(),
branches: new WeakMap(),
leaves: new WeakMap(),
errors: [],
};
}
Expand All @@ -520,6 +522,7 @@ function buildPerEventExecutionContext(
addPath: createPathFactory(),
subsequentPayloads: new Set(),
branches: new WeakMap(),
leaves: new WeakMap(),
errors: [],
};
}
Expand Down Expand Up @@ -639,7 +642,9 @@ function executeFieldsSerially(

const returnType = fieldDef.type;

if (!shouldExecute(fieldGroup, returnType)) {
const isLeaf = isLeafType(getNamedType(returnType));

if (!shouldExecute(fieldGroup, isLeaf)) {
return results;
}
const result = executeField(
Expand All @@ -666,10 +671,10 @@ function executeFieldsSerially(

function shouldExecute(
fieldGroup: FieldGroup,
returnType: GraphQLOutputType,
isLeaf: boolean,
deferDepth?: number | undefined,
): boolean {
if (deferDepth === undefined || !isLeafType(getNamedType(returnType))) {
if (deferDepth === undefined || !isLeaf) {
return fieldGroup.some(
({ deferDepth: fieldDeferDepth }) => fieldDeferDepth === deferDepth,
);
Expand Down Expand Up @@ -702,6 +707,8 @@ function executeFields(
const results = Object.create(null);
let containsPromise = false;

const shouldMask = Object.create(null);

try {
for (const [responseName, fieldGroup] of groupedFieldSet) {
const fieldPath = exeContext.addPath(path, responseName, parentType.name);
Expand All @@ -714,9 +721,27 @@ function executeFields(

const returnType = fieldDef.type;

if (
shouldExecute(fieldGroup, returnType, asyncPayloadRecord?.deferDepth)
) {
const isLeaf = isLeafType(getNamedType(returnType));

if (shouldExecute(fieldGroup, isLeaf, asyncPayloadRecord?.deferDepth)) {
if (
asyncPayloadRecord !== undefined &&
isLeafType(getNamedType(returnType))
) {
shouldMask[responseName] = () => {
const set = exeContext.leaves.get(groupedFieldSet);
if (set === undefined) {
exeContext.leaves.set(groupedFieldSet, new Set([fieldPath]));
return false;
}
if (set.has(fieldPath)) {
return true;
}
set.add(fieldPath);
return false;
};
}

const result = executeField(
exeContext,
parentType,
Expand Down Expand Up @@ -748,13 +773,27 @@ function executeFields(

// If there are no promises, we can just return the object
if (!containsPromise) {
return results;
return asyncPayloadRecord === undefined
? results
: new Proxy(results, {
ownKeys: (target) =>
Reflect.ownKeys(target).filter((key) => !shouldMask[key]?.()),
});
}

// Otherwise, results is a map from field name to the result of resolving that
// field, which is possibly a promise. Return a promise that will return this
// same map, but with any promises replaced with the values they resolved to.
return promiseForObject(results);
const promisedResult = promiseForObject(results);
return asyncPayloadRecord === undefined
? promisedResult
: promisedResult.then(
(resolved) =>
new Proxy(resolved, {
ownKeys: (target) =>
Reflect.ownKeys(target).filter((key) => !shouldMask[key]?.()),
}),
);
}

function toNodes(fieldGroup: FieldGroup): ReadonlyArray<FieldNode> {
Expand Down

0 comments on commit 2e6796f

Please sign in to comment.