Skip to content

Commit

Permalink
fix(incremental): skip all empty subsequent results (#3993)
Browse files Browse the repository at this point in the history
The publish method checks to see if a subsequent result is empty; this
same logic should be employed to suppress pending notices for empty
records.

This has already been achieved for subsequent results that are children
of the initial result, as we generated the pending notices from the list
of initially published records.

For subsequent results that are children of other subsequent results, we
previously generated the pending notice prior to actually publishing.

This change integrates the logic: the publishing method itself returns a
pending notice as required. This results in a bug-fix for subsequent
records of other subsequent records as well as a reduction of code for
subsequent results to the initial result.
  • Loading branch information
yaacovCR committed Mar 19, 2024
1 parent 9c90a23 commit ef478a2
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 52 deletions.
85 changes: 44 additions & 41 deletions src/execution/IncrementalPublisher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -301,25 +301,11 @@ export class IncrementalPublisher {
initialResultRecord: InitialResultRecord,
data: ObjMap<unknown> | null,
): ExecutionResult | ExperimentalIncrementalExecutionResults {
for (const child of initialResultRecord.children) {
if (child.filtered) {
continue;
}
this._publish(child);
}
const pendingSources = this._publish(initialResultRecord.children);

const errors = initialResultRecord.errors;
const initialResult = errors.length === 0 ? { data } : { errors, data };
const pending = this._pending;
if (pending.size > 0) {
const pendingSources = new Set<DeferredFragmentRecord | StreamRecord>();
for (const subsequentResultRecord of pending) {
const pendingSource = isStreamItemsRecord(subsequentResultRecord)
? subsequentResultRecord.streamRecord
: subsequentResultRecord;
pendingSources.add(pendingSource);
}

if (pendingSources.size > 0) {
return {
initialResult: {
...initialResult,
Expand Down Expand Up @@ -538,18 +524,7 @@ export class IncrementalPublisher {
const incrementalResults: Array<IncrementalResult> = [];
const completedResults: Array<CompletedResult> = [];
for (const subsequentResultRecord of completedRecords) {
for (const child of subsequentResultRecord.children) {
if (child.filtered) {
continue;
}
const pendingSource = isStreamItemsRecord(child)
? child.streamRecord
: child;
if (!pendingSource.pendingSent) {
newPendingSources.add(pendingSource);
}
this._publish(child);
}
this._publish(subsequentResultRecord.children, newPendingSources);
if (isStreamItemsRecord(subsequentResultRecord)) {
if (subsequentResultRecord.isFinalRecord) {
newPendingSources.delete(subsequentResultRecord.streamRecord);
Expand Down Expand Up @@ -613,6 +588,8 @@ export class IncrementalPublisher {
let idWithLongestPath: string | undefined;
for (const deferredFragmentRecord of deferredFragmentRecords) {
const id = deferredFragmentRecord.id;
// TODO: add test
/* c8 ignore next 3 */
if (id === undefined) {
continue;
}
Expand Down Expand Up @@ -655,25 +632,51 @@ export class IncrementalPublisher {
return result;
}

private _publish(subsequentResultRecord: SubsequentResultRecord): void {
if (isStreamItemsRecord(subsequentResultRecord)) {
if (subsequentResultRecord.isCompleted) {
private _publish(
subsequentResultRecords: ReadonlySet<SubsequentResultRecord>,
pendingSources = new Set<DeferredFragmentRecord | StreamRecord>(),
): Set<DeferredFragmentRecord | StreamRecord> {
const emptyRecords: Array<SubsequentResultRecord> = [];

for (const subsequentResultRecord of subsequentResultRecords) {
if (subsequentResultRecord.filtered) {
continue;
}
if (isStreamItemsRecord(subsequentResultRecord)) {
if (subsequentResultRecord.isCompleted) {
this._push(subsequentResultRecord);
} else {
this._introduce(subsequentResultRecord);
}

const stream = subsequentResultRecord.streamRecord;
if (!stream.pendingSent) {
pendingSources.add(stream);
}
continue;
}

if (subsequentResultRecord._pending.size > 0) {
this._introduce(subsequentResultRecord);
} else if (
subsequentResultRecord.deferredGroupedFieldSetRecords.size === 0
) {
emptyRecords.push(subsequentResultRecord);
continue;
} else {
this._push(subsequentResultRecord);
return;
}

this._introduce(subsequentResultRecord);
return;
if (!subsequentResultRecord.pendingSent) {
pendingSources.add(subsequentResultRecord);
}
}

if (subsequentResultRecord._pending.size > 0) {
this._introduce(subsequentResultRecord);
} else if (
subsequentResultRecord.deferredGroupedFieldSetRecords.size > 0 ||
subsequentResultRecord.children.size > 0
) {
this._push(subsequentResultRecord);
for (const emptyRecord of emptyRecords) {
this._publish(emptyRecord.children, pendingSources);
}

return pendingSources;
}

private _getChildren(
Expand Down
15 changes: 4 additions & 11 deletions src/execution/__tests__/defer-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1023,34 +1023,27 @@ describe('Execute: defer directive', () => {
},
},
pending: [
{ id: '0', path: ['hero'] },
{ id: '0', path: ['hero', 'nestedObject', 'deeperObject'] },
{ id: '1', path: ['hero', 'nestedObject', 'deeperObject'] },
],
hasNext: true,
},
{
pending: [{ id: '2', path: ['hero', 'nestedObject', 'deeperObject'] }],
incremental: [
{
data: {
foo: 'foo',
},
id: '1',
id: '0',
},
],
completed: [{ id: '0' }, { id: '1' }],
hasNext: true,
},
{
incremental: [
{
data: {
bar: 'bar',
},
id: '2',
id: '1',
},
],
completed: [{ id: '2' }],
completed: [{ id: '0' }, { id: '1' }],
hasNext: false,
},
]);
Expand Down

0 comments on commit ef478a2

Please sign in to comment.