fix(core): Respect OnPush change detection strategy for dynamically created components (#51356)

atscott · thePunderWoman · commit 40bb45f32973 · 2023-09-01T17:28:27.000Z
This commit fixes a bug in the change detection algorithm that would ignore the `OnPush`/dirty flag of a component's host when it is created dynamically. That is, `OnPush` components that were not marked dirty but were created as embedded views would have their host bindings and `ngDoCheck` function always run even if they were not dirty. BREAKING CHANGE: `OnPush` components that are created dynamically now only have their host bindings refreshed and `ngDoCheck run` during change detection if they are dirty. Previously, a bug in the change detection would result in the `OnPush` configuration of dynamically created components to be ignored when executing host bindings and the `ngDoCheck` function. This is rarely encountered but can happen if code has a handle on the `ComponentRef` instance and updates values read in the `OnPush` component template without then calling either `markForCheck` or `detectChanges` on that component's `ChangeDetectorRef`. PR Close #51356
diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts
@@ -96,18 +96,6 @@ const enum ChangeDetectionMode {
    * flag are refreshed.
    */
   Targeted,
-  /**
-   * Used when refreshing a view to force a refresh of its embedded views. This mode
-   * refreshes views without taking into account their LView flags, i.e. non-dirty OnPush components
-   * will be refreshed in this mode.
-   *
-   * TODO: we should work to remove this mode. It's used in `refreshView` because that's how the
-   * code worked before introducing ChangeDetectionMode. Instead, it should pass `Global` to the
-   * `detectChangesInEmbeddedViews`. We should aim to fix this by v17 or, at the very least, prevent
-   * this flag from affecting signal views not specifically marked for refresh (currently, this flag
-   * would _also_ force signal views to be refreshed).
-   */
-  BugToForceRefreshAndIgnoreViewFlags
 }
 
 /**
@@ -164,7 +152,7 @@ export function refreshView<T>(
     // 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.BugToForceRefreshAndIgnoreViewFlags);
+    detectChangesInEmbeddedViews(lView, ChangeDetectionMode.Global);
 
     // Content query results must be refreshed before content hooks are called.
     if (tView.contentQueries !== null) {
@@ -316,8 +304,7 @@ function detectChangesInView(lView: LView, mode: ChangeDetectionMode) {
   const flags = lView[FLAGS];
   if ((flags & (LViewFlags.CheckAlways | LViewFlags.Dirty) &&
        mode === ChangeDetectionMode.Global) ||
-      flags & LViewFlags.RefreshView ||
-      mode === ChangeDetectionMode.BugToForceRefreshAndIgnoreViewFlags) {
+      flags & LViewFlags.RefreshView) {
     refreshView(tView, lView, tView.template, lView[CONTEXT]);
   } else if (lView[DESCENDANT_VIEWS_TO_REFRESH] > 0) {
     detectChangesInEmbeddedViews(lView, ChangeDetectionMode.Targeted);
diff --git a/packages/core/test/acceptance/change_detection_spec.ts b/packages/core/test/acceptance/change_detection_spec.ts
@@ -8,7 +8,7 @@
 
 
 import {CommonModule} from '@angular/common';
-import {ApplicationRef, ChangeDetectionStrategy, ChangeDetectorRef, Component, ComponentRef, Directive, DoCheck, EmbeddedViewRef, ErrorHandler, EventEmitter, Input, NgModule, OnInit, Output, QueryList, TemplateRef, Type, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core';
+import {ApplicationRef, ChangeDetectionStrategy, ChangeDetectorRef, Component, ComponentRef, createComponent, Directive, DoCheck, EmbeddedViewRef, ErrorHandler, EventEmitter, inject, Input, NgModule, OnInit, Output, QueryList, TemplateRef, Type, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core';
 import {ComponentFixture, fakeAsync, TestBed, tick} from '@angular/core/testing';
 import {expect} from '@angular/platform-browser/testing/src/matchers';
 import {BehaviorSubject} from 'rxjs';
@@ -66,6 +66,39 @@ describe('change detection', () => {
       expect(viewRef.rootNodes[0]).toHaveText('change-detected');
     });
 
+    it('should not detect changes for OnPush embedded views when they are not dirty', () => {
+      @Component({
+        selector: 'onpush',
+        template: '',
+        standalone: true,
+        changeDetection: ChangeDetectionStrategy.OnPush
+      })
+      class OnPushComponent {
+        checks = 0;
+        cdRef = inject(ChangeDetectorRef);
+        ngDoCheck() {
+          this.checks++;
+        }
+      }
+
+      @Component({template: '<ng-template #template></ng-template>', standalone: true})
+      class Container {
+        @ViewChild('template', {read: ViewContainerRef, static: true}) vcr!: ViewContainerRef;
+      }
+      const fixture = TestBed.createComponent(Container);
+      const ref = fixture.componentInstance.vcr!.createComponent(OnPushComponent);
+
+      fixture.detectChanges(false);
+      expect(ref.instance.checks).toBe(1);
+
+      fixture.detectChanges(false);
+      expect(ref.instance.checks).toBe(1);
+
+      ref.instance.cdRef.markForCheck();
+      fixture.detectChanges(false);
+      expect(ref.instance.checks).toBe(2);
+    });
+
     it('should not detect changes in child embedded views while they are detached', () => {
       const counters = {componentView: 0, embeddedView: 0};
 
