fix(core): Avoid refreshing a host view twice when using transplanted views (#53021)

atscott · alxhub · commit 58ed76be937c · 2023-12-07T12:05:24.000-08:00
This change fixes and issue where the expectation was that change detection always goes through `detectChangesInView`. In reality, `detectChangesInternal` directly calls `refreshView` and refreshes a view directly without checking if it was dirty (to my discontent). This update changes the implementation of `detectChangesInternal` to actually be "detect changes" not "force refresh of root view and detect changes". In addition, it adds the refresh flag to APIs that were previously calling `detectChangesInternal` so we get the same behavior as before (host view is forced to refresh). Note that the use of `RefreshView` instead of `Dirty` is _intentional_ here. The `RefreshView` flag is cleared before refreshing the view while the `Dirty` flag is cleared at the very end. Using the `Dirty` flag could have consequences because it is a more long-lasting change to the view flags. Because `detectChangesInView` will immediately clear the `RefreshView` flag, this change is much more limited and does not result in a different set of flags during the view refresh. PR Close #53021
diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts
@@ -43,9 +43,6 @@ export function detectChangesInternal(lView: LView, notifyErrorHandler = true) {
   }
 
   try {
-    const tView = lView[TVIEW];
-    const context = lView[CONTEXT];
-    refreshView(tView, lView, tView.template, context);
     detectChangesInViewWhileDirty(lView);
   } catch (error) {
     if (notifyErrorHandler) {
@@ -67,6 +64,8 @@ export function detectChangesInternal(lView: LView, notifyErrorHandler = true) {
 }
 
 function detectChangesInViewWhileDirty(lView: LView) {
+  detectChangesInView(lView, ChangeDetectionMode.Global);
+
   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
diff --git a/packages/core/src/render3/util/change_detection_utils.ts b/packages/core/src/render3/util/change_detection_utils.ts
@@ -10,7 +10,7 @@ import {assertDefined} from '../../util/assert';
 import {getComponentViewByInstance} from '../context_discovery';
 import {detectChangesInternal} from '../instructions/change_detection';
 import {markViewDirty} from '../instructions/mark_view_dirty';
-import {TVIEW} from '../interfaces/view';
+import {FLAGS, LViewFlags} from '../interfaces/view';
 
 import {getRootComponents} from './discovery_utils';
 
@@ -38,5 +38,6 @@ export function applyChanges(component: {}): void {
  */
 function detectChanges(component: {}): void {
   const view = getComponentViewByInstance(component);
+  view[FLAGS] |= LViewFlags.RefreshView;
   detectChangesInternal(view);
 }
diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts
@@ -284,6 +284,12 @@ export class ViewRef<T> implements EmbeddedViewRef<T>, ChangeDetectorRefInterfac
    * See {@link ChangeDetectorRef#detach} for more information.
    */
   detectChanges(): void {
+    // Add `RefreshView` flag to ensure this view is refreshed if not already dirty.
+    // `RefreshView` flag is used intentionally over `Dirty` because it gets cleared before
+    // executing any of the actual refresh code while the `Dirty` flag doesn't get cleared
+    // until the end of the refresh. Using `RefreshView` prevents creating a potential difference
+    // in the state of the LViewFlags during template execution.
+    this._lView[FLAGS] |= LViewFlags.RefreshView;
     detectChangesInternal(this._lView, this.notifyErrorHandler);
   }
 
diff --git a/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts b/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts
@@ -734,8 +734,7 @@ describe('change detection for transplanted views', () => {
       // Detach view, manually call `detectChanges`, and verify the template was refreshed
       component.rootViewContainerRef.detach();
       viewRef.detectChanges();
-      // This view is a backwards reference so it's refreshed twice
-      expect(component.checks).toEqual(2);
+      expect(component.checks).toEqual(1);
     });
 
     it('should work when change detecting detached transplanted view already marked for refresh',
@@ -751,8 +750,7 @@ describe('change detection for transplanted views', () => {
            // should not affect parent counters.
            viewRef.detectChanges();
          }).not.toThrow();
-         // This view is a backwards reference so it's refreshed twice
-         expect(component.checks).toEqual(2);
+         expect(component.checks).toEqual(1);
        });
 
     it('should work when re-inserting a previously detached transplanted view marked for refresh',
