fix(core): execute query setters in non-reactive context (#49906)

pkozlowski-opensource · thePunderWoman · commit f8b95b9da62d · 2023-04-19T18:12:15.000Z
This commit assures that query results setters run when there is no active reactive consumer set. PR Close #49906
diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts
@@ -536,13 +536,18 @@ function executeTemplate<T>(
 
 export function executeContentQueries(tView: TView, tNode: TNode, lView: LView) {
   if (isContentQueryHost(tNode)) {
-    const start = tNode.directiveStart;
-    const end = tNode.directiveEnd;
-    for (let directiveIndex = start; directiveIndex < end; directiveIndex++) {
-      const def = tView.data[directiveIndex] as DirectiveDef<any>;
-      if (def.contentQueries) {
-        def.contentQueries(RenderFlags.Create, lView[directiveIndex], directiveIndex);
+    const prevConsumer = setActiveConsumer(null);
+    try {
+      const start = tNode.directiveStart;
+      const end = tNode.directiveEnd;
+      for (let directiveIndex = start; directiveIndex < end; directiveIndex++) {
+        const def = tView.data[directiveIndex] as DirectiveDef<any>;
+        if (def.contentQueries) {
+          def.contentQueries(RenderFlags.Create, lView[directiveIndex], directiveIndex);
+        }
       }
+    } finally {
+      setActiveConsumer(prevConsumer);
     }
   }
 }
@@ -1873,7 +1878,12 @@ function executeViewQueryFn<T>(
     flags: RenderFlags, viewQueryFn: ViewQueriesFunction<T>, component: T): void {
   ngDevMode && assertDefined(viewQueryFn, 'View queries function to execute must be defined.');
   setCurrentQueryIndex(0);
-  viewQueryFn(flags, component);
+  const prevConsumer = setActiveConsumer(null);
+  try {
+    viewQueryFn(flags, component);
+  } finally {
+    setActiveConsumer(prevConsumer);
+  }
 }
 
 ///////////////////////////////
diff --git a/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts b/packages/core/test/acceptance/change_detection_signals_in_zones_spec.ts
@@ -7,7 +7,7 @@
  */
 
 import {NgIf} from '@angular/common';
-import {ChangeDetectionStrategy, Component, Input} from '@angular/core';
+import {ChangeDetectionStrategy, Component, Input, ViewChild} from '@angular/core';
 import {TestBed} from '@angular/core/testing';
 
 import {signal} from '../../src/signals';
@@ -192,6 +192,56 @@ describe('OnPush components with signals', () => {
        expect(fixture.nativeElement.textContent.trim()).toEqual('initial:input');
      });
 
+  it('should not mark components as dirty when signal is read in a query result setter', () => {
+    const state = signal('initial');
+
+    @Component({
+      selector: 'with-query-setter',
+      standalone: true,
+      template: '<div #el>child</div>',
+    })
+    class WithQuerySetter {
+      el: unknown;
+      @ViewChild('el', {static: true})
+      set elQuery(result: unknown) {
+        // read a signal in a setter
+        state();
+        this.el = result;
+      }
+    }
+
+    @Component({
+      template: `
+         {{incrementTemplateExecutions()}}
+         <!-- Template constructed to execute child component constructor in the update pass of a host component -->
+         <ng-template [ngIf]="true"><with-query-setter /></ng-template>
+       `,
+      changeDetection: ChangeDetectionStrategy.OnPush,
+      standalone: true,
+      imports: [NgIf, WithQuerySetter],
+    })
+    class OnPushCmp {
+      numTemplateExecutions = 0;
+      incrementTemplateExecutions() {
+        this.numTemplateExecutions++;
+        return '';
+      }
+    }
+
+    const fixture = TestBed.createComponent(OnPushCmp);
+    const instance = fixture.componentInstance;
+
+    fixture.detectChanges();
+    expect(instance.numTemplateExecutions).toBe(1);
+    expect(fixture.nativeElement.textContent.trim()).toEqual('child');
+
+    // The "state" signal is not accesses in the template's update function anywhere so it
+    // shouldn't mark components as dirty / impact change detection.
+    state.set('new');
+    fixture.detectChanges();
+    expect(instance.numTemplateExecutions).toBe(1);
+  });
+
   it('can read a signal in a host binding', () => {
     @Component({
       template: `{{incrementTemplateExecutions()}}`,
diff --git a/packages/core/test/render3/reactivity_spec.ts b/packages/core/test/render3/reactivity_spec.ts
@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {AfterViewInit, Component, destroyPlatform, effect, inject, Injector, Input, NgZone, OnChanges, signal, SimpleChanges} from '@angular/core';
+import {AfterViewInit, Component, ContentChildren, createComponent, destroyPlatform, effect, EnvironmentInjector, inject, Injector, Input, NgZone, OnChanges, QueryList, signal, SimpleChanges, ViewChild} from '@angular/core';
 import {TestBed} from '@angular/core/testing';
 import {bootstrapApplication} from '@angular/platform-browser';
 import {withBody} from '@angular/private/testing';
@@ -307,4 +307,83 @@ describe('effects', () => {
     fixture.detectChanges();
     expect(fixture.nativeElement.textContent).toBe('binding|static');
   });
+
+  it('should allow writing to signals in query result setters', () => {
+    @Component({
+      selector: 'with-query',
+      standalone: true,
+      template: '{{items().length}}',
+    })
+    class WithQuery {
+      items = signal<unknown[]>([]);
+
+      @ContentChildren('item')
+      set itemsQuery(result: QueryList<unknown>) {
+        this.items.set(result.toArray());
+      }
+    }
+
+    @Component({
+      selector: 'test-cmp',
+      standalone: true,
+      imports: [WithQuery],
+      template: `<with-query><div #item></div></with-query>`,
+    })
+    class Cmp {
+    }
+
+    const fixture = TestBed.createComponent(Cmp);
+    fixture.detectChanges();
+    expect(fixture.nativeElement.textContent).toBe('1');
+  });
+
+  it('should not execute query setters in the reactive context', () => {
+    const state = signal('initial');
+
+    @Component({
+      selector: 'with-query-setter',
+      standalone: true,
+      template: '<div #el></div>',
+
+    })
+    class WithQuerySetter {
+      el: unknown;
+      @ViewChild('el', {static: true})
+      set elQuery(result: unknown) {
+        // read a signal in a setter - I want to verify that framework executes this code outside of
+        // the reactive context
+        state();
+        this.el = result;
+      }
+    }
+
+    @Component({
+      selector: 'test-cmp',
+      standalone: true,
+      template: ``,
+    })
+    class Cmp {
+      noOfCmpCreated = 0;
+      constructor(environmentInjector: EnvironmentInjector) {
+        // A slightly artificial setup where a component instance is created using imperative APIs.
+        // We don't have control over the timing / reactive context of such API calls so need to
+        // code defensively in the framework.
+
+        // Here we want to specifically verify that an effect is _not_ re-run if a signal read
+        // happens in a query setter of a dynamically created component.
+        effect(() => {
+          createComponent(WithQuerySetter, {environmentInjector});
+          this.noOfCmpCreated++;
+        });
+      }
+    }
+
+    const fixture = TestBed.createComponent(Cmp);
+    fixture.detectChanges();
+    expect(fixture.componentInstance.noOfCmpCreated).toBe(1);
+
+    state.set('changed');
+    fixture.detectChanges();
+    expect(fixture.componentInstance.noOfCmpCreated).toBe(1);
+  });
 });
