fix(core): QueryList does not fire changes if the underlying list d…

…id not change.

Previous implementation would fire changes `QueryList.changes.subscribe`
even if underlying list did not change. Such situation can arise if a
`LView` is inserted or removed, causing `QueryList` to recompute, but
the recompute results in the same exact `QueryList` result. In such
a case we don't want to fire a change event (as there is no change.)

goldens/public-api/core/core.d.ts

@@ -746,7 +746,9 @@ export declare abstract class Query {
 
 export declare class QueryList<T> implements Iterable<T> {
     [Symbol.iterator]: () => Iterator<T>;
-    readonly changes: Observable<any>;
+    get changes(): Observable<any>;
+    /** @deprecated */ get changesDeprecated(): Observable<QueryList<T>>;
+    get changesStrict(): Observable<QueryList<T>>;
     readonly dirty = true;
     readonly first: T;
     readonly last: T;
@@ -758,9 +760,9 @@ export declare class QueryList<T> implements Iterable<T> {
     forEach(fn: (item: T, index: number, array: T[]) => void): void;
     get(index: number): T | undefined;
     map<U>(fn: (item: T, index: number, array: T[]) => U): U[];
-    notifyOnChanges(): void;
+    notifyOnChanges(changesDetected: boolean): void;
     reduce<U>(fn: (prevValue: U, curValue: T, curIndex: number, array: T[]) => U, init: U): U;
-    reset(resultsTree: Array<T | any[]>): void;
+    reset(resultsTree: Array<T | any[]>): boolean;
     setDirty(): void;
     some(fn: (value: T, index: number, array: T[]) => boolean): boolean;
     toArray(): T[];

packages/core/src/linker/query_list.ts

@@ -9,7 +9,7 @@
 import {Observable} from 'rxjs';
 
 import {EventEmitter} from '../event_emitter';
-import {flatten} from '../util/array_utils';
+import {flattenIntoExisting} from '../util/array_utils';
 import {getSymbolIterator} from '../util/symbol';
 
 function symbolIterator<T>(this: QueryList<T>): Iterator<T> {
@@ -45,13 +45,47 @@ function symbolIterator<T>(this: QueryList<T>): Iterator<T> {
 export class QueryList<T> implements Iterable<T> {
   public readonly dirty = true;
   private _results: Array<T> = [];
-  public readonly changes: Observable<any> = new EventEmitter();
+  private _changesDeprecated: EventEmitter<QueryList<T>>|null = null;
+  private _changesStrict: EventEmitter<QueryList<T>>|null = null;
 
   readonly length: number = 0;
-  // TODO(issue/24571): remove '!'.
-  readonly first!: T;
-  // TODO(issue/24571): remove '!'.
-  readonly last!: T;
+  readonly first: T = undefined!;
+  readonly last: T = undefined!;
+
+  /**
+   * Returns `Observable` of `QueryList` notifying the subscriber of changes.
+   *
+   * NOTE: This currently points to `changesDeprecated` which incorrectly notifies of changes even
+   * if no changes to `QueryList` have occurred. (It fires more often than it needs to.)
+   * The implementation will change to point `changesStrict` starting with v12 (or later.)
+   */
+  get changes(): Observable<any> {
+    return this.changesDeprecated;
+  }
+
+  /**
+   * Returns `Observable` of `QueryList` notifying the subscriber of changes.
+   *
+   * NOTE: This deprecated implementation incorrectly notifies of changes even
+   * if no changes to `QueryList` have occurred. (It fires more often than it needs to.)
+   * The implementation is here for convenience during migration and will be remove in future
+   * versions. Please change your code to use `changesStrict`.
+   *
+   * @deprecated Since v11.1, use `changesStrict` instead.
+   */
+  get changesDeprecated(): Observable<QueryList<T>> {
+    return this._changesDeprecated || (this._changesDeprecated = new EventEmitter());
+  }
+
+  /**
+   * Returns `Observable` of `QueryList` notifying the subscriber of changes.
+   *
+   * NOTE: This is a proper implementation which only fires if there are underlying changes in the
+   * `Observable` (the previous implementation would fire even if no changes were detected.)
+   */
+  get changesStrict(): Observable<QueryList<T>> {
+    return this._changesStrict || (this._changesStrict = new EventEmitter());
+  }
 
   constructor() {
     // This function should be declared on the prototype, but doing so there will cause the class
@@ -135,28 +169,36 @@ export class QueryList<T> implements Iterable<T> {
    * occurs.
    *
    * @param resultsTree The query results to store
+   * @returns `true` only if the new `resultTree` resulted in a different query set.
+   * @internal
    */
-  reset(resultsTree: Array<T|any[]>): void {
-    this._results = flatten(resultsTree);
+  reset(resultsTree: Array<T|any[]>): boolean {
     (this as {dirty: boolean}).dirty = false;
-    (this as {length: number}).length = this._results.length;
-    (this as {last: T}).last = this._results[this.length - 1];
-    (this as {first: T}).first = this._results[0];
+    if (flattenIntoExisting(resultsTree, this._results)) {
+      (this as {length: number}).length = this._results.length;
+      (this as {last: T}).last = this._results[this.length - 1];
+      (this as {first: T}).first = this._results[0];
+      return true;
+    } else {
+      return false;
+    }
   }
 
   /**
    * Triggers a change event by emitting on the `changes` {@link EventEmitter}.
+   * @internal
    */
-  notifyOnChanges(): void {
-    (this.changes as EventEmitter<any>).emit(this);
+  notifyOnChanges(changesDetected: boolean): void {
+    if (this._changesDeprecated) this._changesDeprecated.emit(this);
+    if (this._changesStrict && changesDetected) this._changesStrict.emit(this);
   }
 
-  /** internal */
+  /** @internal */
   setDirty() {
     (this as {dirty: boolean}).dirty = true;
   }
 
-  /** internal */
+  /** @internal */
   destroy(): void {
     (this.changes as EventEmitter<any>).complete();
     (this.changes as EventEmitter<any>).unsubscribe();

packages/core/src/render3/query.ts

@@ -434,8 +434,7 @@ export function ɵɵqueryRefresh(queryList: QueryList<any>): boolean {
       const result = tQuery.crossesNgTemplate ?
           collectQueryResults(tView, lView, queryIndex, []) :
           materializeViewResults(tView, lView, tQuery, queryIndex);
-      queryList.reset(result);
-      queryList.notifyOnChanges();
+      queryList.notifyOnChanges(queryList.reset(result));
     }
     return true;
   }

packages/core/src/util/array_utils.ts

@@ -24,22 +24,63 @@ export function addAllToArray(items: any[], arr: any[]) {
  * Flattens an array.
  */
 export function flatten(list: any[], dst?: any[]): any[] {
-  if (dst === undefined) dst = list;
-  for (let i = 0; i < list.length; i++) {
-    let item = list[i];
+  if (dst === undefined) dst = [];
+  flattenIntoExisting(list, dst);
+  return dst;
+}
+
+/**
+ * Flatten `src` array into `dst` array and computes if differences were detected.
+ *
+ * @param src Source array which can be hierarchical (array within array)
+ * @param dst Destination array which can have existing values.
+ * @returns `true` if `dst` array was mutated in order to represent flatten version of `src`.
+ */
+export function flattenIntoExisting(src: unknown[], dst: unknown[]): boolean {
+  let dstIdx = flattenIntoExistingRecursive(src, dst, 0);
+  let differenceDetected = dstIdx < 0;
+  dstIdx = differenceDetected ? ~dstIdx : dstIdx;
+  if (dstIdx < dst.length) {
+    // Fewer items than were previously in `dst`, so trim the array.
+    dst.length = dstIdx;
+    differenceDetected = true;
+  }
+  return differenceDetected;
+}
+
+/**
+ * Flatten `src` array into `dst` array and computes differences and location where diffing ended.
+ * @param src Source array which can be hierarchical (array within array)
+ * @param dst Destination array which can have existing values.
+ * @param dstIdx Index into `dst` where the diffing should start.
+ * @returns Location where the diffing ended in `dst` array. If `<0` then `~value` is `dstIdx` and a
+ *     `dst` array was mutated (change detected).
+ */
+function flattenIntoExistingRecursive(src: unknown[], dst: unknown[], dstIdx: number): number {
+  let differenceDetected = false;
+  for (let i = 0; i < src.length; i++) {
+    let item = src[i];
     if (Array.isArray(item)) {
-      // we need to inline it.
-      if (dst === list) {
-        // Our assumption that the list was already flat was wrong and
-        // we need to clone flat since we need to write to it.
-        dst = list.slice(0, i);
+      dstIdx = flattenIntoExistingRecursive(item, dst, dstIdx);
+      if (dstIdx < 0) {
+        // we detected a difference internally
+        differenceDetected = true;
+        dstIdx = ~dstIdx;
       }
-      flatten(item, dst);
-    } else if (dst !== list) {
-      dst.push(item);
+    } else {
+      if (dstIdx >= dst.length) {
+        // Destination is missing the item
+        dst.push(item);
+        differenceDetected = true;
+      } else if (item !== dst[dstIdx]) {
+        // The destination has the wrong item
+        dst[dstIdx] = item;
+        differenceDetected = true;
+      }
+      dstIdx++;
     }
   }
-  return dst;
+  return differenceDetected ? ~dstIdx : dstIdx;
 }
 
 export function deepForEach<T>(input: (T|any[])[], fn: (value: T) => void): void {

packages/core/src/view/query.ts

@@ -107,7 +107,7 @@ export function checkAndUpdateQuery(view: ViewData, nodeDef: NodeDef) {
     newValues = calcQueryValues(view, 0, view.def.nodes.length - 1, nodeDef.query!, []);
     directiveInstance = view.component;
   }
-  queryList.reset(newValues);
+  const queryChangesDetected = queryList.reset(newValues);
   const bindings = nodeDef.query!.bindings;
   let notify = false;
   for (let i = 0; i < bindings.length; i++) {
@@ -125,7 +125,7 @@ export function checkAndUpdateQuery(view: ViewData, nodeDef: NodeDef) {
     directiveInstance[binding.propName] = boundValue;
   }
   if (notify) {
-    queryList.notifyOnChanges();
+    queryList.notifyOnChanges(queryChangesDetected);
   }
 }
 

packages/core/test/acceptance/query_spec.ts

@@ -1123,6 +1123,53 @@ describe('query logic', () => {
       fixture.detectChanges();
       expect(changes).toBe(1);
     });
+
+    it('should only fire if the content of the query changes', () => {
+      // When views are inserted/removed the content query need to be recomputed.
+      // Recomputing query may result in no changes to the query (the item added/remove was not part
+      // of the query). This tests asserts that the query does not fire when no changes occur.
+      TestBed.configureTestingModule(
+          {declarations: [QueryCompWithNoChangesParent, QueryCompWithNoChanges]});
+      const fixture = TestBed.createComponent(QueryCompWithNoChanges);
+      let changesDeprecated = 0;
+      let changesStrict = 0;
+      const componentInstance = fixture.componentInstance.queryComp;
+      fixture.detectChanges();
+
+      componentInstance.foos.changesDeprecated.subscribe((value: any) => {
+        // subscribe to the changes and record when changes occur.
+        changesDeprecated += 1;
+      });
+      componentInstance.foos.changesStrict.subscribe((value: any) => {
+        // subscribe to the changes and record when changes occur.
+        changesStrict += 1;
+      });
+
+      // First verify that the subscription is working.
+      fixture.componentInstance.innerShowing = false;
+      fixture.detectChanges();
+      expect(changesDeprecated).toBe(1);  // We detected a change
+      expect(changesStrict).toBe(1);      // We detected a change
+      expect(componentInstance.foos.toArray().length).toEqual(1);
+
+
+      // now verify that removing a view does not needlessly fire subscription
+      fixture.componentInstance.showing = false;
+      fixture.detectChanges();
+      expect(changesDeprecated).toBe(2);
+      expect(changesStrict).toBe(1);  // We detected a change
+      expect(componentInstance.foos.toArray().length).toEqual(1);
+
+      // now verify that adding a view does not needlessly fire subscription
+      fixture.componentInstance.showing = true;
+      fixture.detectChanges();
+      expect(changesDeprecated).toBe(3);
+      expect(changesStrict).toBe(1);  // We detected a change
+      // Note: even though the `showing` is `true` and the second `<div>` is displayed, the child
+      // element of that <div> is hidden because the `innerShowing` flag is still `false`, so we
+      // expect only one element to be present in the `foos` array.
+      expect(componentInstance.foos.toArray().length).toEqual(1);
+    });
   });
 
   describe('view boundaries', () => {
@@ -1219,7 +1266,7 @@ describe('query logic', () => {
        * - detect the situation where the indexes are the same and do no processing in such case.
        *
        * This tests asserts on the implementation choices done by the VE (detach and insert) so we
-       * can replicate the same behaviour in ivy.
+       * can replicate the same behavior in ivy.
        */
       it('should notify on changes when a given view is removed and re-inserted at the same index',
          () => {
@@ -1232,13 +1279,15 @@ describe('query logic', () => {
            })
            class TestComponent implements AfterViewInit {
              queryListNotificationCounter = 0;
+             queryListStrictNotificationCounter = 0;
 
              @ViewChild(ViewContainerManipulatorDirective) vc!: ViewContainerManipulatorDirective;
              @ViewChild('tpl') tpl!: TemplateRef<any>;
              @ViewChildren('foo') query!: QueryList<any>;
 
              ngAfterViewInit() {
-               this.query.changes.subscribe(() => this.queryListNotificationCounter++);
+               this.query.changesDeprecated.subscribe(() => this.queryListNotificationCounter++);
+               this.query.changesStrict.subscribe(() => this.queryListStrictNotificationCounter++);
              }
            }
 
@@ -1254,11 +1303,13 @@ describe('query logic', () => {
            fixture.detectChanges();
            expect(queryList.length).toBe(1);
            expect(fixture.componentInstance.queryListNotificationCounter).toBe(1);
+           expect(fixture.componentInstance.queryListStrictNotificationCounter).toBe(1);
 
            vc.move(viewRef, 0);
            fixture.detectChanges();
            expect(queryList.length).toBe(1);
            expect(fixture.componentInstance.queryListNotificationCounter).toBe(2);
+           expect(fixture.componentInstance.queryListStrictNotificationCounter).toBe(1);
          });
 
       it('should support a mix of content queries from the declaration and embedded view', () => {
@@ -1964,6 +2015,33 @@ export class QueryCompWithChanges {
   showing = false;
 }
 
+@Component({
+  selector: 'query-with-no-changes',
+  template: `
+    <query-component>
+      <div *ngIf="true" #foo></div>
+      <div *ngIf="showing">
+        Showing me should not change the content of the query
+        <div *ngIf="innerShowing" #foo></div>
+      </div>
+    </query-component>
+  `
+})
+export class QueryCompWithNoChanges {
+  showing: boolean = true;
+  innerShowing: boolean = true;
+  queryComp!: QueryCompWithNoChangesParent;
+}
+
+@Component({selector: 'query-component', template: `<ng-content></ng-content>`})
+export class QueryCompWithNoChangesParent {
+  @ContentChildren('foo', {descendants: true}) foos!: QueryList<any>;
+
+  constructor(public queryCompWithNoChanges: QueryCompWithNoChanges) {
+    queryCompWithNoChanges.queryComp = this;
+  }
+}
+
 @Component({selector: 'query-target', template: '<ng-content></ng-content>'})
 class SuperDirectiveQueryTarget {
 }