Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix projected change detection #16592

Closed
wants to merge 2 commits into from
Closed

Fix projected change detection #16592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bdd9a16
fix(core): projected views should be dirty checked when the declaring…
tbosch May 5, 2017
1b32dd7
fix(core): detach projected views when a parent view is destroyed
tbosch May 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
60 changes: 32 additions & 28 deletions packages/core/src/view/types.ts
View file
Open in desktop
Expand Up @@ -141,37 +141,38 @@ export const enum NodeFlags {
None = 0,
TypeElement = 1 << 0,
TypeText = 1 << 1,
ProjectedTemplate = 1 << 2,
CatRenderNode = TypeElement | TypeText,
TypeNgContent = 1 << 2,
TypePipe = 1 << 3,
TypePureArray = 1 << 4,
TypePureObject = 1 << 5,
TypePurePipe = 1 << 6,
TypeNgContent = 1 << 3,
TypePipe = 1 << 4,
TypePureArray = 1 << 5,
TypePureObject = 1 << 6,
TypePurePipe = 1 << 7,
CatPureExpression = TypePureArray | TypePureObject | TypePurePipe,
TypeValueProvider = 1 << 7,
TypeClassProvider = 1 << 8,
TypeFactoryProvider = 1 << 9,
TypeUseExistingProvider = 1 << 10,
LazyProvider = 1 << 11,
PrivateProvider = 1 << 12,
TypeDirective = 1 << 13,
Component = 1 << 14,
TypeValueProvider = 1 << 8,
TypeClassProvider = 1 << 9,
TypeFactoryProvider = 1 << 10,
TypeUseExistingProvider = 1 << 11,
LazyProvider = 1 << 12,
PrivateProvider = 1 << 13,
TypeDirective = 1 << 14,
Component = 1 << 15,
CatProvider = TypeValueProvider | TypeClassProvider | TypeFactoryProvider |
TypeUseExistingProvider | TypeDirective,
OnInit = 1 << 15,
OnDestroy = 1 << 16,
DoCheck = 1 << 17,
OnChanges = 1 << 18,
AfterContentInit = 1 << 19,
AfterContentChecked = 1 << 20,
AfterViewInit = 1 << 21,
AfterViewChecked = 1 << 22,
EmbeddedViews = 1 << 23,
ComponentView = 1 << 24,
TypeContentQuery = 1 << 25,
TypeViewQuery = 1 << 26,
StaticQuery = 1 << 27,
DynamicQuery = 1 << 28,
OnInit = 1 << 16,
OnDestroy = 1 << 17,
DoCheck = 1 << 18,
OnChanges = 1 << 19,
AfterContentInit = 1 << 20,
AfterContentChecked = 1 << 21,
AfterViewInit = 1 << 22,
AfterViewChecked = 1 << 23,
EmbeddedViews = 1 << 24,
ComponentView = 1 << 25,
TypeContentQuery = 1 << 26,
TypeViewQuery = 1 << 27,
StaticQuery = 1 << 28,
DynamicQuery = 1 << 29,
CatQuery = TypeContentQuery | TypeViewQuery,

// mutually exclusive values...
Expand Down Expand Up @@ -328,7 +329,10 @@ export const enum ViewState {
FirstCheck = 1 << 1,
Attached = 1 << 2,
ChecksEnabled = 1 << 3,
Destroyed = 1 << 4,
IsProjectedView = 1 << 4,
CheckProjectedView = 1 << 5,
CheckProjectedViews = 1 << 6,
Destroyed = 1 << 7,

CatDetectChanges = Attached | ChecksEnabled,
CatInit = BeforeFirstCheck | CatDetectChanges
Expand Down
8 changes: 8 additions & 0 deletions packages/core/src/view/util.ts
View file
Open in desktop
Expand Up @@ -117,6 +117,14 @@ export function markParentViewsForCheck(view: ViewData) {
}
}

export function markParentViewsForCheckProjectedViews(view: ViewData, endView: ViewData) {
let currView: ViewData|null = view;
while (currView && currView !== endView) {
currView.state |= ViewState.CheckProjectedViews;
currView = currView.viewContainerParent || currView.parent;
}
}

export function dispatchEvent(
view: ViewData, nodeIndex: number, eventName: string, event: any): boolean {
const nodeDef = view.def.nodes[nodeIndex];
Expand Down
79 changes: 71 additions & 8 deletions packages/core/src/view/view.ts
View file
Open in desktop
Expand Up @@ -17,7 +17,8 @@ import {checkAndUpdateQuery, createQuery} from './query';
import {createTemplateData, createViewContainerData} from './refs';
import {checkAndUpdateTextDynamic, checkAndUpdateTextInline, createText} from './text';
import {ArgumentType, CheckType, ElementData, NodeData, NodeDef, NodeFlags, ProviderData, RootData, Services, ViewData, ViewDefinition, ViewFlags, ViewHandleEventFn, ViewState, ViewUpdateFn, asElementData, asQueryList, asTextData} from './types';
import {NOOP, checkBindingNoChanges, isComponentView, resolveViewDefinition} from './util';
import {NOOP, checkBindingNoChanges, isComponentView, markParentViewsForCheckProjectedViews, resolveViewDefinition} from './util';
import {detachProjectedView} from './view_attach';

export function viewDef(
flags: ViewFlags, nodes: NodeDef[], updateDirectives?: ViewUpdateFn,
Expand Down Expand Up @@ -314,12 +315,14 @@ function createViewNodes(view: ViewData) {
}

export function checkNoChangesView(view: ViewData) {
markProjectedViewsForCheck(view);
Services.updateDirectives(view, CheckType.CheckNoChanges);
execEmbeddedViewsAction(view, ViewAction.CheckNoChanges);
Services.updateRenderer(view, CheckType.CheckNoChanges);
execComponentViewsAction(view, ViewAction.CheckNoChanges);
// Note: We don't check queries for changes as we didn't do this in v2.x.
// TODO(tbosch): investigate if we can enable the check again in v5.x with a nicer error message.
view.state &= ~(ViewState.CheckProjectedViews | ViewState.CheckProjectedView);
}

export function checkAndUpdateView(view: ViewData) {
Expand All @@ -329,6 +332,7 @@ export function checkAndUpdateView(view: ViewData) {
} else {
view.state &= ~ViewState.FirstCheck;
}
markProjectedViewsForCheck(view);
Services.updateDirectives(view, CheckType.CheckAndUpdate);
execEmbeddedViewsAction(view, ViewAction.CheckAndUpdate);
execQueriesAction(
Expand All @@ -343,14 +347,14 @@ export function checkAndUpdateView(view: ViewData) {
execComponentViewsAction(view, ViewAction.CheckAndUpdate);
execQueriesAction(
view, NodeFlags.TypeViewQuery, NodeFlags.DynamicQuery, CheckType.CheckAndUpdate);

callLifecycleHooksChildrenFirst(
view, NodeFlags.AfterViewChecked |
(view.state & ViewState.FirstCheck ? NodeFlags.AfterViewInit : 0));

if (view.def.flags & ViewFlags.OnPush) {
view.state &= ~ViewState.ChecksEnabled;
}
view.state &= ~(ViewState.CheckProjectedViews | ViewState.CheckProjectedView);
}

export function checkAndUpdateNode(
Expand All @@ -363,6 +367,31 @@ export function checkAndUpdateNode(
}
}

function markProjectedViewsForCheck(view: ViewData) {
const def = view.def;
if (!(def.nodeFlags & NodeFlags.ProjectedTemplate)) {
return;
}
for (let i = 0; i < def.nodes.length; i++) {
const nodeDef = def.nodes[i];
if (nodeDef.flags & NodeFlags.ProjectedTemplate) {
const projectedViews = asElementData(view, i).template._projectedViews;
if (projectedViews) {
for (let i = 0; i < projectedViews.length; i++) {
const projectedView = projectedViews[i];
projectedView.state |= ViewState.CheckProjectedView;
markParentViewsForCheckProjectedViews(projectedView, view);
}
}
} else if ((nodeDef.childFlags & NodeFlags.ProjectedTemplate) === 0) {
// a parent with leafs
// no child is a component,
// then skip the children
i += nodeDef.childCount;
}
}
}

function checkAndUpdateNodeInline(
view: ViewData, nodeDef: NodeDef, v0?: any, v1?: any, v2?: any, v3?: any, v4?: any, v5?: any,
v6?: any, v7?: any, v8?: any, v9?: any): boolean {
Expand Down Expand Up @@ -474,6 +503,7 @@ export function destroyView(view: ViewData) {
view.disposables[i]();
}
}
detachProjectedView(view);
if (view.renderer.destroyNode) {
destroyViewNodes(view);
}
Expand All @@ -498,7 +528,9 @@ function destroyViewNodes(view: ViewData) {
enum ViewAction {
CreateViewNodes,
CheckNoChanges,
CheckNoChangesProjectedViews,
CheckAndUpdate,
CheckAndUpdateProjectedViews,
Destroy
}

Expand Down Expand Up @@ -547,18 +579,44 @@ function callViewAction(view: ViewData, action: ViewAction) {
const viewState = view.state;
switch (action) {
case ViewAction.CheckNoChanges:
if ((viewState & ViewState.CatDetectChanges) === ViewState.CatDetectChanges &&
(viewState & ViewState.Destroyed) === 0) {
checkNoChangesView(view);
if ((viewState & ViewState.Destroyed) === 0) {
if ((viewState & ViewState.CatDetectChanges) === ViewState.CatDetectChanges) {
checkNoChangesView(view);
} else if (viewState & ViewState.CheckProjectedViews) {
execProjectedViewsAction(view, ViewAction.CheckNoChangesProjectedViews);
}
}
break;
case ViewAction.CheckNoChangesProjectedViews:
if ((viewState & ViewState.Destroyed) === 0) {
if (viewState & ViewState.CheckProjectedView) {
checkNoChangesView(view);
} else if (viewState & ViewState.CheckProjectedViews) {
execProjectedViewsAction(view, action);
}
}
break;
case ViewAction.CheckAndUpdate:
if ((viewState & ViewState.CatDetectChanges) === ViewState.CatDetectChanges &&
(viewState & ViewState.Destroyed) === 0) {
checkAndUpdateView(view);
if ((viewState & ViewState.Destroyed) === 0) {
if ((viewState & ViewState.CatDetectChanges) === ViewState.CatDetectChanges) {
checkAndUpdateView(view);
} else if (viewState & ViewState.CheckProjectedViews) {
execProjectedViewsAction(view, ViewAction.CheckAndUpdateProjectedViews);
}
}
break;
case ViewAction.CheckAndUpdateProjectedViews:
if ((viewState & ViewState.Destroyed) === 0) {
if (viewState & ViewState.CheckProjectedView) {
checkAndUpdateView(view);
} else if (viewState & ViewState.CheckProjectedViews) {
execProjectedViewsAction(view, action);
}
}
break;
case ViewAction.Destroy:
// Note: destroyView recurses over all views,
// so we don't need to special case projected views here.
destroyView(view);
break;
case ViewAction.CreateViewNodes:
Expand All @@ -567,6 +625,11 @@ function callViewAction(view: ViewData, action: ViewAction) {
}
}

function execProjectedViewsAction(view: ViewData, action: ViewAction) {
execEmbeddedViewsAction(view, action);
execComponentViewsAction(view, action);
}

function execQueriesAction(
view: ViewData, queryFlags: NodeFlags, staticDynamicQueryFlag: NodeFlags,
checkType: CheckType) {
Expand Down
71 changes: 55 additions & 16 deletions packages/core/src/view/view_attach.ts
View file
Open in desktop
Expand Up @@ -6,8 +6,8 @@
* found in the LICENSE file at https://angular.io/license
*/

import {ElementData, Services, ViewData} from './types';
import {RenderNodeAction, declaredViewContainer, renderNode, visitRootRenderNodes} from './util';
import {ElementData, NodeDef, NodeFlags, Services, ViewData, ViewDefinition, ViewState} from './types';
import {RenderNodeAction, declaredViewContainer, isComponentView, renderNode, visitRootRenderNodes} from './util';

export function attachEmbeddedView(
parentView: ViewData, elementData: ElementData, viewIndex: number | undefined | null,
Expand All @@ -18,21 +18,51 @@ export function attachEmbeddedView(
}
view.viewContainerParent = parentView;
addToArray(embeddedViews, viewIndex !, view);
const dvcElementData = declaredViewContainer(view);
if (dvcElementData && dvcElementData !== elementData) {
let projectedViews = dvcElementData.template._projectedViews;
if (!projectedViews) {
projectedViews = dvcElementData.template._projectedViews = [];
}
projectedViews.push(view);
}
attachProjectedView(elementData, view);

Services.dirtyParentQueries(view);

const prevView = viewIndex ! > 0 ? embeddedViews[viewIndex ! - 1] : null;
renderAttachEmbeddedView(elementData, prevView, view);
}

function attachProjectedView(vcElementData: ElementData, view: ViewData) {
const dvcElementData = declaredViewContainer(view);
if (!dvcElementData || dvcElementData === vcElementData ||
view.state & ViewState.IsProjectedView) {
return;
}
// Note: For performance reasons, we
// - add a view to template._projectedViews only 1x throughout its lifetime,
// and remove it not until the view is destroyed.
// (hard, as when a parent view is attached/detached we would need to attach/detach all
// nested projected views as well, even accross component boundaries).
// - don't track the insertion order of views in the projected views array
// (hard, as when the views of the same template are inserted different view containers)
view.state |= ViewState.IsProjectedView;
let projectedViews = dvcElementData.template._projectedViews;
if (!projectedViews) {
projectedViews = dvcElementData.template._projectedViews = [];
}
projectedViews.push(view);
// Note: we are changing the NodeDef here as we cannot calculate
// the fact whether a template is used for projection during compilation.
markNodeAsProjectedTemplate(view.parent !.def, view.parentNodeDef !);
}

function markNodeAsProjectedTemplate(viewDef: ViewDefinition, nodeDef: NodeDef) {
if (nodeDef.flags & NodeFlags.ProjectedTemplate) {
return;
}
viewDef.nodeFlags |= NodeFlags.ProjectedTemplate;
nodeDef.flags |= NodeFlags.ProjectedTemplate;
let parentNodeDef = nodeDef.parent;
while (parentNodeDef) {
parentNodeDef.childFlags |= NodeFlags.ProjectedTemplate;
parentNodeDef = parentNodeDef.parent;
}
}

export function detachEmbeddedView(elementData: ElementData, viewIndex?: number): ViewData|null {
const embeddedViews = elementData.viewContainer !._embeddedViews;
if (viewIndex == null || viewIndex >= embeddedViews.length) {
Expand All @@ -45,19 +75,28 @@ export function detachEmbeddedView(elementData: ElementData, viewIndex?: number)
view.viewContainerParent = null;
removeFromArray(embeddedViews, viewIndex);

const dvcElementData = declaredViewContainer(view);
if (dvcElementData && dvcElementData !== elementData) {
const projectedViews = dvcElementData.template._projectedViews;
removeFromArray(projectedViews, projectedViews.indexOf(view));
}

// See attachProjectedView for why we don't update projectedViews here.
Services.dirtyParentQueries(view);

renderDetachView(view);

return view;
}

export function detachProjectedView(view: ViewData) {
if (!(view.state & ViewState.IsProjectedView)) {
return;
}
const dvcElementData = declaredViewContainer(view);
if (dvcElementData) {
const projectedViews = dvcElementData.template._projectedViews;
if (projectedViews) {
removeFromArray(projectedViews, projectedViews.indexOf(view));
Services.dirtyParentQueries(view);
}
}
}

export function moveEmbeddedView(
elementData: ElementData, oldViewIndex: number, newViewIndex: number): ViewData {
const embeddedViews = elementData.viewContainer !._embeddedViews;
Expand Down