diff --git a/packages/core/src/render3/instructions/change_detection.ts b/packages/core/src/render3/instructions/change_detection.ts index 35d88d5eb92..a5f3423c167 100644 --- a/packages/core/src/render3/instructions/change_detection.ts +++ b/packages/core/src/render3/instructions/change_detection.ts @@ -10,12 +10,12 @@ import {assertDefined, assertEqual} from '../../util/assert'; import {assertLContainer} from '../assert'; import {getComponentViewByInstance} from '../context_discovery'; import {executeCheckHooks, executeInitAndCheckHooks, incrementInitPhaseFlags} from '../hooks'; -import {CONTAINER_HEADER_OFFSET, HAS_TRANSPLANTED_VIEWS, LContainer, MOVED_VIEWS} from '../interfaces/container'; +import {CONTAINER_HEADER_OFFSET, HAS_CHILD_VIEWS_TO_REFRESH, HAS_TRANSPLANTED_VIEWS, LContainer, MOVED_VIEWS} from '../interfaces/container'; import {ComponentTemplate, RenderFlags} from '../interfaces/definition'; -import {CONTEXT, DESCENDANT_VIEWS_TO_REFRESH, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, TVIEW, TView} from '../interfaces/view'; +import {CONTEXT, ENVIRONMENT, FLAGS, InitPhaseState, LView, LViewFlags, PARENT, TVIEW, TView} from '../interfaces/view'; import {enterView, isInCheckNoChangesMode, leaveView, setBindingIndex, setIsInCheckNoChangesMode} from '../state'; import {getFirstLContainer, getNextLContainer} from '../util/view_traversal_utils'; -import {clearViewRefreshFlag, getComponentLViewByIndex, isCreationMode, markViewForRefresh, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils'; +import {getComponentLViewByIndex, isCreationMode, markAncestorsForTraversal, markViewForRefresh, resetPreOrderHookFlags, viewAttachedToChangeDetector} from '../util/view_utils'; import {executeTemplate, executeViewQueryFn, handleError, processHostBindingOpCodes, refreshContentQueries} from './shared'; @@ -228,7 +228,15 @@ export function refreshView( if (!isInCheckNoChangesPass) { lView[FLAGS] &= ~(LViewFlags.Dirty | LViewFlags.FirstLViewPass); } - clearViewRefreshFlag(lView); + lView[FLAGS] &= ~LViewFlags.RefreshView; + } catch (e) { + // If refreshing a view causes an error, we need to remark the ancestors as needing traversal + // because the error might have caused a situation where views below the current location are + // dirty but will be unreachable because the "has dirty children" flag in the ancestors has been + // cleared during change detection and we failed to run to completion. + + markAncestorsForTraversal(lView); + throw e; } finally { leaveView(); } @@ -241,9 +249,10 @@ export function refreshView( function detectChangesInEmbeddedViews(lView: LView, mode: ChangeDetectionMode) { for (let lContainer = getFirstLContainer(lView); lContainer !== null; lContainer = getNextLContainer(lContainer)) { + lContainer[HAS_CHILD_VIEWS_TO_REFRESH] = false; for (let i = CONTAINER_HEADER_OFFSET; i < lContainer.length; i++) { const embeddedLView = lContainer[i]; - detectChangesInView(embeddedLView, mode); + detectChangesInViewIfAttached(embeddedLView, mode); } } } @@ -279,34 +288,44 @@ function detectChangesInComponent( hostLView: LView, componentHostIdx: number, mode: ChangeDetectionMode): void { ngDevMode && assertEqual(isCreationMode(hostLView), false, 'Should be run in update mode'); const componentView = getComponentLViewByIndex(componentHostIdx, hostLView); - detectChangesInView(componentView, mode); + detectChangesInViewIfAttached(componentView, mode); } /** * Visits a view as part of change detection traversal. * - * - If the view is detached, no additional traversal happens. + * If the view is detached, no additional traversal happens. + */ +function detectChangesInViewIfAttached(lView: LView, mode: ChangeDetectionMode) { + if (!viewAttachedToChangeDetector(lView)) { + return; + } + detectChangesInView(lView, mode); +} + +/** + * Visits a view as part of change detection traversal. * * The view is refreshed if: * - If the view is CheckAlways or Dirty and ChangeDetectionMode is `Global` * - If the view has the `RefreshTransplantedView` flag * * The view is not refreshed, but descendants are traversed in `ChangeDetectionMode.Targeted` if the - * view has a non-zero TRANSPLANTED_VIEWS_TO_REFRESH counter. - * + * view HasChildViewsToRefresh flag is set. */ function detectChangesInView(lView: LView, mode: ChangeDetectionMode) { - if (!viewAttachedToChangeDetector(lView)) { - return; - } - const tView = lView[TVIEW]; const flags = lView[FLAGS]; + + // Flag cleared before change detection runs so that the view can be re-marked for traversal if + // necessary. + lView[FLAGS] &= ~LViewFlags.HasChildViewsToRefresh; + if ((flags & (LViewFlags.CheckAlways | LViewFlags.Dirty) && mode === ChangeDetectionMode.Global) || flags & LViewFlags.RefreshView) { refreshView(tView, lView, tView.template, lView[CONTEXT]); - } else if (lView[DESCENDANT_VIEWS_TO_REFRESH] > 0) { + } else if (flags & LViewFlags.HasChildViewsToRefresh) { detectChangesInEmbeddedViews(lView, ChangeDetectionMode.Targeted); const components = tView.components; if (components !== null) { diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 0f03f10361d..7b86be4bb5e 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -1414,8 +1414,8 @@ export function createLContainer( false, // has transplanted views currentView, // parent null, // next - 0, // transplanted views to refresh count tNode, // t_host + false, // has child views to refresh native, // native, null, // view refs null, // moved views diff --git a/packages/core/src/render3/interfaces/container.ts b/packages/core/src/render3/interfaces/container.ts index 524874ea4f5..512cf1c0998 100644 --- a/packages/core/src/render3/interfaces/container.ts +++ b/packages/core/src/render3/interfaces/container.ts @@ -10,7 +10,7 @@ import {DehydratedContainerView} from '../../hydration/interfaces'; import {TNode} from './node'; import {RComment, RElement} from './renderer_dom'; -import {DESCENDANT_VIEWS_TO_REFRESH, HOST, LView, NEXT, PARENT, T_HOST} from './view'; +import {HOST, LView, NEXT, PARENT, T_HOST} from './view'; @@ -37,18 +37,18 @@ export const TYPE = 1; */ export const HAS_TRANSPLANTED_VIEWS = 2; -// PARENT, NEXT, DESCENDANT_VIEWS_TO_REFRESH are indices 3, 4, and 5 +// PARENT and NEXT are indices 3 and 4 // As we already have these constants in LView, we don't need to re-create them. -// T_HOST is index 6 +// T_HOST is index 5 // We already have this constants in LView, we don't need to re-create it. +export const HAS_CHILD_VIEWS_TO_REFRESH = 6; export const NATIVE = 7; export const VIEW_REFS = 8; export const MOVED_VIEWS = 9; export const DEHYDRATED_VIEWS = 10; - /** * Size of LContainer's header. Represents the index after which all views in the * container will be inserted. We need to keep a record of current views so we know @@ -101,12 +101,10 @@ export interface LContainer extends Array { [NEXT]: LView|LContainer|null; /** - * The number of direct transplanted views which need a refresh or have descendants themselves - * that need a refresh but have not marked their ancestors as Dirty. This tells us that during - * change detection we should still descend to find those children to refresh, even if the parents - * are not `Dirty`/`CheckAlways`. + * Indicates that this LContainer has a view underneath it that needs to be refreshed during + * change detection. */ - [DESCENDANT_VIEWS_TO_REFRESH]: number; + [HAS_CHILD_VIEWS_TO_REFRESH]: boolean; /** * A collection of views created based on the underlying `` element but inserted into diff --git a/packages/core/src/render3/interfaces/view.ts b/packages/core/src/render3/interfaces/view.ts index e85cc790799..8067664bfba 100644 --- a/packages/core/src/render3/interfaces/view.ts +++ b/packages/core/src/render3/interfaces/view.ts @@ -33,10 +33,14 @@ import {TDeferBlockDetails} from './defer'; export const HOST = 0; export const TVIEW = 1; export const FLAGS = 2; + +// Shared with LContainer export const PARENT = 3; export const NEXT = 4; -export const DESCENDANT_VIEWS_TO_REFRESH = 5; -export const T_HOST = 6; +export const T_HOST = 5; +// End shared with LContainer + +export const HYDRATION = 6; export const CLEANUP = 7; export const CONTEXT = 8; export const INJECTOR = 9; @@ -53,9 +57,9 @@ export const QUERIES = 18; export const ID = 19; export const EMBEDDED_VIEW_INJECTOR = 20; export const ON_DESTROY_HOOKS = 21; -export const HYDRATION = 22; export const REACTIVE_TEMPLATE_CONSUMER = 23; export const REACTIVE_HOST_BINDING_CONSUMER = 24; + /** * Size of LView's header. Necessary to adjust for it when setting slots. * @@ -318,14 +322,6 @@ export interface LView extends Array { */ [PREORDER_HOOK_FLAGS]: PreOrderHookFlags; - /** - * The number of direct transplanted views which need a refresh or have descendants themselves - * that need a refresh but have not marked their ancestors as Dirty. This tells us that during - * change detection we should still descend to find those children to refresh, even if the parents - * are not `Dirty`/`CheckAlways`. - */ - [DESCENDANT_VIEWS_TO_REFRESH]: number; - /** Unique ID of the view. Used for `__ngContext__` lookups in the `LView` registry. */ [ID]: number; @@ -424,8 +420,8 @@ export const enum LViewFlags { /** * Whether this moved LView was needs to be refreshed. Similar to the Dirty flag, but used for * transplanted and signal views where the parent/ancestor views are not marked dirty as well. - * i.e. "Refresh just this view". Used in conjunction with the DESCENDANT_VIEWS_TO_REFRESH - * counter. + * i.e. "Refresh just this view". Used in conjunction with the HAS_CHILD_VIEWS_TO_REFRESH + * flag. */ RefreshView = 1 << 10, @@ -435,10 +431,17 @@ export const enum LViewFlags { /** Indicates that the view was created with `signals: true`. */ SignalView = 1 << 12, + /** + * Indicates that this LView has a view underneath it that needs to be refreshed during change + * detection. This flag indicates that even if this view is not dirty itself, we still need to + * traverse its children during change detection. + */ + HasChildViewsToRefresh = 1 << 13, + /** * This is the count of the bits the 1 was shifted above (base 10) */ - IndexWithinInitPhaseShift = 13, + IndexWithinInitPhaseShift = 14, /** * Index of the current init phase on last 21 bits diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index bcd35f06b72..9f317b78261 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -31,7 +31,7 @@ import {assertTNodeType} from './node_assert'; import {profiler, ProfilerEvent} from './profiler'; import {setUpAttributes} from './util/attrs_utils'; import {getLViewParent} from './util/view_traversal_utils'; -import {clearViewRefreshFlag, getNativeByTNode, unwrapRNode} from './util/view_utils'; +import {getNativeByTNode, unwrapRNode, updateAncestorTraversalFlagsOnAttach} from './util/view_utils'; const enum WalkTNodeTreeAction { /** node create in the native environment. Run on initial creation. */ @@ -275,6 +275,7 @@ export function insertView(tView: TView, lView: LView, lContainer: LContainer, i lQueries.insertView(tView); } + updateAncestorTraversalFlagsOnAttach(lView); // Sets the attached flag lView[FLAGS] |= LViewFlags.Attached; } @@ -316,11 +317,6 @@ function detachMovedView(declarationContainer: LContainer, lView: LView) { const declarationViewIndex = movedViews.indexOf(lView); const insertionLContainer = lView[PARENT] as LContainer; ngDevMode && assertLContainer(insertionLContainer); - - // If the view was marked for refresh but then detached before it was checked (where the flag - // would be cleared and the counter decremented), we need to update the status here. - clearViewRefreshFlag(lView); - movedViews.splice(declarationViewIndex, 1); } diff --git a/packages/core/src/render3/util/view_utils.ts b/packages/core/src/render3/util/view_utils.ts index c114d5385d6..1a7756426fa 100644 --- a/packages/core/src/render3/util/view_utils.ts +++ b/packages/core/src/render3/util/view_utils.ts @@ -9,11 +9,11 @@ import {RuntimeError, RuntimeErrorCode} from '../../errors'; import {assertDefined, assertGreaterThan, assertGreaterThanOrEqual, assertIndexInRange, assertLessThan} from '../../util/assert'; import {assertTNode, assertTNodeForLView} from '../assert'; -import {LContainer, TYPE} from '../interfaces/container'; +import {HAS_CHILD_VIEWS_TO_REFRESH, LContainer, 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, DESCENDANT_VIEWS_TO_REFRESH, 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, TData, TView} from '../interfaces/view'; @@ -165,24 +165,16 @@ export function resetPreOrderHookFlags(lView: LView) { } /** - * Adds the `RefreshView` flag from the lView and updates DESCENDANT_VIEWS_TO_REFRESH counters of + * Adds the `RefreshView` flag from the lView and updates HAS_CHILD_VIEWS_TO_REFRESH flag of * parents. */ export function markViewForRefresh(lView: LView) { - if ((lView[FLAGS] & LViewFlags.RefreshView) === 0) { - lView[FLAGS] |= LViewFlags.RefreshView; - updateViewsToRefresh(lView, 1); - } -} - -/** - * Removes the `RefreshView` flag from the lView and updates DESCENDANT_VIEWS_TO_REFRESH counters of - * parents. - */ -export function clearViewRefreshFlag(lView: LView) { if (lView[FLAGS] & LViewFlags.RefreshView) { - lView[FLAGS] &= ~LViewFlags.RefreshView; - updateViewsToRefresh(lView, -1); + return; + } + lView[FLAGS] |= LViewFlags.RefreshView; + if (viewAttachedToChangeDetector(lView)) { + markAncestorsForTraversal(lView); } } @@ -210,20 +202,45 @@ export function walkUpViews(nestingLevel: number, currentView: LView): LView { * 1. counter goes from 0 to 1, indicating that there is a new child that has a view to refresh * or * 2. counter goes from 1 to 0, indicating there are no more descendant views to refresh + * When attaching/re-attaching an `LView` to the change detection tree, we need to ensure that the + * views above it are traversed during change detection if this one is marked for refresh or has + * some child or descendant that needs to be refreshed. */ -function updateViewsToRefresh(lView: LView, amount: 1|- 1) { - let parent: LView|LContainer|null = lView[PARENT]; +export function updateAncestorTraversalFlagsOnAttach(lView: LView) { + if (lView[FLAGS] & (LViewFlags.RefreshView | LViewFlags.HasChildViewsToRefresh)) { + markAncestorsForTraversal(lView); + } +} + +/** + * Ensures views above the given `lView` are traversed during change detection even when they are + * not dirty. + * + * This is done by setting the `HAS_CHILD_VIEWS_TO_REFRESH` flag up to the root, stopping when the + * flag is already `true` or the `lView` is detached. + */ +export function markAncestorsForTraversal(lView: LView) { + let parent = lView[PARENT]; if (parent === null) { return; } - parent[DESCENDANT_VIEWS_TO_REFRESH] += amount; - let viewOrContainer: LView|LContainer = parent; - parent = parent[PARENT]; - while (parent !== null && - ((amount === 1 && viewOrContainer[DESCENDANT_VIEWS_TO_REFRESH] === 1) || - (amount === -1 && viewOrContainer[DESCENDANT_VIEWS_TO_REFRESH] === 0))) { - parent[DESCENDANT_VIEWS_TO_REFRESH] += amount; - viewOrContainer = parent; + + while (parent !== null) { + // We stop adding markers to the ancestors once we reach one that already has the marker. This + // is to avoid needlessly traversing all the way to the root when the marker already exists. + if ((isLContainer(parent) && parent[HAS_CHILD_VIEWS_TO_REFRESH] || + (isLView(parent) && parent[FLAGS] & LViewFlags.HasChildViewsToRefresh))) { + break; + } + + if (isLContainer(parent)) { + parent[HAS_CHILD_VIEWS_TO_REFRESH] = true; + } else { + parent[FLAGS] |= LViewFlags.HasChildViewsToRefresh; + if (!viewAttachedToChangeDetector(parent)) { + break; + } + } parent = parent[PARENT]; } } diff --git a/packages/core/src/render3/view_ref.ts b/packages/core/src/render3/view_ref.ts index 40080ea84b6..6aeaedc35bb 100644 --- a/packages/core/src/render3/view_ref.ts +++ b/packages/core/src/render3/view_ref.ts @@ -19,7 +19,7 @@ import {CONTAINER_HEADER_OFFSET, VIEW_REFS} from './interfaces/container'; import {isLContainer} from './interfaces/type_checks'; import {CONTEXT, FLAGS, LView, LViewFlags, PARENT, TVIEW} from './interfaces/view'; import {destroyLView, detachView, detachViewFromDOM} from './node_manipulation'; -import {storeLViewOnDestroy} from './util/view_utils'; +import {storeLViewOnDestroy, updateAncestorTraversalFlagsOnAttach} from './util/view_utils'; // Needed due to tsickle downleveling where multiple `implements` with classes creates @@ -258,6 +258,7 @@ export class ViewRef implements EmbeddedViewRef, InternalViewRef, ChangeDe * ``` */ reattach(): void { + updateAncestorTraversalFlagsOnAttach(this._lView); this._lView[FLAGS] |= LViewFlags.Attached; } diff --git a/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts b/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts index e7f85582acd..3eba4179097 100644 --- a/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts +++ b/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts @@ -7,8 +7,8 @@ */ import {CommonModule} from '@angular/common'; -import {ChangeDetectionStrategy, ChangeDetectorRef, Component, DoCheck, Input, TemplateRef, Type, ViewChild, ViewContainerRef} from '@angular/core'; -import {AfterViewChecked} from '@angular/core/src/core'; +import {ChangeDetectionStrategy, ChangeDetectorRef, Component, DoCheck, inject, Input, TemplateRef, Type, ViewChild, ViewContainerRef} from '@angular/core'; +import {AfterViewChecked, EmbeddedViewRef} from '@angular/core/src/core'; import {ComponentFixture, TestBed} from '@angular/core/testing'; describe('change detection for transplanted views', () => { @@ -306,12 +306,27 @@ describe('change detection for transplanted views', () => { expect(log).toEqual(['Insert', null]); log.length = 0; }); + + it('does not cause infinite change detection if transplanted view is dirty and destroyed before refresh', + () => { + // mark declaration point dirty + onPushDeclareComp.changeDetector.markForCheck(); + // detach insertion so the transplanted view doesn't get refreshed when CD runs + insertForOnPushDeclareComp.changeDetectorRef.detach(); + // run CD, which will set the `RefreshView` flag on the transplanted view + fixture.detectChanges(false); + // reattach insertion so the DESCENDANT_VIEWS counters update + insertForOnPushDeclareComp.changeDetectorRef.reattach(); + // make it so the insertion is destroyed before getting refreshed + fixture.componentInstance.showInsertForOnPushDeclare = false; + // run CD again. If we didn't clear the flag/counters when destroying the view, this + // would cause an infinite CD because the counters will be >1 but we will never reach a + // view to refresh and decrement the counters. + fixture.detectChanges(false); + }); }); }); - // Note that backwards references are not handled well in VE or Ivy at the moment. - // These tests assert the current behavior. Some of these would need to be updated - // if future work refreshes backwards referenced transplanted views. describe('backwards references', () => { @Component({ selector: 'insertion', @@ -383,7 +398,7 @@ describe('change detection for transplanted views', () => { }); - it('should not update declaration view when there is a change in the declaration and insertion is marked dirty', + it('should update declaration view when there is a change in the declaration and insertion is marked dirty', () => { appComponent.declaration.name = 'new name'; appComponent.insertion.changeDetectorRef.markForCheck(); @@ -416,33 +431,15 @@ describe('change detection for transplanted views', () => { expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(1); }); - it('should not update anything when there is a change to insertion and declaration is marked dirty', - () => { - appComponent.insertion.name = 'new name'; - appComponent.declaration.changeDetectorRef.markForCheck(); - fixture.detectChanges(false); - // Name should not update in insertion view because only declaration was marked dirty - // Context name also does not update in the template because the insertion view needs to be - // checked to update the `ngTemplateOutletContext` input. - expect(fixture.nativeElement.textContent) - .toEqual( - 'Insertion(initial)TemplateDeclaration(initial)TemplateContext(initial)Declaration(initial)', - 'Name should not update in insertion view because only declaration was marked dirty\n' + - 'Context name also does not update in the template because the insertion view needs to be' + - 'checked to update the `ngTemplateOutletContext` input.'); - // Note here that if backwards references were better supported, we would be able to - // refresh the transplanted view in the first `detectChanges` call but because the - // insertion point was already checked, we need to call detectChanges again to refresh it. - expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(0); - - fixture.detectChanges(false); - expect(fixture.nativeElement.textContent) - .toEqual( - 'Insertion(initial)TemplateDeclaration(initial)TemplateContext(initial)Declaration(initial)', - 'Expected bindings to still be initial values. Again, TemplateContext can only update if ' + - 'insertion point is dirty and updates the ngTemplateOutletContext input.'); - expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(1); - }); + it('should not update when there is a change to insertion and declaration is marked dirty', () => { + appComponent.insertion.name = 'new name'; + appComponent.declaration.changeDetectorRef.markForCheck(); + fixture.detectChanges(false); + expect(fixture.nativeElement.textContent) + .toEqual( + 'Insertion(initial)TemplateDeclaration(initial)TemplateContext(initial)Declaration(initial)'); + expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(0); + }); it('should update insertion view and template when there is a change to insertion and insertion marked dirty', () => { @@ -460,14 +457,13 @@ describe('change detection for transplanted views', () => { expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(0); }); - it('should only refresh template once when declaration and insertion are marked dirty', () => { + it('should refresh template when declaration and insertion are marked dirty', () => { appComponent.declaration.changeDetectorRef.markForCheck(); appComponent.insertion.changeDetectorRef.markForCheck(); fixture.detectChanges(false); expect(appComponent.declaration.transplantedViewRefreshCount) - .toEqual( - 1, - 'Expected transplanted view to only be refreshed when insertion component is refreshed'); + .withContext('Should refresh once because backwards references are not rechecked') + .toEqual(1); }); }); @@ -498,7 +494,17 @@ describe('change detection for transplanted views', () => { }) class OnPushDeclaration { @ViewChild(OnPushInsertionHost) onPushInsertionHost?: OnPushInsertionHost; - value = 'initial'; + private _value = 'initial'; + throwErrorInView = false; + get value() { + if (this.throwErrorInView) { + throw new Error('error getting value in transplanted view'); + } + return this._value; + } + set value(v: string) { + this._value = v; + } constructor(readonly cdr: ChangeDetectorRef) {} } @@ -523,6 +529,22 @@ describe('change detection for transplanted views', () => { .createComponent(componentUnderTest); } + it('can recover from errors thrown during change detection', () => { + const fixture = getFixture(OnPushDeclaration); + fixture.detectChanges(); + fixture.componentInstance.value = 'new'; + fixture.componentInstance.cdr.markForCheck(); + fixture.componentInstance.throwErrorInView = true; + expect(() => { + fixture.detectChanges(); + }).toThrow(); + // Ensure that the transplanted view can still get refreshed if we rerun change detection + // without the error + fixture.componentInstance.throwErrorInView = false; + fixture.detectChanges(); + expect(fixture.nativeElement.textContent).toEqual('new'); + }); + it('refresh when transplanted view is declared in CheckAlways component', () => { const fixture = getFixture(CheckAlwaysDeclaration); fixture.detectChanges(); @@ -593,40 +615,113 @@ describe('change detection for transplanted views', () => { 'Expected transplanted view to be refreshed even when insertion is not dirty'); }); - it('should not fail when change detecting detached transplanted view', () => { + describe('ViewRef and ViewContainerRef operations', () => { @Component({template: '{{incrementChecks()}}'}) class AppComponent { @ViewChild(TemplateRef) templateRef!: TemplateRef<{}>; - constructor(readonly rootVref: ViewContainerRef, readonly cdr: ChangeDetectorRef) {} + constructor( + readonly rootViewContainerRef: ViewContainerRef, readonly cdr: ChangeDetectorRef) {} - checks = 0; + templateExecutions = 0; incrementChecks() { - this.checks++; + this.templateExecutions++; } } - const fixture = TestBed.configureTestingModule({declarations: [AppComponent]}) - .createComponent(AppComponent); - const component = fixture.componentInstance; - fixture.detectChanges(); + let fixture: ComponentFixture; + let component: AppComponent; + let viewRef: EmbeddedViewRef<{}>; + beforeEach(() => { + fixture = TestBed.configureTestingModule({declarations: [AppComponent]}) + .createComponent(AppComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + viewRef = component.templateRef.createEmbeddedView({}); + component.rootViewContainerRef.insert(viewRef); + }); - const viewRef = component.templateRef.createEmbeddedView({}); - // This `ViewContainerRef` is for the root view - component.rootVref.insert(viewRef); - // `detectChanges` on this `ChangeDetectorRef` will refresh this view and children, not the root - // view that has the transplanted `viewRef` inserted. - component.cdr.detectChanges(); - // The template should not have been refreshed because it was inserted "above" the component so - // `detectChanges` will not refresh it. - expect(component.checks).toEqual(0); + it('should not fail when change detecting detached transplanted view', () => { + // This `ViewContainerRef` is for the root view + // `detectChanges` on this `ChangeDetectorRef` will refresh this view and children, not the + // root view that has the transplanted `viewRef` inserted. + component.cdr.detectChanges(); + // The template should not have been refreshed because it was inserted "above" the component + // so `detectChanges` will not refresh it. + expect(component.templateExecutions).toEqual(0); - // Detach view, manually call `detectChanges`, and verify the template was refreshed - component.rootVref.detach(); - viewRef.detectChanges(); - expect(component.checks).toEqual(1); + // Detach view, manually call `detectChanges`, and verify the template was refreshed + component.rootViewContainerRef.detach(); + viewRef.detectChanges(); + expect(component.templateExecutions).toEqual(1); + }); + + it('should work when change detecting detached transplanted view already marked for refresh', + () => { + // detach the viewRef only. This just removes the LViewFlags.Attached rather than actually + // detaching the view from the container. + viewRef.detach(); + // Calling detectChanges marks transplanted views for check + component.cdr.detectChanges(); + expect(() => { + // Calling detectChanges on the transplanted view itself will clear the refresh flag. It + // _should not_ also attempt to update the parent counters because it's detached and + // should not affect parent counters. + viewRef.detectChanges(); + }).not.toThrow(); + expect(component.templateExecutions).toEqual(1); + }); + + it('should work when re-inserting a previously detached transplanted view marked for refresh', + () => { + // Test case for inserting a view with refresh flag + viewRef.detach(); + // mark transplanted views for check but does not refresh transplanted view because it is + // detached + component.cdr.detectChanges(); + // reattach view itself + viewRef.reattach(); + expect(() => { + // detach and reattach view from ViewContainerRef + component.rootViewContainerRef.detach(); + component.rootViewContainerRef.insert(viewRef); + // calling detectChanges will clear the refresh flag. If the above operations messed up + // the counter, this would fail when attempted to decrement. + fixture.detectChanges(false); + }).not.toThrow(); + expect(component.templateExecutions).toBeGreaterThan(0); + }); + + it('should work when detaching an attached transplanted view with the refresh flag', () => { + viewRef.detach(); + // mark transplanted views for check but does not refresh transplanted view because it is + // detached + component.cdr.detectChanges(); + // reattach view with refresh flag should increment parent counters + viewRef.reattach(); + expect(() => { + // detach view with refresh flag should decrement parent counters + viewRef.detach(); + // detectChanges on parent should not cause infinite loop if the above counters were updated + // correctly both times. + fixture.detectChanges(); + }).not.toThrow(); + }); + + it('should work when destroying a view with the refresh flag', () => { + viewRef.detach(); + // mark transplanted views for check but does not refresh transplanted view because it is + // detached + component.cdr.detectChanges(); + viewRef.reattach(); + expect(() => { + viewRef.destroy(); + fixture.detectChanges(); + }).not.toThrow(); + }); }); + describe('when detached', () => { @Component({ selector: 'on-push-component', @@ -743,6 +838,96 @@ describe('change detection for transplanted views', () => { expect(appComponent.transplantedViewRefreshCount).toEqual(3); }); }); + + it('does not cause error if running change detection on detached view', () => { + @Component({ + standalone: true, + selector: 'insertion', + template: ``, + }) + class Insertion { + @ViewChild('vc', {read: ViewContainerRef, static: true}) viewContainer!: ViewContainerRef; + @Input() template!: TemplateRef<{}>; + ngOnChanges() { + return this.viewContainer.createEmbeddedView(this.template); + } + } + + @Component({ + standalone: true, + template: ` + + + `, + imports: [Insertion] + }) + class Root { + readonly cdr = inject(ChangeDetectorRef); + } + + const fixture = TestBed.createComponent(Root); + fixture.componentInstance.cdr.detach(); + fixture.componentInstance.cdr.detectChanges(); + }); + + it('backwards reference still updated if detaching root during change detection', () => { + @Component({ + standalone: true, + selector: 'insertion', + template: ``, + changeDetection: ChangeDetectionStrategy.OnPush + }) + class Insertion { + @ViewChild('vc', {read: ViewContainerRef, static: true}) viewContainer!: ViewContainerRef; + @Input() template!: TemplateRef<{}>; + ngOnChanges() { + return this.viewContainer.createEmbeddedView(this.template); + } + } + + @Component({ + template: '{{value}}', + selector: 'declaration', + standalone: true, + }) + class Declaration { + @ViewChild('template', {static: true}) transplantedTemplate!: TemplateRef<{}>; + @Input() value?: string; + } + + @Component({ + standalone: true, + template: ` + + + {{incrementChecks()}} + `, + imports: [Insertion, Declaration] + }) + class Root { + @ViewChild(Declaration, {static: true}) declaration!: Declaration; + readonly cdr = inject(ChangeDetectorRef); + value = 'initial'; + templateExecutions = 0; + incrementChecks() { + this.templateExecutions++; + } + } + + const fixture = TestBed.createComponent(Root); + fixture.detectChanges(false); + expect(fixture.nativeElement.innerText).toEqual('initial'); + expect(fixture.componentInstance.templateExecutions).toEqual(1); + + // Root is detached and value in transplanted view updates during CD. Because it is inserted + // backwards, this requires a rerun of the traversal at the root. This test ensures we still + // get the rerun even when the root is detached. + fixture.componentInstance.cdr.detach(); + fixture.componentInstance.value = 'new'; + fixture.componentInstance.cdr.detectChanges(); + expect(fixture.componentInstance.templateExecutions).toEqual(2); + expect(fixture.nativeElement.innerText).toEqual('new'); + }); }); }); diff --git a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json index df5b1a0619c..4ca4f24c3d0 100644 --- a/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations-standalone/bundle.golden_symbols.json @@ -245,6 +245,9 @@ { "name": "GenericBrowserDomAdapter" }, + { + "name": "HAS_CHILD_VIEWS_TO_REFRESH" + }, { "name": "HAS_TRANSPLANTED_VIEWS" }, @@ -713,9 +716,6 @@ { "name": "cleanUpView" }, - { - "name": "clearViewRefreshFlag" - }, { "name": "cloakAndComputeStyles" }, @@ -834,7 +834,7 @@ "name": "detectChangesInEmbeddedViews" }, { - "name": "detectChangesInView" + "name": "detectChangesInViewIfAttached" }, { "name": "detectChangesInternal" @@ -1232,6 +1232,9 @@ { "name": "map" }, + { + "name": "markAncestorsForTraversal" + }, { "name": "markAsComponentHost" }, @@ -1491,10 +1494,10 @@ "name": "updateMicroTaskStatus" }, { - "name": "updateViewsToRefresh" + "name": "urlParsingNode" }, { - "name": "urlParsingNode" + "name": "viewAttachedToChangeDetector" }, { "name": "visitDslNode" diff --git a/packages/core/test/bundling/animations/bundle.golden_symbols.json b/packages/core/test/bundling/animations/bundle.golden_symbols.json index 1d278a38bed..b388ab135e0 100644 --- a/packages/core/test/bundling/animations/bundle.golden_symbols.json +++ b/packages/core/test/bundling/animations/bundle.golden_symbols.json @@ -269,6 +269,9 @@ { "name": "GenericBrowserDomAdapter" }, + { + "name": "HAS_CHILD_VIEWS_TO_REFRESH" + }, { "name": "HAS_TRANSPLANTED_VIEWS" }, @@ -773,9 +776,6 @@ { "name": "cleanUpView" }, - { - "name": "clearViewRefreshFlag" - }, { "name": "cloakAndComputeStyles" }, @@ -900,7 +900,7 @@ "name": "detectChangesInEmbeddedViews" }, { - "name": "detectChangesInView" + "name": "detectChangesInViewIfAttached" }, { "name": "detectChangesInternal" @@ -1301,6 +1301,9 @@ { "name": "map" }, + { + "name": "markAncestorsForTraversal" + }, { "name": "markAsComponentHost" }, @@ -1566,10 +1569,10 @@ "name": "updateMicroTaskStatus" }, { - "name": "updateViewsToRefresh" + "name": "urlParsingNode" }, { - "name": "urlParsingNode" + "name": "viewAttachedToChangeDetector" }, { "name": "visitDslNode" diff --git a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json index 72418860918..339526394f3 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -164,6 +164,9 @@ { "name": "GenericBrowserDomAdapter" }, + { + "name": "HAS_CHILD_VIEWS_TO_REFRESH" + }, { "name": "HAS_TRANSPLANTED_VIEWS" }, @@ -575,9 +578,6 @@ { "name": "cleanUpView" }, - { - "name": "clearViewRefreshFlag" - }, { "name": "collectNativeNodes" }, @@ -669,7 +669,7 @@ "name": "detectChangesInEmbeddedViews" }, { - "name": "detectChangesInView" + "name": "detectChangesInViewIfAttached" }, { "name": "detectChangesInternal" @@ -1022,6 +1022,9 @@ { "name": "map" }, + { + "name": "markAncestorsForTraversal" + }, { "name": "markAsComponentHost" }, @@ -1239,10 +1242,10 @@ "name": "updateMicroTaskStatus" }, { - "name": "updateViewsToRefresh" + "name": "urlParsingNode" }, { - "name": "urlParsingNode" + "name": "viewAttachedToChangeDetector" }, { "name": "walkProviderTree" diff --git a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json index ce3ecc58c9f..2541c49cac9 100644 --- a/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_reactive/bundle.golden_symbols.json @@ -242,6 +242,9 @@ { "name": "GenericBrowserDomAdapter" }, + { + "name": "HAS_CHILD_VIEWS_TO_REFRESH" + }, { "name": "HAS_TRANSPLANTED_VIEWS" }, @@ -779,9 +782,6 @@ { "name": "cleanUpView" }, - { - "name": "clearViewRefreshFlag" - }, { "name": "collectNativeNodes" }, @@ -912,7 +912,7 @@ "name": "detectChangesInEmbeddedViews" }, { - "name": "detectChangesInView" + "name": "detectChangesInViewIfAttached" }, { "name": "detectChangesInternal" @@ -1409,6 +1409,9 @@ { "name": "map" }, + { + "name": "markAncestorsForTraversal" + }, { "name": "markAsComponentHost" }, @@ -1718,6 +1721,9 @@ { "name": "unwrapRNode" }, + { + "name": "updateAncestorTraversalFlagsOnAttach" + }, { "name": "updateControl" }, @@ -1725,10 +1731,10 @@ "name": "updateMicroTaskStatus" }, { - "name": "updateViewsToRefresh" + "name": "urlParsingNode" }, { - "name": "urlParsingNode" + "name": "viewAttachedToChangeDetector" }, { "name": "walkProviderTree" 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 index c54997f8a3d..c6f8a4f81f5 100644 --- a/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json +++ b/packages/core/test/bundling/forms_template_driven/bundle.golden_symbols.json @@ -227,6 +227,9 @@ { "name": "GenericBrowserDomAdapter" }, + { + "name": "HAS_CHILD_VIEWS_TO_REFRESH" + }, { "name": "HAS_TRANSPLANTED_VIEWS" }, @@ -758,9 +761,6 @@ { "name": "cleanUpView" }, - { - "name": "clearViewRefreshFlag" - }, { "name": "collectNativeNodes" }, @@ -882,7 +882,7 @@ "name": "detectChangesInEmbeddedViews" }, { - "name": "detectChangesInView" + "name": "detectChangesInViewIfAttached" }, { "name": "detectChangesInternal" @@ -1367,6 +1367,9 @@ { "name": "map" }, + { + "name": "markAncestorsForTraversal" + }, { "name": "markAsComponentHost" }, @@ -1694,6 +1697,9 @@ { "name": "unwrapRNode" }, + { + "name": "updateAncestorTraversalFlagsOnAttach" + }, { "name": "updateControl" }, @@ -1701,10 +1707,10 @@ "name": "updateMicroTaskStatus" }, { - "name": "updateViewsToRefresh" + "name": "urlParsingNode" }, { - "name": "urlParsingNode" + "name": "viewAttachedToChangeDetector" }, { "name": "walkProviderTree" diff --git a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json index 4599632798d..abb62e44cc2 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -104,6 +104,9 @@ { "name": "GenericBrowserDomAdapter" }, + { + "name": "HAS_CHILD_VIEWS_TO_REFRESH" + }, { "name": "HAS_TRANSPLANTED_VIEWS" }, @@ -440,9 +443,6 @@ { "name": "cleanUpView" }, - { - "name": "clearViewRefreshFlag" - }, { "name": "collectNativeNodes" }, @@ -528,7 +528,7 @@ "name": "detectChangesInEmbeddedViews" }, { - "name": "detectChangesInView" + "name": "detectChangesInViewIfAttached" }, { "name": "detectChangesInternal" @@ -806,6 +806,9 @@ { "name": "map" }, + { + "name": "markAncestorsForTraversal" + }, { "name": "markViewDirty" }, @@ -984,10 +987,10 @@ "name": "updateMicroTaskStatus" }, { - "name": "updateViewsToRefresh" + "name": "urlParsingNode" }, { - "name": "urlParsingNode" + "name": "viewAttachedToChangeDetector" }, { "name": "walkProviderTree" diff --git a/packages/core/test/bundling/hydration/bundle.golden_symbols.json b/packages/core/test/bundling/hydration/bundle.golden_symbols.json index 4d68a081e7e..4468b84fc62 100644 --- a/packages/core/test/bundling/hydration/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hydration/bundle.golden_symbols.json @@ -185,6 +185,9 @@ { "name": "GenericBrowserDomAdapter" }, + { + "name": "HAS_CHILD_VIEWS_TO_REFRESH" + }, { "name": "HAS_TRANSPLANTED_VIEWS" }, @@ -635,9 +638,6 @@ { "name": "clearElementContents" }, - { - "name": "clearViewRefreshFlag" - }, { "name": "collectNativeNodes" }, @@ -726,7 +726,7 @@ "name": "detectChangesInEmbeddedViews" }, { - "name": "detectChangesInView" + "name": "detectChangesInViewIfAttached" }, { "name": "detectChangesInternal" @@ -1094,6 +1094,9 @@ { "name": "map" }, + { + "name": "markAncestorsForTraversal" + }, { "name": "markViewDirty" }, @@ -1320,10 +1323,10 @@ "name": "updateMicroTaskStatus" }, { - "name": "updateViewsToRefresh" + "name": "urlParsingNode" }, { - "name": "urlParsingNode" + "name": "viewAttachedToChangeDetector" }, { "name": "walkProviderTree" diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index d18a246e22e..80b9dd7a73b 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -272,6 +272,9 @@ { "name": "GuardsCheckStart" }, + { + "name": "HAS_CHILD_VIEWS_TO_REFRESH" + }, { "name": "HAS_TRANSPLANTED_VIEWS" }, @@ -980,9 +983,6 @@ { "name": "cleanUpView" }, - { - "name": "clearViewRefreshFlag" - }, { "name": "collectNativeNodes" }, @@ -1167,7 +1167,7 @@ "name": "detectChangesInEmbeddedViews" }, { - "name": "detectChangesInView" + "name": "detectChangesInViewIfAttached" }, { "name": "detectChangesInternal" @@ -1700,6 +1700,9 @@ { "name": "map" }, + { + "name": "markAncestorsForTraversal" + }, { "name": "markAsComponentHost" }, @@ -2066,6 +2069,9 @@ { "name": "unwrapSafeValue" }, + { + "name": "updateAncestorTraversalFlagsOnAttach" + }, { "name": "updateMicroTaskStatus" }, @@ -2076,10 +2082,10 @@ "name": "updateSegmentGroupChildren" }, { - "name": "updateViewsToRefresh" + "name": "urlParsingNode" }, { - "name": "urlParsingNode" + "name": "viewAttachedToChangeDetector" }, { "name": "walkProviderTree" diff --git a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json index 180a316824d..9f8c2ecd961 100644 --- a/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json +++ b/packages/core/test/bundling/standalone_bootstrap/bundle.golden_symbols.json @@ -146,6 +146,9 @@ { "name": "GenericBrowserDomAdapter" }, + { + "name": "HAS_CHILD_VIEWS_TO_REFRESH" + }, { "name": "HAS_TRANSPLANTED_VIEWS" }, @@ -515,9 +518,6 @@ { "name": "cleanUpView" }, - { - "name": "clearViewRefreshFlag" - }, { "name": "collectNativeNodes" }, @@ -597,7 +597,7 @@ "name": "detectChangesInEmbeddedViews" }, { - "name": "detectChangesInView" + "name": "detectChangesInViewIfAttached" }, { "name": "detectChangesInternal" @@ -899,6 +899,9 @@ { "name": "map" }, + { + "name": "markAncestorsForTraversal" + }, { "name": "markViewDirty" }, @@ -1089,10 +1092,10 @@ "name": "updateMicroTaskStatus" }, { - "name": "updateViewsToRefresh" + "name": "urlParsingNode" }, { - "name": "urlParsingNode" + "name": "viewAttachedToChangeDetector" }, { "name": "walkProviderTree" diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index fe61840afa2..b1adad7c6d1 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -167,6 +167,9 @@ { "name": "GenericBrowserDomAdapter" }, + { + "name": "HAS_CHILD_VIEWS_TO_REFRESH" + }, { "name": "HAS_TRANSPLANTED_VIEWS" }, @@ -683,9 +686,6 @@ { "name": "cleanUpView" }, - { - "name": "clearViewRefreshFlag" - }, { "name": "collectNativeNodes" }, @@ -798,7 +798,7 @@ "name": "detectChangesInEmbeddedViews" }, { - "name": "detectChangesInView" + "name": "detectChangesInViewIfAttached" }, { "name": "detectChangesInternal" @@ -1232,6 +1232,9 @@ { "name": "map" }, + { + "name": "markAncestorsForTraversal" + }, { "name": "markAsComponentHost" }, @@ -1484,14 +1487,17 @@ { "name": "unwrapRNode" }, + { + "name": "updateAncestorTraversalFlagsOnAttach" + }, { "name": "updateMicroTaskStatus" }, { - "name": "updateViewsToRefresh" + "name": "urlParsingNode" }, { - "name": "urlParsingNode" + "name": "viewAttachedToChangeDetector" }, { "name": "walkProviderTree"