fix(core): Avoid refreshing a host view twice when using transplanted…

… views 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 clears the refresh flags at the top of `refreshView` as well as before traversing child views in `Targeted` mode. The issue only partially exists with signals in that the flags aren't cleared but we don't end up refreshing the view a second time because the `consumerPollProducersForChange` returns `false` the second time we enter `detectChangesInView`.
angular · Nov 20, 2023 · e0cc73b · e0cc73b
1 parent c3e9a2b
commit e0cc73b
Show file tree

Hide file tree

Showing 14 changed files with 55 additions and 13 deletions.
diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts
@@ -18,7 +18,7 @@ import {CONTEXT, EFFECTS_TO_SCHEDULE, ENVIRONMENT, FLAGS, InitPhaseState, LView,
 import {getOrBorrowReactiveLViewConsumer, maybeReturnReactiveLViewConsumer, ReactiveLViewConsumer} from '../reactive_lview_consumer';
 import {enterView, isInCheckNoChangesMode, leaveView, setBindingIndex, setIsInCheckNoChangesMode} from '../state';
 import {getFirstLContainer, getNextLContainer} from '../util/view_traversal_utils';
-import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils';
+import {clearRefreshAndTraversalFlags, getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils';
 
 import {executeTemplate, executeViewQueryFn, handleError, processHostBindingOpCodes, refreshContentQueries} from './shared';
 
@@ -130,6 +130,8 @@ const enum ChangeDetectionMode {
 export function refreshView<T>(
     tView: TView, lView: LView, templateFn: ComponentTemplate<{}>|null, context: T) {
   ngDevMode && assertEqual(isCreationMode(lView), false, 'Should be run in update mode');
+  // Remove refresh and traversal flags so that they can be re-dirtied during the refresh process.
+  clearRefreshAndTraversalFlags(lView);
   const flags = lView[FLAGS];
   if ((flags & LViewFlags.Destroyed) === LViewFlags.Destroyed) return;
 
@@ -400,16 +402,11 @@ function detectChangesInView(lView: LView, mode: ChangeDetectionMode) {
   // Refresh views when they have a dirty reactive consumer, regardless of mode.
   shouldRefreshView ||= !!(consumer?.dirty && consumerPollProducersForChange(consumer));
 
-  // Mark the Flags and `ReactiveNode` as not dirty before refreshing the component, so that they
-  // can be re-dirtied during the refresh process.
-  if (consumer) {
-    consumer.dirty = false;
-  }
-  lView[FLAGS] &= ~(LViewFlags.HasChildViewsToRefresh | LViewFlags.RefreshView);
-
   if (shouldRefreshView) {
     refreshView(tView, lView, tView.template, lView[CONTEXT]);
   } else if (flags & LViewFlags.HasChildViewsToRefresh) {
+    // Remove refresh and traversal flags so that they can be re-dirtied during the refresh process.
+    clearRefreshAndTraversalFlags(lView);
     detectChangesInEmbeddedViews(lView, ChangeDetectionMode.Targeted);
     const components = tView.components;
     if (components !== null) {

diff --git a/packages/core/src/render3/util/view_utils.ts b/packages/core/src/render3/util/view_utils.ts
@@ -13,7 +13,7 @@ import {LContainer, LContainerFlags, TYPE} from '../interfaces/container';
 import {TConstants, TNode} from '../interfaces/node';
 import {RNode} from '../interfaces/renderer_dom';
 import {isLContainer, isLView} from '../interfaces/type_checks';
-import {DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, ON_DESTROY_HOOKS, PARENT, PREORDER_HOOK_FLAGS, PreOrderHookFlags, TData, TView} from '../interfaces/view';
+import {DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, ON_DESTROY_HOOKS, PARENT, PREORDER_HOOK_FLAGS, PreOrderHookFlags, REACTIVE_TEMPLATE_CONSUMER, TData, TView} from '../interfaces/view';
 
 
 
@@ -212,6 +212,20 @@ export function updateAncestorTraversalFlagsOnAttach(lView: LView) {
   }
 }
 
+/**
+ * Clears flags related to view refresh and traversal (excluding `LViewFlags.Dirty`).
+ *
+ * This helper method is meant to be used before refreshing a view. Since `LViewFlags.Dirty` is not
+ * cleared until the end of view refresh, it is not cleared in this function.
+ */
+export function clearRefreshAndTraversalFlags(lView: LView) {
+  const consumer = lView[REACTIVE_TEMPLATE_CONSUMER];
+  if (consumer) {
+    consumer.dirty = false;
+  }
+  lView[FLAGS] &= ~(LViewFlags.HasChildViewsToRefresh | LViewFlags.RefreshView);
+}
+
 /**
  * Ensures views above the given `lView` are traversed during change detection even when they are
  * not dirty.

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',

diff --git a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json
@@ -698,6 +698,9 @@
   {
     "name": "cleanUpView"
   },
+  {
+    "name": "clearRefreshAndTraversalFlags"
+  },
   {
     "name": "cloakAndComputeStyles"
   },

diff --git a/packages/core/test/bundling/animations/bundle.golden_symbols.json b/packages/core/test/bundling/animations/bundle.golden_symbols.json
@@ -758,6 +758,9 @@
   {
     "name": "cleanUpView"
   },
+  {
+    "name": "clearRefreshAndTraversalFlags"
+  },
   {
     "name": "cloakAndComputeStyles"
   },

diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json
@@ -572,6 +572,9 @@
   {
     "name": "cleanUpView"
   },
+  {
+    "name": "clearRefreshAndTraversalFlags"
+  },
   {
     "name": "collectNativeNodes"
   },

diff --git a/packages/core/test/bundling/defer/bundle.golden_symbols.json b/packages/core/test/bundling/defer/bundle.golden_symbols.json
@@ -650,6 +650,9 @@
   {
     "name": "cleanUpView"
   },
+  {
+    "name": "clearRefreshAndTraversalFlags"
+  },
   {
     "name": "collectNativeNodes"
   },

diff --git a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json
@@ -776,6 +776,9 @@
   {
     "name": "cleanUpView"
   },
+  {
+    "name": "clearRefreshAndTraversalFlags"
+  },
   {
     "name": "collectNativeNodes"
   },

diff --git a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json
@@ -755,6 +755,9 @@
   {
     "name": "cleanUpView"
   },
+  {
+    "name": "clearRefreshAndTraversalFlags"
+  },
   {
     "name": "collectNativeNodes"
   },

diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json
@@ -437,6 +437,9 @@
   {
     "name": "cleanUpView"
   },
+  {
+    "name": "clearRefreshAndTraversalFlags"
+  },
   {
     "name": "collectNativeNodes"
   },

diff --git a/packages/core/test/bundling/hydration/bundle.golden_symbols.json b/packages/core/test/bundling/hydration/bundle.golden_symbols.json
@@ -656,6 +656,9 @@
   {
     "name": "clearElementContents"
   },
+  {
+    "name": "clearRefreshAndTraversalFlags"
+  },
   {
     "name": "collectNativeNodes"
   },

diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json
@@ -989,6 +989,9 @@
   {
     "name": "cleanUpView"
   },
+  {
+    "name": "clearRefreshAndTraversalFlags"
+  },
   {
     "name": "collectNativeNodes"
   },

diff --git a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json
@@ -512,6 +512,9 @@
   {
     "name": "cleanUpView"
   },
+  {
+    "name": "clearRefreshAndTraversalFlags"
+  },
   {
     "name": "collectNativeNodes"
   },

diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json
@@ -680,6 +680,9 @@
   {
     "name": "cleanUpView"
   },
+  {
+    "name": "clearRefreshAndTraversalFlags"
+  },
   {
     "name": "collectNativeNodes"
   },