From 8592585f6a61737df1fdf942ee6bc68f666b037f Mon Sep 17 00:00:00 2001
From: Andrew Scott <atscott@google.com>
Date: Thu, 26 Oct 2023 16:56:06 -0700
Subject: [PATCH] test(core): Add test to ensure writing to signals in
 afterRender hooks throws error (#52475)

We do not yet handle running change detection again if `afterRender`
hooks write to signals.

PR Close #52475
---
 .../render3/instructions/change_detection.ts  | 44 ++++++++++---------
 .../change_detection_signals_in_zones_spec.ts | 28 +++++++++++-
 2 files changed, 51 insertions(+), 21 deletions(-)

diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts
index 89950b7e76f22..01b9eef0b30a0 100644
--- a/packages/core/src/render3/instructions/change_detection.ts
+++ b/packages/core/src/render3/instructions/change_detection.ts
@@ -46,26 +46,7 @@ export function detectChangesInternal<T>(
 
   try {
     refreshView(tView, lView, tView.template, context);
-    let retries = 0;
-    // If after running change detection, this view still needs to be refreshed or there are
-    // descendants views that need to be refreshed due to re-dirtying during the change detection
-    // run, detect changes on the view again. We run change detection in `Targeted` mode to only
-    // refresh views with the `RefreshView` flag.
-    while (lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh) ||
-           lView[REACTIVE_TEMPLATE_CONSUMER]?.dirty) {
-      if (retries === MAXIMUM_REFRESH_RERUNS) {
-        throw new RuntimeError(
-            RuntimeErrorCode.INFINITE_CHANGE_DETECTION,
-            ngDevMode &&
-                'Infinite change detection while trying to refresh views. ' +
-                    'There may be components which each cause the other to require a refresh, ' +
-                    'causing an infinite loop.');
-      }
-      retries++;
-      // Even if this view is detached, we still detect changes in targeted mode because this was
-      // the root of the change detection run.
-      detectChangesInView(lView, ChangeDetectionMode.Targeted);
-    }
+    detectChangesInViewWhileDirty(lView);
   } catch (error) {
     if (notifyErrorHandler) {
       handleError(lView, error);
@@ -85,6 +66,29 @@ export function detectChangesInternal<T>(
   }
 }
 
+function detectChangesInViewWhileDirty(lView: LView) {
+  let retries = 0;
+  // If after running change detection, this view still needs to be refreshed or there are
+  // descendants views that need to be refreshed due to re-dirtying during the change detection
+  // run, detect changes on the view again. We run change detection in `Targeted` mode to only
+  // refresh views with the `RefreshView` flag.
+  while (lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh) ||
+         lView[REACTIVE_TEMPLATE_CONSUMER]?.dirty) {
+    if (retries === MAXIMUM_REFRESH_RERUNS) {
+      throw new RuntimeError(
+          RuntimeErrorCode.INFINITE_CHANGE_DETECTION,
+          ngDevMode &&
+              'Infinite change detection while trying to refresh views. ' +
+                  'There may be components which each cause the other to require a refresh, ' +
+                  'causing an infinite loop.');
+    }
+    retries++;
+    // Even if this view is detached, we still detect changes in targeted mode because this was
+    // the root of the change detection run.
+    detectChangesInView(lView, ChangeDetectionMode.Targeted);
+  }
+}
+
 export function checkNoChangesInternal<T>(
     tView: TView, lView: LView, context: T, notifyErrorHandler = true) {
   setIsInCheckNoChangesMode(true);
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
index f580278d21713..aafffb0dcca9e 100644
--- 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,8 @@
  */
 
 import {NgFor, NgIf} from '@angular/common';
-import {ChangeDetectionStrategy, ChangeDetectorRef, Component, computed, Directive, inject, Input, signal, ViewChild} from '@angular/core';
+import {PLATFORM_BROWSER_ID} from '@angular/common/src/platform_id';
+import {afterNextRender, ChangeDetectionStrategy, ChangeDetectorRef, Component, computed, Directive, inject, Input, PLATFORM_ID, signal, ViewChild} from '@angular/core';
 import {TestBed} from '@angular/core/testing';
 
 describe('CheckAlways components', () => {
@@ -823,6 +824,31 @@ describe('OnPush components with signals', () => {
     fixture.componentInstance.cdr.detectChanges();
     expect(fixture.nativeElement.innerText).toEqual('2');
   });
+
+  // Note: We probably don't want this to throw but need to decide how to handle reruns
+  // This asserts current behavior and should be updated when this is handled
+  it('throws error when writing to a signal in afterRender', () => {
+    const counter = signal(0);
+
+    @Component({
+      selector: 'test-component',
+      standalone: true,
+      template: ` {{counter()}} `,
+    })
+    class TestCmp {
+      counter = counter;
+      constructor() {
+        afterNextRender(() => {
+          this.counter.set(1);
+        });
+      }
+    }
+    TestBed.configureTestingModule(
+        {providers: [{provide: PLATFORM_ID, useValue: PLATFORM_BROWSER_ID}]});
+
+    const fixture = TestBed.createComponent(TestCmp);
+    expect(() => fixture.detectChanges()).toThrowError(/ExpressionChanged/);
+  });
 });