From c8c2272a9f2aec7ef27dc8dc7c3a44d6f75b3cbb Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Mon, 13 Jan 2020 15:12:16 -0800 Subject: [PATCH] fix(core): Refresh transplanted views at insertion point only (#35968) Only refresh transplanted views at the insertion location in Ivy. Previously, Ivy would check transplanted views at both the insertion and declaration points. This is achieved by adding a marker to the insertion tree when we encounter a transplanted view that needs to be refreshed at its declaration. We use this marker as an extra indication that we still need to descend and refresh those transplanted views at their insertion locations even if the insertion view and/or its parents are not dirty. This change fixes several issues: * Transplanted views refreshed twice if both insertion and declaration are dirty. This could be an error if the insertion component changes result in data not being available to the transplanted view because it is slated to be removed. * CheckAlways transplanted views not refreshed if shielded by non-dirty OnPush (fixes #35400) * Transplanted views still refreshed when insertion tree is detached (fixes #21324) PR Close #35968 --- goldens/size-tracking/aio-payloads.json | 2 +- .../size-tracking/integration-payloads.json | 12 +- .../core/src/render3/instructions/shared.ts | 185 ++++-- .../core/src/render3/interfaces/container.ts | 18 +- packages/core/src/render3/interfaces/view.ts | 27 +- .../core/src/render3/node_manipulation.ts | 35 +- packages/core/src/render3/util/view_utils.ts | 24 +- .../test/acceptance/change_detection_spec.ts | 166 ----- ...change_detection_transplanted_view_spec.ts | 601 ++++++++++++++++++ .../cyclic_import/bundle.golden_symbols.json | 21 +- .../hello_world/bundle.golden_symbols.json | 21 +- .../bundling/todo/bundle.golden_symbols.json | 21 +- 12 files changed, 859 insertions(+), 274 deletions(-) create mode 100644 packages/core/test/acceptance/change_detection_transplanted_view_spec.ts diff --git a/goldens/size-tracking/aio-payloads.json b/goldens/size-tracking/aio-payloads.json index e10ce7bf43..68ba71ad60 100755 --- a/goldens/size-tracking/aio-payloads.json +++ b/goldens/size-tracking/aio-payloads.json @@ -12,7 +12,7 @@ "master": { "uncompressed": { "runtime-es2015": 2987, - "main-es2015": 452060, + "main-es2015": 452876, "polyfills-es2015": 52195 } } diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json index 18be2ed2aa..e62b88baee 100644 --- a/goldens/size-tracking/integration-payloads.json +++ b/goldens/size-tracking/integration-payloads.json @@ -3,7 +3,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 142073, + "main-es2015": 142794, "polyfills-es2015": 36657 } } @@ -12,7 +12,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 16514, + "main-es2015": 16959, "polyfills-es2015": 36657 } } @@ -21,7 +21,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 148196, + "main-es2015": 148932, "polyfills-es2015": 36657 } } @@ -30,7 +30,7 @@ "master": { "uncompressed": { "runtime-es2015": 1485, - "main-es2015": 137897, + "main-es2015": 138343, "polyfills-es2015": 37334 } } @@ -39,7 +39,7 @@ "master": { "uncompressed": { "runtime-es2015": 2289, - "main-es2015": 247727, + "main-es2015": 248671, "polyfills-es2015": 36657, "5-es2015": 751 } @@ -49,7 +49,7 @@ "master": { "uncompressed": { "runtime-es2015": 2289, - "main-es2015": 227258, + "main-es2015": 227677, "polyfills-es2015": 36657, "5-es2015": 779 } diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index d5b18c87c8..3796cf1fa6 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -28,15 +28,15 @@ import {AttributeMarker, InitialInputData, InitialInputs, LocalRefExtractor, Pro import {isProceduralRenderer, RComment, RElement, Renderer3, RendererFactory3, RNode, RText} from '../interfaces/renderer'; import {SanitizerFn} from '../interfaces/sanitization'; import {isComponentDef, isComponentHost, isContentQueryHost, isLContainer, isRootView} from '../interfaces/type_checks'; -import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, InitPhaseState, INJECTOR, LView, LViewFlags, NEXT, PARENT, RENDERER, RENDERER_FACTORY, RootContext, RootContextFlags, SANITIZER, T_HOST, TData, TVIEW, TView, TViewType} from '../interfaces/view'; +import {CHILD_HEAD, CHILD_TAIL, CLEANUP, CONTEXT, DECLARATION_COMPONENT_VIEW, DECLARATION_VIEW, FLAGS, HEADER_OFFSET, HOST, InitPhaseState, INJECTOR, LView, LViewFlags, NEXT, PARENT, RENDERER, RENDERER_FACTORY, RootContext, RootContextFlags, SANITIZER, T_HOST, TData, TRANSPLANTED_VIEWS_TO_REFRESH, TVIEW, TView, TViewType} from '../interfaces/view'; import {assertNodeOfPossibleTypes} from '../node_assert'; import {isInlineTemplate, isNodeMatchingSelectorList} from '../node_selector_matcher'; -import {enterView, getBindingsEnabled, getCheckNoChangesMode, getCurrentDirectiveIndex, getIsParent, getPreviousOrParentTNode, getSelectedIndex, getTView, leaveView, setBindingIndex, setBindingRootForHostBindings, setCheckNoChangesMode, setCurrentDirectiveIndex, setCurrentQueryIndex, setPreviousOrParentTNode, setSelectedIndex} from '../state'; +import {enterView, getBindingsEnabled, getCheckNoChangesMode, getCurrentDirectiveIndex, getIsParent, getPreviousOrParentTNode, getSelectedIndex, leaveView, setBindingIndex, setBindingRootForHostBindings, setCheckNoChangesMode, setCurrentDirectiveIndex, setCurrentQueryIndex, setPreviousOrParentTNode, setSelectedIndex} from '../state'; import {NO_CHANGE} from '../tokens'; import {isAnimationProp, mergeHostAttrs} from '../util/attrs_utils'; import {INTERPOLATION_DELIMITER, renderStringify, stringifyForError} from '../util/misc_utils'; import {getLViewParent} from '../util/view_traversal_utils'; -import {getComponentLViewByIndex, getNativeByIndex, getNativeByTNode, getTNode, isCreationMode, readPatchedLView, resetPreOrderHookFlags, unwrapLView, viewAttachedToChangeDetector} from '../util/view_utils'; +import {getComponentLViewByIndex, getNativeByIndex, getNativeByTNode, isCreationMode, readPatchedLView, resetPreOrderHookFlags, unwrapLView, updateTransplantedViewCount, viewAttachedToChangeDetector} from '../util/view_utils'; import {selectIndexInternal} from './advance'; import {attachLContainerDebug, attachLViewDebug, cloneToLViewFromTViewBlueprint, cloneToTViewData, LCleanup, LViewBlueprint, MatchesArray, TCleanup, TNodeDebug, TNodeInitialInputs, TNodeLocalNames, TViewComponents, TViewConstructor} from './lview_debug'; @@ -431,6 +431,10 @@ export function refreshView( } } + // 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); refreshDynamicEmbeddedViews(lView); // Content query results must be refreshed before content hooks are called. @@ -507,6 +511,10 @@ export function refreshView( if (!checkNoChangesMode) { lView[FLAGS] &= ~(LViewFlags.Dirty | LViewFlags.FirstLViewPass); } + if (lView[FLAGS] & LViewFlags.RefreshTransplantedView) { + lView[FLAGS] &= ~LViewFlags.RefreshTransplantedView; + updateTransplantedViewCount(lView[PARENT] as LContainer, -1); + } } finally { leaveView(); } @@ -1600,85 +1608,94 @@ export function createLContainer( ActiveIndexFlag.DYNAMIC_EMBEDDED_VIEWS_ONLY << ActiveIndexFlag.SHIFT, // active index currentView, // parent null, // next - null, // queries - tNode, // t_host - native, // native, - null, // view refs + 0, // transplanted views to refresh count + tNode, // t_host + native, // native, + null, // view refs + null, // moved views ); + ngDevMode && + assertEqual( + lContainer.length, CONTAINER_HEADER_OFFSET, + 'Should allocate correct number of slots for LContainer header.'); ngDevMode && attachLContainerDebug(lContainer); return lContainer; } - /** * Goes over dynamic embedded views (ones created through ViewContainerRef APIs) and refreshes * them by executing an associated template function. */ function refreshDynamicEmbeddedViews(lView: LView) { - let viewOrContainer = lView[CHILD_HEAD]; - while (viewOrContainer !== null) { - // Note: viewOrContainer can be an LView or an LContainer instance, but here we are only - // interested in LContainer - let activeIndexFlag: ActiveIndexFlag; - if (isLContainer(viewOrContainer) && - (activeIndexFlag = viewOrContainer[ACTIVE_INDEX]) >> ActiveIndexFlag.SHIFT === - ActiveIndexFlag.DYNAMIC_EMBEDDED_VIEWS_ONLY) { - for (let i = CONTAINER_HEADER_OFFSET; i < viewOrContainer.length; i++) { - const embeddedLView = viewOrContainer[i] as LView; - const embeddedTView = embeddedLView[TVIEW]; - ngDevMode && assertDefined(embeddedTView, 'TView must be allocated'); - if (viewAttachedToChangeDetector(embeddedLView)) { - refreshView( - embeddedTView, embeddedLView, embeddedTView.template, embeddedLView[CONTEXT]!); - } - } - if ((activeIndexFlag & ActiveIndexFlag.HAS_TRANSPLANTED_VIEWS) !== 0) { - // We should only CD moved views if the component where they were inserted does not match - // the component where they were declared and insertion is on-push. Moved views also - // contains intra component moves, or check-always which need to be skipped. - refreshTransplantedViews(viewOrContainer, lView[DECLARATION_COMPONENT_VIEW]!); + for (let lContainer = getFirstLContainer(lView); lContainer !== null; + lContainer = getNextLContainer(lContainer)) { + for (let i = CONTAINER_HEADER_OFFSET; i < lContainer.length; i++) { + const embeddedLView = lContainer[i]; + const embeddedTView = embeddedLView[TVIEW]; + ngDevMode && assertDefined(embeddedTView, 'TView must be allocated'); + if (viewAttachedToChangeDetector(embeddedLView)) { + refreshView(embeddedTView, embeddedLView, embeddedTView.template, embeddedLView[CONTEXT]!); } } - viewOrContainer = viewOrContainer[NEXT]; } } +/** + * Gets the first `LContainer` in the LView or `null` if none exists. + */ +function getFirstLContainer(lView: LView): LContainer|null { + let viewOrContainer = lView[CHILD_HEAD]; + while (viewOrContainer !== null && + !(isLContainer(viewOrContainer) && + viewOrContainer[ACTIVE_INDEX] >> ActiveIndexFlag.SHIFT === + ActiveIndexFlag.DYNAMIC_EMBEDDED_VIEWS_ONLY)) { + viewOrContainer = viewOrContainer[NEXT]; + } + return viewOrContainer; +} /** - * Refresh transplanted LViews. + * Gets the next `LContainer` that is a sibling of the given container. + */ +function getNextLContainer(container: LContainer): LContainer|null { + let viewOrContainer = container[NEXT]; + while (viewOrContainer !== null && + !(isLContainer(viewOrContainer) && + viewOrContainer[ACTIVE_INDEX] >> ActiveIndexFlag.SHIFT === + ActiveIndexFlag.DYNAMIC_EMBEDDED_VIEWS_ONLY)) { + viewOrContainer = viewOrContainer[NEXT]; + } + return viewOrContainer; +} + +/** + * Mark transplanted views as needing to be refreshed at their insertion points. * * See: `ActiveIndexFlag.HAS_TRANSPLANTED_VIEWS` and `LView[DECLARATION_COMPONENT_VIEW]` for * explanation of transplanted views. * - * @param lContainer The `LContainer` which has transplanted views. - * @param declaredComponentLView The `lContainer` parent component `LView`. + * @param lView The `LView` that may have transplanted views. */ -function refreshTransplantedViews(lContainer: LContainer, declaredComponentLView: LView) { - const movedViews = lContainer[MOVED_VIEWS]!; - ngDevMode && assertDefined(movedViews, 'Transplanted View flags set but missing MOVED_VIEWS'); - for (let i = 0; i < movedViews.length; i++) { - const movedLView = movedViews[i]!; - const insertionLContainer = movedLView[PARENT] as LContainer; - ngDevMode && assertLContainer(insertionLContainer); - const insertedComponentLView = insertionLContainer[PARENT][DECLARATION_COMPONENT_VIEW]!; - ngDevMode && assertDefined(insertedComponentLView, 'Missing LView'); - // Check if we have a transplanted view by compering declaration and insertion location. - if (insertedComponentLView !== declaredComponentLView) { - // Yes the `LView` is transplanted. - // Here we would like to know if the component is `OnPush`. We don't have - // explicit `OnPush` flag instead we set `CheckAlways` to false (which is `OnPush`) - // Not to be confused with `ManualOnPush` which is used with wether a DOM event - // should automatically mark a view as dirty. - const insertionComponentIsOnPush = - (insertedComponentLView[FLAGS] & LViewFlags.CheckAlways) === 0; - if (insertionComponentIsOnPush) { - // Here we know that the template has been transplanted across components and is - // on-push (not just moved within a component). If the insertion is marked dirty, then - // there is no need to CD here as we will do it again later when we get to insertion - // point. - const movedTView = movedLView[TVIEW]; - ngDevMode && assertDefined(movedTView, 'TView must be allocated'); - refreshView(movedTView, movedLView, movedTView.template, movedLView[CONTEXT]!); +function markTransplantedViewsForRefresh(lView: LView) { + for (let lContainer = getFirstLContainer(lView); lContainer !== null; + lContainer = getNextLContainer(lContainer)) { + if ((lContainer[ACTIVE_INDEX] & ActiveIndexFlag.HAS_TRANSPLANTED_VIEWS) !== 0) { + const movedViews = lContainer[MOVED_VIEWS]!; + ngDevMode && assertDefined(movedViews, 'Transplanted View flags set but missing MOVED_VIEWS'); + for (let i = 0; i < movedViews.length; i++) { + const movedLView = movedViews[i]!; + const insertionLContainer = movedLView[PARENT] as LContainer; + ngDevMode && assertLContainer(insertionLContainer); + // We don't want to increment the counter if the moved LView was already marked for + // refresh. + if ((movedLView[FLAGS] & LViewFlags.RefreshTransplantedView) === 0) { + updateTransplantedViewCount(insertionLContainer, 1); + } + // Note, it is possible that the `movedViews` is tracking views that are transplanted *and* + // those that aren't (declaration component === insertion component). In the latter case, + // it's fine to add the flag, as we will clear it immediately in + // `refreshDynamicEmbeddedViews` for the view currently being refreshed. + movedLView[FLAGS] |= LViewFlags.RefreshTransplantedView; } } } @@ -1695,10 +1712,50 @@ function refreshComponent(hostLView: LView, componentHostIdx: number): void { ngDevMode && assertEqual(isCreationMode(hostLView), false, 'Should be run in update mode'); const componentView = getComponentLViewByIndex(componentHostIdx, hostLView); // Only attached components that are CheckAlways or OnPush and dirty should be refreshed - if (viewAttachedToChangeDetector(componentView) && - componentView[FLAGS] & (LViewFlags.CheckAlways | LViewFlags.Dirty)) { - const componentTView = componentView[TVIEW]; - refreshView(componentTView, componentView, componentTView.template, componentView[CONTEXT]); + if (viewAttachedToChangeDetector(componentView)) { + const tView = componentView[TVIEW]; + if (componentView[FLAGS] & (LViewFlags.CheckAlways | LViewFlags.Dirty)) { + refreshView(tView, componentView, tView.template, componentView[CONTEXT]); + } else if (componentView[TRANSPLANTED_VIEWS_TO_REFRESH] > 0) { + // Only attached components that are CheckAlways or OnPush and dirty should be refreshed + refreshContainsDirtyView(componentView); + } + } +} + +/** + * Refreshes all transplanted views marked with `LViewFlags.RefreshTransplantedView` that are + * children or descendants of the given lView. + * + * @param lView The lView which contains descendant transplanted views that need to be refreshed. + */ +function refreshContainsDirtyView(lView: LView) { + for (let lContainer = getFirstLContainer(lView); lContainer !== null; + lContainer = getNextLContainer(lContainer)) { + for (let i = CONTAINER_HEADER_OFFSET; i < lContainer.length; i++) { + const embeddedLView = lContainer[i]; + if (embeddedLView[FLAGS] & LViewFlags.RefreshTransplantedView) { + const embeddedTView = embeddedLView[TVIEW]; + ngDevMode && assertDefined(embeddedTView, 'TView must be allocated'); + refreshView(embeddedTView, embeddedLView, embeddedTView.template, embeddedLView[CONTEXT]!); + } else if (embeddedLView[TRANSPLANTED_VIEWS_TO_REFRESH] > 0) { + refreshContainsDirtyView(embeddedLView); + } + } + } + + const tView = lView[TVIEW]; + // Refresh child component views. + const components = tView.components; + if (components !== null) { + for (let i = 0; i < components.length; i++) { + const componentView = getComponentLViewByIndex(components[i], lView); + // Only attached components that are CheckAlways or OnPush and dirty should be refreshed + if (viewAttachedToChangeDetector(componentView) && + componentView[TRANSPLANTED_VIEWS_TO_REFRESH] > 0) { + refreshContainsDirtyView(componentView); + } + } } } diff --git a/packages/core/src/render3/interfaces/container.ts b/packages/core/src/render3/interfaces/container.ts index 44e292daf2..b4d1e07369 100644 --- a/packages/core/src/render3/interfaces/container.ts +++ b/packages/core/src/render3/interfaces/container.ts @@ -10,8 +10,8 @@ import {ViewRef} from '../../linker/view_ref'; import {TNode} from './node'; import {RComment, RElement} from './renderer'; +import {HOST, LView, NEXT, PARENT, T_HOST, TRANSPLANTED_VIEWS_TO_REFRESH} from './view'; -import {HOST, LView, NEXT, PARENT, T_HOST} from './view'; /** @@ -27,16 +27,16 @@ export const TYPE = 1; */ export const ACTIVE_INDEX = 2; -// PARENT and NEXT are indices 3 and 4 +// PARENT, NEXT, TRANSPLANTED_VIEWS_TO_REFRESH are indices 3, 4, and 5 // As we already have these constants in LView, we don't need to re-create them. -export const MOVED_VIEWS = 5; - // T_HOST is index 6 // We already have this constants in LView, we don't need to re-create it. export const NATIVE = 7; export const VIEW_REFS = 8; +export const MOVED_VIEWS = 9; + /** * Size of LContainer's header. Represents the index after which all views in the @@ -44,7 +44,7 @@ export const VIEW_REFS = 8; * which views are already in the DOM (and don't need to be re-added) and so we can * remove views from the DOM when they are no longer required. */ -export const CONTAINER_HEADER_OFFSET = 9; +export const CONTAINER_HEADER_OFFSET = 10; /** @@ -132,6 +132,14 @@ 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`. + */ + [TRANSPLANTED_VIEWS_TO_REFRESH]: number; + /** * A collection of views created based on the underlying `` element but inserted into * a different `LContainer`. We need to track views created from a given declaration point since diff --git a/packages/core/src/render3/interfaces/view.ts b/packages/core/src/render3/interfaces/view.ts index 4fc01b3f2b..0d02144c84 100644 --- a/packages/core/src/render3/interfaces/view.ts +++ b/packages/core/src/render3/interfaces/view.ts @@ -31,7 +31,7 @@ export const TVIEW = 1; export const FLAGS = 2; export const PARENT = 3; export const NEXT = 4; -export const QUERIES = 5; +export const TRANSPLANTED_VIEWS_TO_REFRESH = 5; export const T_HOST = 6; export const CLEANUP = 7; export const CONTEXT = 8; @@ -45,8 +45,9 @@ export const DECLARATION_VIEW = 15; export const DECLARATION_COMPONENT_VIEW = 16; export const DECLARATION_LCONTAINER = 17; export const PREORDER_HOOK_FLAGS = 18; +export const QUERIES = 19; /** Size of LView's header. Necessary to adjust for it when setting slots. */ -export const HEADER_OFFSET = 19; +export const HEADER_OFFSET = 20; // This interface replaces the real LView interface if it is an arg or a @@ -282,6 +283,14 @@ export interface LView extends Array { * More flags for this view. See PreOrderHookFlags for more info. */ [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`. + */ + [TRANSPLANTED_VIEWS_TO_REFRESH]: number; } /** Flags associated with an LView (saved in LView[FLAGS]) */ @@ -342,11 +351,17 @@ export const enum LViewFlags { IsRoot = 0b001000000000, /** - * Index of the current init phase on last 22 bits + * Whether this moved LView was needs to be refreshed at the insertion location because the + * declaration was dirty. */ - IndexWithinInitPhaseIncrementer = 0b010000000000, - IndexWithinInitPhaseShift = 10, - IndexWithinInitPhaseReset = 0b001111111111, + RefreshTransplantedView = 0b0010000000000, + + /** + * Index of the current init phase on last 21 bits + */ + IndexWithinInitPhaseIncrementer = 0b0100000000000, + IndexWithinInitPhaseShift = 11, + IndexWithinInitPhaseReset = 0b0011111111111, } /** diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts index ce5315440c..373c20a97c 100644 --- a/packages/core/src/render3/node_manipulation.ts +++ b/packages/core/src/render3/node_manipulation.ts @@ -23,7 +23,7 @@ import {isLContainer, isLView} from './interfaces/type_checks'; import {CHILD_HEAD, CLEANUP, DECLARATION_COMPONENT_VIEW, DECLARATION_LCONTAINER, DestroyHookData, FLAGS, HookData, HookFn, HOST, LView, LViewFlags, NEXT, PARENT, QUERIES, RENDERER, T_HOST, TVIEW, TView, unusedValueExportToPlacateAjd as unused5} from './interfaces/view'; import {assertNodeOfPossibleTypes, assertNodeType} from './node_assert'; import {getLViewParent} from './util/view_traversal_utils'; -import {getNativeByTNode, unwrapRNode} from './util/view_utils'; +import {getNativeByTNode, unwrapRNode, updateTransplantedViewCount} from './util/view_utils'; const unusedValueToPlacateAjd = unused1 + unused2 + unused3 + unused4 + unused5; @@ -270,18 +270,13 @@ function trackMovedView(declarationContainer: LContainer, lView: LView) { ngDevMode && assertLContainer(insertedLContainer); const insertedComponentLView = insertedLContainer[PARENT]![DECLARATION_COMPONENT_VIEW]; ngDevMode && assertDefined(insertedComponentLView, 'Missing insertedComponentLView'); - const insertedComponentIsOnPush = - (insertedComponentLView[FLAGS] & LViewFlags.CheckAlways) !== LViewFlags.CheckAlways; - if (insertedComponentIsOnPush) { - const declaredComponentLView = lView[DECLARATION_COMPONENT_VIEW]; - ngDevMode && assertDefined(declaredComponentLView, 'Missing declaredComponentLView'); - if (declaredComponentLView !== insertedComponentLView) { - // At this point the declaration-component is not same as insertion-component and we are in - // on-push mode, this means that this is a transplanted view. Mark the declared lView as - // having - // transplanted views so that those views can participate in CD. - declarationContainer[ACTIVE_INDEX] |= ActiveIndexFlag.HAS_TRANSPLANTED_VIEWS; - } + const declaredComponentLView = lView[DECLARATION_COMPONENT_VIEW]; + ngDevMode && assertDefined(declaredComponentLView, 'Missing declaredComponentLView'); + if (declaredComponentLView !== insertedComponentLView) { + // At this point the declaration-component is not same as insertion-component; this means that + // this is a transplanted view. Mark the declared lView as having transplanted views so that + // those views can participate in CD. + declarationContainer[ACTIVE_INDEX] |= ActiveIndexFlag.HAS_TRANSPLANTED_VIEWS; } if (movedViews === null) { declarationContainer[MOVED_VIEWS] = [lView]; @@ -297,8 +292,18 @@ function detachMovedView(declarationContainer: LContainer, lView: LView) { declarationContainer[MOVED_VIEWS], 'A projected view should belong to a non-empty projected views collection'); const movedViews = declarationContainer[MOVED_VIEWS]!; - const declaredViewIndex = movedViews.indexOf(lView); - movedViews.splice(declaredViewIndex, 1); + 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 decrement the view counter here + // instead. + if (lView[FLAGS] & LViewFlags.RefreshTransplantedView) { + updateTransplantedViewCount(insertionLContainer, -1); + } + + 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 54fe62be01..59a3d4edaa 100644 --- a/packages/core/src/render3/util/view_utils.ts +++ b/packages/core/src/render3/util/view_utils.ts @@ -13,7 +13,7 @@ import {LContext, MONKEY_PATCH_KEY_NAME} from '../interfaces/context'; import {TConstants, TNode} from '../interfaces/node'; import {isProceduralRenderer, RNode} from '../interfaces/renderer'; import {isLContainer, isLView} from '../interfaces/type_checks'; -import {FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, PARENT, PREORDER_HOOK_FLAGS, RENDERER, TData, TView} from '../interfaces/view'; +import {FLAGS, HEADER_OFFSET, HOST, LView, LViewFlags, PARENT, PREORDER_HOOK_FLAGS, RENDERER, TData, TRANSPLANTED_VIEWS_TO_REFRESH, TView} from '../interfaces/view'; @@ -194,4 +194,24 @@ export function getLContainerActiveIndex(lContainer: LContainer) { export function setLContainerActiveIndex(lContainer: LContainer, index: number) { lContainer[ACTIVE_INDEX] = index << ActiveIndexFlag.SHIFT; -} \ No newline at end of file +} + +/** + * Updates the `TRANSPLANTED_VIEWS_TO_REFRESH` counter on the `LContainer` as well as the parents + * whose + * 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 + */ +export function updateTransplantedViewCount(lContainer: LContainer, amount: 1|- 1) { + lContainer[TRANSPLANTED_VIEWS_TO_REFRESH] += amount; + let viewOrContainer: LView|LContainer = lContainer; + let parent: LView|LContainer|null = lContainer[PARENT]; + while (parent !== null && + ((amount === 1 && viewOrContainer[TRANSPLANTED_VIEWS_TO_REFRESH] === 1) || + (amount === -1 && viewOrContainer[TRANSPLANTED_VIEWS_TO_REFRESH] === 0))) { + parent[TRANSPLANTED_VIEWS_TO_REFRESH] += amount; + viewOrContainer = parent; + parent = parent[PARENT]; + } +} diff --git a/packages/core/test/acceptance/change_detection_spec.ts b/packages/core/test/acceptance/change_detection_spec.ts index 9e9643ddbc..df6d67f9c8 100644 --- a/packages/core/test/acceptance/change_detection_spec.ts +++ b/packages/core/test/acceptance/change_detection_spec.ts @@ -9,7 +9,6 @@ import {CommonModule} from '@angular/common'; import {ApplicationRef, ChangeDetectionStrategy, ChangeDetectorRef, Component, ComponentFactoryResolver, ComponentRef, Directive, DoCheck, EmbeddedViewRef, ErrorHandler, Input, NgModule, OnInit, QueryList, TemplateRef, Type, ViewChild, ViewChildren, ViewContainerRef} from '@angular/core'; -import {AfterViewChecked} from '@angular/core/src/core'; import {ComponentFixture, TestBed} from '@angular/core/testing'; import {expect} from '@angular/platform-browser/testing/src/matchers'; import {ivyEnabled} from '@angular/private/testing'; @@ -1227,167 +1226,6 @@ describe('change detection', () => { }); }); - describe('transplanted views', () => { - @Component({ - selector: 'insert-comp', - changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - InsertComp({{greeting}}) -
- - - -
- ` - }) - class InsertComp implements DoCheck, AfterViewChecked { - get template(): TemplateRef { - return declareComp.myTmpl; - } - greeting: string = 'Hello'; - constructor(public changeDetectorRef: ChangeDetectorRef) { - insertComp = this; - } - ngDoCheck(): void { - logValue = 'Insert'; - } - ngAfterViewChecked(): void { - logValue = null; - } - } - - @Component({ - selector: `declare-comp`, - template: ` - DeclareComp({{name}}) - - {{greeting}} {{logName()}}! - - ` - }) - class DeclareComp implements DoCheck, AfterViewChecked { - @ViewChild('myTmpl') myTmpl!: TemplateRef; - name: string = 'world'; - constructor() { - declareComp = this; - } - ngDoCheck(): void { - logValue = 'Declare'; - } - logName() { - // This will log when the embedded view gets CD. The `logValue` will show if the CD was from - // `Insert` or from `Declare` component. - log.push(logValue!); - return this.name; - } - ngAfterViewChecked(): void { - logValue = null; - } - } - - @Component({ - template: ` - - - ` - }) - class AppComp { - showDeclare: boolean = true; - showInsert: boolean = true; - constructor() { - appComp = this; - } - } - - let log!: string[]; - let logValue!: string|null; - let fixture!: ComponentFixture; - let appComp!: AppComp; - let insertComp!: InsertComp; - let declareComp!: DeclareComp; - - beforeEach(() => { - TestBed.configureTestingModule({ - declarations: [InsertComp, DeclareComp, AppComp], - imports: [CommonModule], - }); - log = []; - fixture = TestBed.createComponent(AppComp); - }); - - it('should CD with declaration', () => { - // NOTE: The CD of VE and Ivy is different and is captured in the assertions: - // `expect(log).toEqual(ivyEnabled ? [...] : [...])` - // - // The reason for this difference is in the algorithm which VE and Ivy use to deal with - // transplanted views: - // - VE: always runs CD at insertion point. If the insertion component is `OnPush` and the - // transplanted view is `CheckAlways` then the insertion component will be changed to - // `CheckAlways` (defeating the benefit of `OnPush`) - // - Ivy: Runs the CD at both the declaration as well as insertion point. The benefit of this - // approach is that each side (declaration/insertion) gets to keep its own semantics (either - // `OnPush` or `CheckAlways`). The implication is that: - // 1. The two semantics are slightly different. - // 2. Ivy will CD the transplanted view twice under some circumstances. (When both insertion - // and declaration are both dirty.) - - fixture.detectChanges(false); - expect(log).toEqual(['Insert']); - log.length = 0; - expect(trim(fixture.nativeElement.textContent)) - .toEqual('DeclareComp(world) InsertComp(Hello) Hello world!'); - - declareComp.name = 'Angular'; - fixture.detectChanges(false); - expect(log).toEqual(ivyEnabled ? ['Declare'] : ['Insert']); - log.length = 0; - // Expect transplanted LView to be CD because the declaration is CD. - expect(trim(fixture.nativeElement.textContent)) - .toEqual('DeclareComp(Angular) InsertComp(Hello) Hello Angular!'); - - insertComp.greeting = 'Hi'; - fixture.detectChanges(false); - expect(log).toEqual(ivyEnabled ? ['Declare'] : ['Insert']); - log.length = 0; - // expect no change because it is on push. - expect(trim(fixture.nativeElement.textContent)) - .toEqual('DeclareComp(Angular) InsertComp(Hello) Hello Angular!'); - - insertComp.changeDetectorRef.markForCheck(); - fixture.detectChanges(false); - expect(log).toEqual(ivyEnabled ? ['Declare', 'Insert'] : ['Insert']); - log.length = 0; - expect(trim(fixture.nativeElement.textContent)) - .toEqual('DeclareComp(Angular) InsertComp(Hi) Hi Angular!'); - - // Destroy insertion should also destroy declaration - appComp.showInsert = false; - insertComp.changeDetectorRef.markForCheck(); - fixture.detectChanges(false); - expect(log).toEqual([]); - log.length = 0; - expect(trim(fixture.nativeElement.textContent)).toEqual('DeclareComp(Angular)'); - - // Restore both - appComp.showInsert = true; - fixture.detectChanges(false); - expect(log).toEqual(['Insert']); - log.length = 0; - expect(trim(fixture.nativeElement.textContent)) - .toEqual('DeclareComp(Angular) InsertComp(Hello) Hello Angular!'); - - // Destroy declaration, But we should still be able to see updates in insertion - appComp.showDeclare = false; - insertComp.greeting = 'Hello'; - insertComp.changeDetectorRef.markForCheck(); - fixture.detectChanges(false); - expect(log).toEqual(['Insert']); - log.length = 0; - expect(trim(fixture.nativeElement.textContent)).toEqual('InsertComp(Hello) Hello Angular!'); - }); - }); describe('OnPush markForCheck in lifecycle hooks', () => { describe('with check no changes enabled', () => createOnPushMarkForCheckTests(true)); @@ -1652,7 +1490,3 @@ describe('change detection', () => { // }); }); }); - -function trim(text: string|null): string { - return text ? text.replace(/[\s\n]+/gm, ' ').trim() : ''; -} diff --git a/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts b/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts new file mode 100644 index 0000000000..0f01867817 --- /dev/null +++ b/packages/core/test/acceptance/change_detection_transplanted_view_spec.ts @@ -0,0 +1,601 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {CommonModule} from '@angular/common'; +import {ChangeDetectionStrategy, ChangeDetectorRef, Component, DoCheck, Input, TemplateRef, Type, ViewChild} from '@angular/core'; +import {AfterViewChecked} from '@angular/core/src/core'; +import {ComponentFixture, TestBed} from '@angular/core/testing'; +import {expect} from '@angular/platform-browser/testing/src/matchers'; +import {onlyInIvy} from '@angular/private/testing'; + +describe('change detection for transplanted views', () => { + describe('when declaration appears before insertion', () => { + const insertCompTemplate = ` + InsertComp({{greeting}}) +
+ + + +
+ `; + @Component({ + selector: 'insert-comp', + changeDetection: ChangeDetectionStrategy.OnPush, + template: insertCompTemplate, + }) + class InsertComp implements DoCheck, AfterViewChecked { + get template(): TemplateRef { + return declareComp.myTmpl; + } + greeting: string = 'Hello'; + constructor(public changeDetectorRef: ChangeDetectorRef) { + if (!(this instanceof InsertForOnPushDeclareComp)) { + insertComp = this; + } + } + ngDoCheck(): void { + logValue = 'Insert'; + } + ngAfterViewChecked(): void { + logValue = null; + } + } + + @Component({ + selector: 'insert-for-onpush-declare-comp', + changeDetection: ChangeDetectionStrategy.OnPush, + template: insertCompTemplate, + }) + class InsertForOnPushDeclareComp extends InsertComp { + constructor(public changeDetectorRef: ChangeDetectorRef) { + super(changeDetectorRef); + insertForOnPushDeclareComp = this; + } + get template(): TemplateRef { + return onPushDeclareComp.myTmpl; + } + } + + @Component({ + selector: `declare-comp`, + template: ` + DeclareComp({{name}}) + + {{greeting}} {{logName()}}! + + ` + }) + class DeclareComp implements DoCheck, AfterViewChecked { + @ViewChild('myTmpl') myTmpl!: TemplateRef; + name: string = 'world'; + constructor(readonly changeDetector: ChangeDetectorRef) { + if (!(this instanceof OnPushDeclareComp)) { + declareComp = this; + } + } + ngDoCheck(): void { + logValue = 'Declare'; + } + logName() { + // This will log when the embedded view gets CD. The `logValue` will show if the CD was + // from `Insert` or from `Declare` component. + log.push(logValue!); + return this.name; + } + ngAfterViewChecked(): void { + logValue = null; + } + } + + @Component({ + selector: `onpush-declare-comp`, + template: ` + OnPushDeclareComp({{name}}) + + {{greeting}} {{logName()}}! + + `, + changeDetection: ChangeDetectionStrategy.OnPush + }) + class OnPushDeclareComp extends DeclareComp { + constructor(readonly changeDetector: ChangeDetectorRef) { + super(changeDetector); + onPushDeclareComp = this; + } + } + + + @Component({ + template: ` + + + + + ` + }) + class AppComp { + showDeclare: boolean = false; + showOnPushDeclare: boolean = false; + showInsert: boolean = false; + showInsertForOnPushDeclare: boolean = false; + constructor() { + appComp = this; + } + } + + let log!: Array; + let logValue!: string|null; + let fixture!: ComponentFixture; + let appComp!: AppComp; + let insertComp!: InsertComp; + let insertForOnPushDeclareComp!: InsertForOnPushDeclareComp; + let declareComp!: DeclareComp; + let onPushDeclareComp!: OnPushDeclareComp; + + beforeEach(() => { + TestBed.configureTestingModule({ + declarations: + [InsertComp, DeclareComp, OnPushDeclareComp, InsertForOnPushDeclareComp, AppComp], + imports: [CommonModule], + }); + log = []; + fixture = TestBed.createComponent(AppComp); + }); + + describe('and declaration component is CheckAlways', () => { + beforeEach(() => { + fixture.componentInstance.showDeclare = true; + fixture.componentInstance.showInsert = true; + fixture.detectChanges(false); + log.length = 0; + }); + + it('should set up the component under test correctly', () => { + expect(log.length).toEqual(0); + expect(trim(fixture.nativeElement.textContent)) + .toEqual('DeclareComp(world) InsertComp(Hello) Hello world!'); + }); + + it('should CD at insertion point only', () => { + declareComp.name = 'Angular'; + fixture.detectChanges(false); + expect(log).toEqual(['Insert']); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual( + 'DeclareComp(Angular) InsertComp(Hello) Hello Angular!', + 'Expect transplanted LView to be CD because the declaration is CD.'); + + insertComp.greeting = 'Hi'; + fixture.detectChanges(false); + expect(log).toEqual(['Insert']); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual( + 'DeclareComp(Angular) InsertComp(Hello) Hello Angular!', + 'expect no change because it is on push.'); + + insertComp.changeDetectorRef.markForCheck(); + fixture.detectChanges(false); + expect(log).toEqual(['Insert']); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual('DeclareComp(Angular) InsertComp(Hi) Hi Angular!'); + + // Destroy insertion should also destroy declaration + appComp.showInsert = false; + fixture.detectChanges(false); + expect(log).toEqual([]); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)).toEqual('DeclareComp(Angular)'); + + // Restore both + appComp.showInsert = true; + fixture.detectChanges(false); + expect(log).toEqual(['Insert']); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual('DeclareComp(Angular) InsertComp(Hello) Hello Angular!'); + + // Destroy declaration, But we should still be able to see updates in insertion + appComp.showDeclare = false; + insertComp.greeting = 'Hello'; + insertComp.changeDetectorRef.markForCheck(); + fixture.detectChanges(false); + expect(log).toEqual(['Insert']); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)).toEqual('InsertComp(Hello) Hello Angular!'); + }); + + it('is not checked if detectChanges is called in declaration component', () => { + declareComp.name = 'Angular'; + declareComp.changeDetector.detectChanges(); + expect(log).toEqual([]); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual('DeclareComp(Angular) InsertComp(Hello) Hello world!'); + }); + + it('is checked as part of CheckNoChanges pass', () => { + fixture.detectChanges(true); + expect(log).toEqual(['Insert', null /* logName set to null afterViewChecked */]); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual('DeclareComp(world) InsertComp(Hello) Hello world!'); + }); + }); + + describe('and declaration component is OnPush', () => { + beforeEach(() => { + fixture.componentInstance.showOnPushDeclare = true; + fixture.componentInstance.showInsertForOnPushDeclare = true; + fixture.detectChanges(false); + log.length = 0; + }); + + it('should set up component under test correctly', () => { + expect(log.length).toEqual(0); + expect(trim(fixture.nativeElement.textContent)) + .toEqual('OnPushDeclareComp(world) InsertComp(Hello) Hello world!'); + }); + + it('should not check anything no views are dirty', () => { + fixture.detectChanges(false); + expect(log).toEqual([]); + }); + + it('should CD at insertion point only', () => { + onPushDeclareComp.name = 'Angular'; + insertForOnPushDeclareComp.greeting = 'Hi'; + // mark declaration point dirty + onPushDeclareComp.changeDetector.markForCheck(); + fixture.detectChanges(false); + expect(log).toEqual(['Insert']); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual('OnPushDeclareComp(Angular) InsertComp(Hello) Hello Angular!'); + + // mark insertion point dirty + insertForOnPushDeclareComp.changeDetectorRef.markForCheck(); + fixture.detectChanges(false); + expect(log).toEqual(['Insert']); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual('OnPushDeclareComp(Angular) InsertComp(Hi) Hi Angular!'); + + // mark both insertion and declaration point dirty + insertForOnPushDeclareComp.changeDetectorRef.markForCheck(); + onPushDeclareComp.changeDetector.markForCheck(); + fixture.detectChanges(false); + expect(log).toEqual(['Insert']); + log.length = 0; + }); + + it('is not checked if detectChanges is called in declaration component', () => { + onPushDeclareComp.name = 'Angular'; + onPushDeclareComp.changeDetector.detectChanges(); + expect(log).toEqual([]); + log.length = 0; + expect(trim(fixture.nativeElement.textContent)) + .toEqual('OnPushDeclareComp(Angular) InsertComp(Hello) Hello world!'); + }); + + // TODO(FW-1774): blocked by https://github.com/angular/angular/pull/34443 + xit('is checked as part of CheckNoChanges pass', () => { + // mark declaration point dirty + onPushDeclareComp.changeDetector.markForCheck(); + fixture.detectChanges(false); + expect(log).toEqual(['Insert', null /* logName set to null in afterViewChecked */]); + log.length = 0; + + // mark insertion point dirty + insertForOnPushDeclareComp.changeDetectorRef.markForCheck(); + fixture.detectChanges(false); + expect(log).toEqual(['Insert', null]); + log.length = 0; + + // mark both insertion and declaration point dirty + insertForOnPushDeclareComp.changeDetectorRef.markForCheck(); + onPushDeclareComp.changeDetector.markForCheck(); + fixture.detectChanges(false); + expect(log).toEqual(['Insert', null]); + log.length = 0; + }); + }); + }); + + // 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', + template: ` +
Insertion({{name}})
+ + `, + changeDetection: ChangeDetectionStrategy.OnPush + }) + class Insertion { + @Input() template !: TemplateRef<{}>; + name = 'initial'; + constructor(readonly changeDetectorRef: ChangeDetectorRef) {} + } + + @Component({ + selector: 'declaration', + template: ` +
Declaration({{name}})
+ +
{{incrementChecks()}}
+
TemplateDeclaration({{name}})
+
TemplateContext({{contextName}})
+ + `, + changeDetection: ChangeDetectionStrategy.OnPush + }) + class Declaration { + @ViewChild('template') template?: TemplateRef<{}>; + name = 'initial'; + transplantedViewRefreshCount = 0; + constructor(readonly changeDetectorRef: ChangeDetectorRef) {} + incrementChecks() { + this.transplantedViewRefreshCount++; + } + } + let fixture: ComponentFixture; + let appComponent: App; + + @Component({ + template: ` + + + + ` + }) + class App { + @ViewChild(Declaration) declaration!: Declaration; + @ViewChild(Insertion) insertion!: Insertion; + template?: TemplateRef<{}>; + showInsertion = false; + } + + beforeEach(() => { + fixture = TestBed.configureTestingModule({declarations: [App, Declaration, Insertion]}) + .createComponent(App); + appComponent = fixture.componentInstance; + fixture.detectChanges(false); + appComponent.showInsertion = true; + fixture.detectChanges(false); + appComponent.declaration.transplantedViewRefreshCount = 0; + }); + + it('should set up component under test correctly', () => { + expect(fixture.nativeElement.textContent) + .toEqual( + 'Insertion(initial)TemplateDeclaration(initial)TemplateContext(initial)Declaration(initial)'); + expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(0); + }); + + + it('should not 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(); + fixture.detectChanges(false); + expect(fixture.nativeElement.textContent) + .toEqual( + 'Insertion(initial)TemplateDeclaration(new name)TemplateContext(initial)Declaration(initial)', + 'Name should not update in declaration view because only insertion was marked dirty'); + expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(1); + }); + + it('updates only the declaration view when there is a change to declaration and declaration is marked dirty', + () => { + appComponent.declaration.name = 'new name'; + appComponent.declaration.changeDetectorRef.markForCheck(); + fixture.detectChanges(false); + + const expectedContent = + 'Insertion(initial)TemplateDeclaration(initial)TemplateContext(initial)Declaration(new name)'; + expect(fixture.nativeElement.textContent).toEqual(expectedContent); + expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(0); + + // Note here that this second change detection should not be necessary, but is because of + // the backwards reference not being fully supported. The assertions below should be true + // after the first CD. + fixture.detectChanges(false); + expect(fixture.nativeElement.textContent) + .toEqual( + 'Insertion(initial)TemplateDeclaration(new name)TemplateContext(initial)Declaration(new name)'); + 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 update insertion view and template when there is a change to insertion and insertion marked dirty', + () => { + appComponent.insertion.name = 'new name'; + appComponent.insertion.changeDetectorRef.markForCheck(); + fixture.detectChanges(false); + expect(fixture.nativeElement.textContent) + .toEqual( + 'Insertion(new name)TemplateDeclaration(initial)TemplateContext(new name)Declaration(initial)'); + expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(1); + }); + + it('should not refresh the template if nothing is marked dirty', () => { + fixture.detectChanges(false); + expect(appComponent.declaration.transplantedViewRefreshCount).toEqual(0); + }); + + it('should only refresh template once 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'); + }); + }); + + describe('transplanted views shielded by OnPush', () => { + @Component({ + selector: 'check-always-insertion', + template: `` + }) + class CheckAlwaysInsertion { + @Input() template !: TemplateRef<{}>; + } + + @Component({ + selector: 'on-push-insertion-host', + template: ``, + changeDetection: ChangeDetectionStrategy.OnPush + }) + class OnPushInsertionHost { + @Input() template !: TemplateRef<{}>; + constructor(readonly cdr: ChangeDetectorRef) {} + } + @Component({ + template: ` + {{value}} + + `, + changeDetection: ChangeDetectionStrategy.OnPush + }) + class OnPushDeclaration { + @ViewChild(OnPushInsertionHost) onPushInsertionHost?: OnPushInsertionHost; + value = 'initial'; + + constructor(readonly cdr: ChangeDetectorRef) {} + } + @Component({ + template: ` + {{value}} + + ` + }) + class CheckAlwaysDeclaration { + @ViewChild(OnPushInsertionHost) onPushInsertionHost?: OnPushInsertionHost; + value = 'initial'; + } + + function getFixture(componentUnderTest: Type): ComponentFixture { + return TestBed + .configureTestingModule({ + declarations: [ + CheckAlwaysDeclaration, OnPushDeclaration, CheckAlwaysInsertion, OnPushInsertionHost + ] + }) + .createComponent(componentUnderTest); + } + + it('refresh when transplanted view is declared in CheckAlways component', () => { + const fixture = getFixture(CheckAlwaysDeclaration); + fixture.detectChanges(); + fixture.componentInstance.value = 'new'; + fixture.detectChanges(); + expect(fixture.nativeElement.textContent).toEqual('new'); + }); + + it('refresh when transplanted view is declared in OnPush component', () => { + const fixture = getFixture(OnPushDeclaration); + fixture.detectChanges(); + fixture.componentInstance.value = 'new'; + fixture.componentInstance.cdr.markForCheck(); + fixture.detectChanges(); + expect(fixture.nativeElement.textContent).toEqual('new'); + }); + + onlyInIvy('behavior is inconsistent in VE').describe('when insertion is detached', () => { + it('does not refresh CheckAlways transplants', () => { + const fixture = getFixture(CheckAlwaysDeclaration); + fixture.detectChanges(); + fixture.componentInstance.onPushInsertionHost!.cdr.detach(); + fixture.componentInstance.value = 'new'; + fixture.detectChanges(); + expect(fixture.nativeElement.textContent).toEqual('initial'); + }); + + it('does not refresh OnPush transplants', () => { + const fixture = getFixture(OnPushDeclaration); + fixture.detectChanges(); + fixture.componentInstance.onPushInsertionHost!.cdr.detach(); + fixture.componentInstance.value = 'new'; + fixture.detectChanges(); + expect(fixture.nativeElement.textContent).toEqual('initial'); + }); + }); + }); + + it('refreshes transplanted views used as template in ngForTemplate', () => { + @Component({ + selector: 'triple', + template: '
', + changeDetection: ChangeDetectionStrategy.OnPush + }) + class TripleTemplate { + @Input() template !: TemplateRef<{}>; + } + + @Component({ + template: ` + {{name}} + + ` + }) + class App { + name = 'Penny'; + } + + const fixture = + TestBed.configureTestingModule({declarations: [App, TripleTemplate]}).createComponent(App); + fixture.detectChanges(); + expect(fixture.nativeElement.textContent).toEqual('PennyPennyPenny'); + fixture.componentInstance.name = 'Sheldon'; + fixture.detectChanges(); + expect(fixture.nativeElement.textContent) + .toEqual( + 'SheldonSheldonSheldon', + 'Expected transplanted view to be refreshed even when insertion is not dirty'); + }); +}); + +function trim(text: string|null): string { + return text ? text.replace(/[\s\n]+/gm, ' ').trim() : ''; +} 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 e9ab239847..9e1adb20ce 100644 --- a/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json +++ b/packages/core/test/bundling/cyclic_import/bundle.golden_symbols.json @@ -116,6 +116,9 @@ { "name": "SANITIZER" }, + { + "name": "TRANSPLANTED_VIEWS_TO_REFRESH" + }, { "name": "TVIEW" }, @@ -326,6 +329,9 @@ { "name": "getFactoryDef" }, + { + "name": "getFirstLContainer" + }, { "name": "getFirstNativeNode" }, @@ -356,6 +362,9 @@ { "name": "getNativeByTNode" }, + { + "name": "getNextLContainer" + }, { "name": "getNodeInjectable" }, @@ -503,6 +512,9 @@ { "name": "markAsComponentHost" }, + { + "name": "markTransplantedViewsForRefresh" + }, { "name": "matchTemplateAttribute" }, @@ -536,15 +548,15 @@ { "name": "refreshComponent" }, + { + "name": "refreshContainsDirtyView" + }, { "name": "refreshContentQueries" }, { "name": "refreshDynamicEmbeddedViews" }, - { - "name": "refreshTransplantedViews" - }, { "name": "refreshView" }, @@ -638,6 +650,9 @@ { "name": "unwrapRNode" }, + { + "name": "updateTransplantedViewCount" + }, { "name": "viewAttachedToChangeDetector" }, 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 67bf1add68..6c2d598829 100644 --- a/packages/core/test/bundling/hello_world/bundle.golden_symbols.json +++ b/packages/core/test/bundling/hello_world/bundle.golden_symbols.json @@ -107,6 +107,9 @@ { "name": "SANITIZER" }, + { + "name": "TRANSPLANTED_VIEWS_TO_REFRESH" + }, { "name": "TVIEW" }, @@ -263,6 +266,9 @@ { "name": "getFactoryDef" }, + { + "name": "getFirstLContainer" + }, { "name": "getFirstNativeNode" }, @@ -287,6 +293,9 @@ { "name": "getNativeByTNode" }, + { + "name": "getNextLContainer" + }, { "name": "getNodeInjectable" }, @@ -389,6 +398,9 @@ { "name": "markAsComponentHost" }, + { + "name": "markTransplantedViewsForRefresh" + }, { "name": "nativeAppendChild" }, @@ -413,15 +425,15 @@ { "name": "refreshComponent" }, + { + "name": "refreshContainsDirtyView" + }, { "name": "refreshContentQueries" }, { "name": "refreshDynamicEmbeddedViews" }, - { - "name": "refreshTransplantedViews" - }, { "name": "refreshView" }, @@ -488,6 +500,9 @@ { "name": "unwrapRNode" }, + { + "name": "updateTransplantedViewCount" + }, { "name": "viewAttachedToChangeDetector" }, diff --git a/packages/core/test/bundling/todo/bundle.golden_symbols.json b/packages/core/test/bundling/todo/bundle.golden_symbols.json index dddc6cf473..be4029b89e 100644 --- a/packages/core/test/bundling/todo/bundle.golden_symbols.json +++ b/packages/core/test/bundling/todo/bundle.golden_symbols.json @@ -206,6 +206,9 @@ { "name": "TNODE" }, + { + "name": "TRANSPLANTED_VIEWS_TO_REFRESH" + }, { "name": "TVIEW" }, @@ -614,6 +617,9 @@ { "name": "getFactoryDef" }, + { + "name": "getFirstLContainer" + }, { "name": "getFirstNativeNode" }, @@ -653,6 +659,9 @@ { "name": "getNativeByTNode" }, + { + "name": "getNextLContainer" + }, { "name": "getNodeInjectable" }, @@ -959,6 +968,9 @@ { "name": "markDuplicates" }, + { + "name": "markTransplantedViewsForRefresh" + }, { "name": "markViewDirty" }, @@ -1019,15 +1031,15 @@ { "name": "refreshComponent" }, + { + "name": "refreshContainsDirtyView" + }, { "name": "refreshContentQueries" }, { "name": "refreshDynamicEmbeddedViews" }, - { - "name": "refreshTransplantedViews" - }, { "name": "refreshView" }, @@ -1202,6 +1214,9 @@ { "name": "updateStyling" }, + { + "name": "updateTransplantedViewCount" + }, { "name": "viewAttachedToChangeDetector" },