fix(core): execute template creation in non-reactive context (#49883)

pkozlowski-opensource · thePunderWoman · commit b7392f906473 · 2023-04-18T13:59:42.000Z
This fix assures that templates functions executed in the creation mode are run outside of the reactive context. This avoids the situation where signal reads in a directive constructor (executed as part of the creation mode) would mark the host component as dirty. Fixes #49871 PR Close #49883
diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts
@@ -17,6 +17,7 @@ import {DoCheck, OnChanges, OnInit} from '../../interface/lifecycle_hooks';
 import {SchemaMetadata} from '../../metadata/schema';
 import {ViewEncapsulation} from '../../metadata/view';
 import {validateAgainstEventAttributes, validateAgainstEventProperties} from '../../sanitization/sanitization';
+import {setActiveConsumer} from '../../signals';
 import {assertDefined, assertEqual, assertGreaterThan, assertGreaterThanOrEqual, assertIndexInRange, assertNotEqual, assertNotSame, assertSame, assertString} from '../../util/assert';
 import {escapeCommentText} from '../../util/dom';
 import {normalizeDebugBindingName, normalizeDebugBindingValue} from '../../util/ng_reflect';
@@ -507,9 +508,18 @@ function executeTemplate<T>(
     const preHookType =
         isUpdatePhase ? ProfilerEvent.TemplateUpdateStart : ProfilerEvent.TemplateCreateStart;
     profiler(preHookType, context as unknown as {});
-    consumer.runInContext(templateFn, rf, context);
+    if (isUpdatePhase) {
+      consumer.runInContext(templateFn, rf, context);
+    } else {
+      const prevConsumer = setActiveConsumer(null);
+      try {
+        templateFn(rf, context);
+      } finally {
+        setActiveConsumer(prevConsumer);
+      }
+    }
   } finally {
-    if (lView[REACTIVE_TEMPLATE_CONSUMER] === null) {
+    if (isUpdatePhase && lView[REACTIVE_TEMPLATE_CONSUMER] === null) {
       commitLViewConsumerIfHasProducers(lView, REACTIVE_TEMPLATE_CONSUMER);
     }
     setSelectedIndex(prevSelectedIndex);
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
@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
+import {NgIf} from '@angular/common';
 import {ChangeDetectionStrategy, Component} from '@angular/core';
 import {TestBed} from '@angular/core/testing';
 
@@ -92,6 +93,54 @@ describe('OnPush components with signals', () => {
     expect(instance.value()).toBe('new');
   });
 
+  it('should not mark components as dirty when signal is read in a constructor of a child component',
+     () => {
+       const state = signal('initial');
+
+       @Component({
+         selector: 'child',
+         template: `child`,
+         changeDetection: ChangeDetectionStrategy.OnPush,
+         standalone: true,
+       })
+       class ChildReadingSignalCmp {
+         constructor() {
+           state();
+         }
+       }
+
+       @Component({
+         template: `
+            {{incrementTemplateExecutions()}}
+            <!-- Template constructed to execute child component constructor in the update pass of a host component -->
+            <ng-template [ngIf]="true"><child></child></ng-template>
+          `,
+         changeDetection: ChangeDetectionStrategy.OnPush,
+         standalone: true,
+         imports: [NgIf, ChildReadingSignalCmp],
+       })
+       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
@@ -239,8 +239,7 @@ describe('effects', () => {
       selector: 'test-cmp',
       standalone: true,
       imports: [WithInput],
-      template: `<with-input [in]="'A'" />|<with-input [in]="'B'" />
-      `,
+      template: `<with-input [in]="'A'" />|<with-input [in]="'B'" />`,
     })
     class Cmp {
     }
@@ -249,4 +248,32 @@ describe('effects', () => {
     fixture.detectChanges();
     expect(fixture.nativeElement.textContent).toBe('A|B');
   });
+
+  it('should allow writing to signals in a constructor', () => {
+    @Component({
+      selector: 'with-constructor',
+      standalone: true,
+      template: '{{state()}}',
+    })
+    class WithConstructor {
+      state = signal('property initializer');
+
+      constructor() {
+        this.state.set('constructor');
+      }
+    }
+
+    @Component({
+      selector: 'test-cmp',
+      standalone: true,
+      imports: [WithConstructor],
+      template: `<with-constructor />`,
+    })
+    class Cmp {
+    }
+
+    const fixture = TestBed.createComponent(Cmp);
+    fixture.detectChanges();
+    expect(fixture.nativeElement.textContent).toBe('constructor');
+  });
 });
