refactor(incremental): set pending result id during execution

This makes id an immutable property of each pending result, reducing the need to track these ids / lazily create them within the publisher. Empty and filtered deferred fragments will lead to "missing" ids and id ordering will now depend on timing/early execution. The id by the specification are opaque strings so that this is not a concern.
graphql · Jun 20, 2024 · 23a1d6c · 23a1d6c
1 parent ab2b7bb
commit 23a1d6c
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 44 deletions.
diff --git a/src/execution/IncrementalPublisher.ts b/src/execution/IncrementalPublisher.ts
@@ -60,12 +60,10 @@ interface SubsequentIncrementalExecutionResultContext {
  */
 class IncrementalPublisher {
   private _context: IncrementalPublisherContext;
-  private _nextId: number;
   private _incrementalGraph: IncrementalGraph;
 
   constructor(context: IncrementalPublisherContext) {
     this._context = context;
-    this._nextId = 0;
     this._incrementalGraph = new IncrementalGraph();
   }
 
@@ -96,10 +94,8 @@ class IncrementalPublisher {
   ): Array<PendingResult> {
     const pendingResults: Array<PendingResult> = [];
     for (const pendingSource of newPending) {
-      const id = String(this._getNextId());
-      pendingSource.id = id;
       const pendingResult: PendingResult = {
-        id,
+        id: pendingSource.id,
         path: pathToArray(pendingSource.path),
       };
       if (pendingSource.label !== undefined) {
@@ -110,10 +106,6 @@ class IncrementalPublisher {
     return pendingResults;
   }
 
-  private _getNextId(): string {
-    return String(this._nextId++);
-  }
-
   private _subscribe(): AsyncGenerator<
     SubsequentIncrementalExecutionResult,
     void,
@@ -231,16 +223,14 @@ class IncrementalPublisher {
     ) {
       for (const deferredFragmentRecord of deferredGroupedFieldSetResult
         .deferredGroupedFieldSetRecord.deferredFragmentRecords) {
-        const id = deferredFragmentRecord.id;
         if (
           !this._incrementalGraph.removeDeferredFragment(deferredFragmentRecord)
         ) {
           // This can occur if multiple deferred grouped field sets error for a fragment.
           continue;
         }
-        invariant(id !== undefined);
         context.completed.push({
-          id,
+          id: deferredFragmentRecord.id,
           errors: deferredGroupedFieldSetResult.errors,
         });
       }
@@ -259,11 +249,10 @@ class IncrementalPublisher {
       if (completion === undefined) {
         continue;
       }
-      const id = deferredFragmentRecord.id;
-      invariant(id !== undefined);
       const incremental = context.incremental;
       const { newPending, reconcilableResults } = completion;
       context.pending.push(...this._pendingSourcesToResults(newPending));
+      const id = deferredFragmentRecord.id;
       for (const reconcilableResult of reconcilableResults) {
         const { bestId, subPath } = this._getBestIdAndSubPath(
           id,
@@ -289,7 +278,6 @@ class IncrementalPublisher {
   ): void {
     const streamRecord = streamItemsResult.streamRecord;
     const id = streamRecord.id;
-    invariant(id !== undefined);
     if (streamItemsResult.errors !== undefined) {
       context.completed.push({
         id,

diff --git a/src/execution/__tests__/defer-test.ts b/src/execution/__tests__/defer-test.ts
@@ -607,12 +607,12 @@ describe('Execute: defer directive', () => {
         data: {
           hero: {},
         },
-        pending: [{ id: '0', path: ['hero'] }],
+        pending: [{ id: '1', path: ['hero'] }],
         hasNext: true,
       },
       {
-        incremental: [{ data: { name: 'Luke' }, id: '0' }],
-        completed: [{ id: '0' }],
+        incremental: [{ data: { name: 'Luke' }, id: '1' }],
+        completed: [{ id: '1' }],
         hasNext: false,
       },
     ]);
@@ -787,8 +787,8 @@ describe('Execute: defer directive', () => {
           hero: {},
         },
         pending: [
-          { id: '0', path: ['hero'], label: 'DeferID' },
-          { id: '1', path: [], label: 'DeferName' },
+          { id: '1', path: ['hero'], label: 'DeferID' },
+          { id: '0', path: [], label: 'DeferName' },
         ],
         hasNext: true,
       },
@@ -798,17 +798,17 @@ describe('Execute: defer directive', () => {
             data: {
               id: '1',
             },
-            id: '0',
+            id: '1',
           },
           {
             data: {
               name: 'Luke',
             },
-            id: '1',
+            id: '0',
             subPath: ['hero'],
           },
         ],
-        completed: [{ id: '0' }, { id: '1' }],
+        completed: [{ id: '1' }, { id: '0' }],
         hasNext: false,
       },
     ]);
@@ -1019,18 +1019,18 @@ describe('Execute: defer directive', () => {
         data: { hero: { friends: [{}, {}, {}] } },
         pending: [
           { id: '0', path: ['hero', 'friends', 0] },
-          { id: '1', path: ['hero', 'friends', 1] },
-          { id: '2', path: ['hero', 'friends', 2] },
+          { id: '4', path: ['hero', 'friends', 1] },
+          { id: '8', path: ['hero', 'friends', 2] },
         ],
         hasNext: true,
       },
       {
         incremental: [
           { data: { id: '2', name: 'Han' }, id: '0' },
-          { data: { id: '3', name: 'Leia' }, id: '1' },
-          { data: { id: '4', name: 'C-3PO' }, id: '2' },
+          { data: { id: '3', name: 'Leia' }, id: '4' },
+          { data: { id: '4', name: 'C-3PO' }, id: '8' },
         ],
-        completed: [{ id: '0' }, { id: '1' }, { id: '2' }],
+        completed: [{ id: '0' }, { id: '4' }, { id: '8' }],
         hasNext: false,
       },
     ]);
@@ -1275,8 +1275,8 @@ describe('Execute: defer directive', () => {
           },
         },
         pending: [
-          { id: '0', path: ['hero', 'nestedObject', 'deeperObject'] },
           { id: '1', path: ['hero', 'nestedObject', 'deeperObject'] },
+          { id: '2', path: ['hero', 'nestedObject', 'deeperObject'] },
         ],
         hasNext: true,
       },
@@ -1286,16 +1286,16 @@ describe('Execute: defer directive', () => {
             data: {
               foo: 'foo',
             },
-            id: '0',
+            id: '1',
           },
           {
             data: {
               bar: 'bar',
             },
-            id: '1',
+            id: '2',
           },
         ],
-        completed: [{ id: '0' }, { id: '1' }],
+        completed: [{ id: '1' }, { id: '2' }],
         hasNext: false,
       },
     ]);
@@ -1351,23 +1351,23 @@ describe('Execute: defer directive', () => {
           },
         },
         pending: [
-          { id: '0', path: ['a', 'b'] },
-          { id: '1', path: [] },
+          { id: '1', path: ['a', 'b'] },
+          { id: '0', path: [] },
         ],
         hasNext: true,
       },
       {
         incremental: [
           {
             data: { e: { f: 'f' } },
-            id: '0',
+            id: '1',
           },
           {
             data: { g: { h: 'h' } },
-            id: '1',
+            id: '0',
           },
         ],
-        completed: [{ id: '0' }, { id: '1' }],
+        completed: [{ id: '1' }, { id: '0' }],
         hasNext: false,
       },
     ]);

diff --git a/src/execution/__tests__/stream-test.ts b/src/execution/__tests__/stream-test.ts
@@ -1978,14 +1978,14 @@ describe('Execute: stream directive', () => {
             nestedFriendList: [],
           },
         },
-        pending: [{ id: '0', path: ['nestedObject', 'nestedFriendList'] }],
+        pending: [{ id: '1', path: ['nestedObject', 'nestedFriendList'] }],
         hasNext: true,
       },
       {
         incremental: [
           {
             items: [{ id: '1', name: 'Luke' }],
-            id: '0',
+            id: '1',
           },
         ],
         hasNext: true,
@@ -1994,13 +1994,13 @@ describe('Execute: stream directive', () => {
         incremental: [
           {
             items: [{ id: '2', name: 'Han' }],
-            id: '0',
+            id: '1',
           },
         ],
         hasNext: true,
       },
       {
-        completed: [{ id: '0' }],
+        completed: [{ id: '1' }],
         hasNext: false,
       },
     ]);

diff --git a/src/execution/execute.ts b/src/execution/execute.ts
@@ -142,6 +142,7 @@ export interface ExecutionContext {
   subscribeFieldResolver: GraphQLFieldResolver<any, any>;
   enableEarlyExecution: boolean;
   errors: Array<GraphQLError> | undefined;
+  nextId: number;
   cancellableStreams: Set<CancellableStreamRecord> | undefined;
 }
 
@@ -299,7 +300,11 @@ function executeOperation(
       const fieldPLan = buildFieldPlan(groupedFieldSet);
       groupedFieldSet = fieldPLan.groupedFieldSet;
       const newGroupedFieldSets = fieldPLan.newGroupedFieldSets;
-      const newDeferMap = addNewDeferredFragments(newDeferUsages, new Map());
+      const newDeferMap = addNewDeferredFragments(
+        exeContext,
+        newDeferUsages,
+        new Map(),
+      );
 
       graphqlWrappedResult = executeRootGroupedFieldSet(
         exeContext,
@@ -505,6 +510,7 @@ export function buildExecutionContext(
     subscribeFieldResolver: subscribeFieldResolver ?? defaultFieldResolver,
     enableEarlyExecution: enableEarlyExecution === true,
     errors: undefined,
+    nextId: 0,
     cancellableStreams: undefined,
   };
 }
@@ -1113,13 +1119,15 @@ async function completeAsyncIteratorValue(
         streamRecord = {
           label: streamUsage.label,
           path,
+          id: String(exeContext.nextId++),
           streamItemQueue,
         };
       } else {
         streamRecord = {
           label: streamUsage.label,
           path,
           streamItemQueue,
+          id: String(exeContext.nextId++),
           earlyReturn: returnFn.bind(asyncIterator),
         };
         if (exeContext.cancellableStreams === undefined) {
@@ -1274,6 +1282,7 @@ function completeIterableValue(
       const syncStreamRecord: StreamRecord = {
         label: streamUsage.label,
         path,
+        id: String(exeContext.nextId++),
         streamItemQueue: buildSyncStreamItemQueue(
           item,
           index,
@@ -1662,6 +1671,7 @@ function invalidReturnTypeError(
  *
  */
 function addNewDeferredFragments(
+  exeContext: ExecutionContext,
   newDeferUsages: ReadonlyArray<DeferUsage>,
   newDeferMap: Map<DeferUsage, DeferredFragmentRecord>,
   path?: Path | undefined,
@@ -1679,6 +1689,7 @@ function addNewDeferredFragments(
     const deferredFragmentRecord = new DeferredFragmentRecord(
       path,
       newDeferUsage.label,
+      String(exeContext.nextId++),
       parent,
     );
 
@@ -1733,6 +1744,7 @@ function collectAndExecuteSubfields(
   groupedFieldSet = subFieldPlan.groupedFieldSet;
   const newGroupedFieldSets = subFieldPlan.newGroupedFieldSets;
   const newDeferMap = addNewDeferredFragments(
+    exeContext,
     newDeferUsages,
     new Map(deferMap),
     path,

diff --git a/src/execution/types.ts b/src/execution/types.ts
@@ -218,7 +218,7 @@ export type SubsequentResultRecord = DeferredFragmentRecord | StreamRecord;
 export class DeferredFragmentRecord {
   path: Path | undefined;
   label: string | undefined;
-  id?: string | undefined;
+  id: string;
   parent: DeferredFragmentRecord | undefined;
   deferredGroupedFieldSetRecords: Set<DeferredGroupedFieldSetRecord>;
   reconcilableResults: Set<ReconcilableDeferredGroupedFieldSetResult>;
@@ -229,10 +229,12 @@ export class DeferredFragmentRecord {
   constructor(
     path: Path | undefined,
     label: string | undefined,
+    id: string,
     parent: DeferredFragmentRecord | undefined,
   ) {
     this.path = path;
     this.label = label;
+    this.id = id;
     this.parent = parent;
     this.deferredGroupedFieldSetRecords = new Set();
     this.reconcilableResults = new Set();
@@ -270,7 +272,7 @@ export type StreamItemRecord = ThunkIncrementalResult<StreamItemResult>;
 export interface StreamRecord {
   path: Path;
   label: string | undefined;
-  id?: string | undefined;
+  id: string;
   streamItemQueue: Array<StreamItemRecord>;
 }