fix(core): return the same children query results if there are no changes (#54392)

pkozlowski-opensource · AndrewKushnir · commit 744cb1e561d9 · 2024-02-12T08:48:29.000-08:00
Assure that the same readonly array corresponding to the children query results is returned for cases where a query is marked as dirty but there were no actual changes to the content of the results array (this can happen if a view is added and removed thus marking queries as dirty but not influencing final results). Fixes #54376 PR Close #54392
diff --git a/packages/core/src/render3/query_reactive.ts b/packages/core/src/render3/query_reactive.ts
@@ -24,6 +24,11 @@ interface QuerySignalNode<T> extends ComputedNode<T|ReadonlyArray<T>> {
   _queryIndex?: number;
   _queryList?: QueryList<T>;
   _dirtyCounter: WritableSignal<number>;
+  /**
+   * Stores the last seen, flattened results for a query. This is to avoid marking the signal result
+   * computed as dirty when there was view manipulation that didn't impact final results.
+   */
+  _flatValue?: T|ReadonlyArray<T>;
 }
 
 /**
@@ -58,6 +63,7 @@ function createQuerySignalFn<V>(firstOnly: boolean, required: boolean) {
   });
   node = signalFn[SIGNAL] as QuerySignalNode<V>;
   node._dirtyCounter = signal(0);
+  node._flatValue = undefined;
 
   if (ngDevMode) {
     signalFn.toString = () => `[Query Signal]`;
@@ -111,5 +117,15 @@ function refreshSignalQuery<V>(node: QuerySignalNode<V>, firstOnly: boolean): V|
 
   queryList.reset(results, unwrapElementRef);
 
-  return firstOnly ? queryList.first : queryList.toArray();
+  if (firstOnly) {
+    return queryList.first;
+  } else {
+    // TODO: remove access to the private _changesDetected field by abstracting / removing usage of
+    // QueryList in the signal-based queries (perf follow-up)
+    const resultChanged = (queryList as any as {_changesDetected: boolean})._changesDetected;
+    if (resultChanged || node._flatValue === undefined) {
+      return node._flatValue = queryList.toArray();
+    }
+    return node._flatValue;
+  }
 }
diff --git a/packages/core/test/acceptance/authoring/signal_queries_spec.ts b/packages/core/test/acceptance/authoring/signal_queries_spec.ts
@@ -364,4 +364,81 @@ describe('queries as signals', () => {
       expect(queryDir.results().length).toBe(0);
     });
   });
+
+  describe('reactivity and performance', () => {
+    it('should not dirty a children query when a list of matches does not change - a view with matches',
+       () => {
+         let recomputeCount = 0;
+
+         @Component({
+           standalone: true,
+           template: `
+          <div #el></div>
+          @if (show) {
+            <div #el></div>
+          }
+        `,
+         })
+         class AppComponent {
+           divEls = viewChildren<ElementRef<HTMLDivElement>>('el');
+           foundElCount = computed(() => {
+             recomputeCount++;
+             return this.divEls().length;
+           });
+           show = false;
+         }
+
+         const fixture = TestBed.createComponent(AppComponent);
+         fixture.detectChanges();
+         expect(fixture.componentInstance.foundElCount()).toBe(1);
+         expect(recomputeCount).toBe(1);
+
+         // trigger view manipulation that should dirty queries but not change the results
+         fixture.componentInstance.show = true;
+         fixture.detectChanges();
+         fixture.componentInstance.show = false;
+         fixture.detectChanges();
+
+         expect(fixture.componentInstance.foundElCount()).toBe(1);
+         expect(recomputeCount).toBe(1);
+       });
+
+    it('should not dirty a children query when a list of matches does not change - a view with another container',
+       () => {
+         let recomputeCount = 0;
+
+         @Component({
+           standalone: true,
+           template: `
+          <div #el></div>
+          @if (show) {
+            <!-- an empty if to create a container -->
+            @if (true) {}
+          }
+        `,
+         })
+         class AppComponent {
+           divEls = viewChildren<ElementRef<HTMLDivElement>>('el');
+           foundElCount = computed(() => {
+             recomputeCount++;
+             return this.divEls().length;
+           });
+           show = false;
+         }
+
+         const fixture = TestBed.createComponent(AppComponent);
+         fixture.detectChanges();
+         expect(fixture.componentInstance.foundElCount()).toBe(1);
+         expect(recomputeCount).toBe(1);
+
+         // trigger view manipulation that should dirty queries but not change the results
+         fixture.componentInstance.show = true;
+         fixture.detectChanges();
+         fixture.componentInstance.show = false;
+         fixture.detectChanges();
+
+         expect(fixture.componentInstance.foundElCount()).toBe(1);
+         expect(recomputeCount).toBe(1);
+       });
+  });
 });
