fix(core): exhaustive checkNoChanges should only do a single pass (#55839)

atscott · dylhunn · commit cae0d3167daa · 2024-05-17T12:24:36.000-07:00
Because exhaustive checks traverse the whole tree regardless of the dirty state, it breaks some expectations around how change detection should be running. When a view has transplanted views, it unconditionally marks all ancestors for traversal, assuming this is fine because the loop will just traverse them and find nothing dirty. However, exhaustive checkNoChanages actually refreshes everything during traversal. This update ensures the exhaustive check only does a single pass and also prevents some unnecessary marking of transplanted views for refresh since we know they're going to be reached. PR Close #55839
diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts
@@ -15,14 +15,8 @@ import {
 
 import {RuntimeError, RuntimeErrorCode} from '../../errors';
 import {assertDefined, assertEqual} from '../../util/assert';
-import {assertLContainer} from '../assert';
 import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks';
-import {
-  CONTAINER_HEADER_OFFSET,
-  LContainer,
-  LContainerFlags,
-  MOVED_VIEWS,
-} from '../interfaces/container';
+import {CONTAINER_HEADER_OFFSET, LContainerFlags, MOVED_VIEWS} from '../interfaces/container';
 import {ComponentTemplate, RenderFlags} from '../interfaces/definition';
 import {
   CONTEXT,
@@ -32,7 +26,6 @@ import {
   InitPhaseState,
   LView,
   LViewFlags,
-  PARENT,
   REACTIVE_TEMPLATE_CONSUMER,
   TVIEW,
   TView,
@@ -119,6 +112,13 @@ function detectChangesInViewWhileDirty(lView: LView, mode: ChangeDetectionMode)
     setIsRefreshingViews(true);
     detectChangesInView(lView, mode);
 
+    // We don't need or want to do any looping when in exhaustive checkNoChanges because we
+    // already traverse all the views and nothing should change so we shouldn't have to do
+    // another pass to pick up new changes.
+    if (ngDevMode && isExhaustiveCheckNoChanges()) {
+      return;
+    }
+
     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
@@ -199,6 +199,7 @@ export function refreshView<T>(
   // Check no changes mode is a dev only mode used to verify that bindings have not changed
   // since they were assigned. We do not want to execute lifecycle hooks in that mode.
   const isInCheckNoChangesPass = ngDevMode && isInCheckNoChangesMode();
+  const isInExhaustiveCheckNoChangesPass = ngDevMode && isExhaustiveCheckNoChanges();
 
   !isInCheckNoChangesPass && lView[ENVIRONMENT].inlineEffectRunner?.flush();
 
@@ -241,10 +242,14 @@ export function refreshView<T>(
       }
     }
 
-    // First mark transplanted views that are declared in this lView as needing a refresh at their
-    // insertion points. This is needed to avoid the situation where the template is defined in this
-    // `LView` but its declaration appears after the insertion component.
-    markTransplantedViewsForRefresh(lView);
+    // We do not need to mark transplanted views for refresh when doing exhaustive checks
+    // because all views will be reached anyways during the traversal.
+    if (!isInExhaustiveCheckNoChangesPass) {
+      // First mark transplanted views that are declared in this lView as needing a refresh at their
+      // insertion points. This is needed to avoid the situation where the template is defined in this
+      // `LView` but its declaration appears after the insertion component.
+      markTransplantedViewsForRefresh(lView);
+    }
     detectChangesInEmbeddedViews(lView, ChangeDetectionMode.Global);
 
     // Content query results must be refreshed before content hooks are called.
diff --git a/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts b/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts
@@ -28,6 +28,7 @@ import {
   ViewChild,
   ViewContainerRef,
 } from '@angular/core';
+import {provideExperimentalCheckNoChangesForDebug} from '@angular/core/src/change_detection/scheduling/exhaustive_check_no_changes';
 import {ComponentFixture, TestBed} from '@angular/core/testing';
 import {expect} from '@angular/platform-browser/testing/src/matchers';
 import {of} from 'rxjs';
@@ -1105,6 +1106,45 @@ describe('change detection for transplanted views', () => {
     // checking the declaration.
     expect(() => appRef.tick()).not.toThrow();
   });
+  it('does not cause infinite loops with exhaustive checkNoChanges', async () => {
+    TestBed.configureTestingModule({
+      providers: [provideExperimentalCheckNoChangesForDebug({interval: 1})],
+    });
+    const errorSpy = spyOn(console, 'error').and.callFake((...v) => {
+      fail('console errored with ' + v);
+    });
+    @Component({
+      standalone: true,
+      selector: 'insertion',
+      template: `<ng-container #vc></ng-container>`,
+      changeDetection: ChangeDetectionStrategy.OnPush,
+    })
+    class Insertion {
+      @ViewChild('vc', {read: ViewContainerRef, static: true}) viewContainer!: ViewContainerRef;
+      @Input() template!: TemplateRef<{}>;
+      ngOnChanges() {
+        return this.viewContainer.createEmbeddedView(this.template);
+      }
+    }
+
+    @Component({
+      standalone: true,
+      template: `
+          <ng-template #template>hello world</ng-template>
+          <insertion [template]="transplantedTemplate"></insertion>
+        `,
+      imports: [Insertion],
+    })
+    class Root {
+      @ViewChild('template', {static: true}) transplantedTemplate!: TemplateRef<{}>;
+    }
+
+    const fixture = TestBed.createComponent(Root);
+    TestBed.inject(ApplicationRef).attachView(fixture.componentRef.hostView);
+    // wait the 1 tick for exhaustive check to trigger
+    await new Promise((r) => setTimeout(r, 1));
+    expect(errorSpy).not.toHaveBeenCalled();
+  });
 });
 
 function trim(text: string | null): string {
