From d2e42567a6b91fa1b1c4567c77c9c08cd3ef6776 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Mon, 27 Feb 2017 13:00:49 -0800 Subject: [PATCH] fix(core): call lifecycle hooks for siblings in declaration order --- modules/@angular/core/src/view/element.ts | 4 +- modules/@angular/core/src/view/ng_content.ts | 2 +- modules/@angular/core/src/view/provider.ts | 48 ++++++---- .../@angular/core/src/view/pure_expression.ts | 2 +- modules/@angular/core/src/view/query.ts | 2 +- modules/@angular/core/src/view/text.ts | 2 +- modules/@angular/core/src/view/types.ts | 11 +-- modules/@angular/core/src/view/view.ts | 51 ++--------- .../change_detection_integration_spec.ts | 13 +-- .../@angular/core/test/view/view_def_spec.ts | 89 ++++++------------- 10 files changed, 82 insertions(+), 142 deletions(-) diff --git a/modules/@angular/core/src/view/element.ts b/modules/@angular/core/src/view/element.ts index d2f9e57d96..630f9de722 100644 --- a/modules/@angular/core/src/view/element.ts +++ b/modules/@angular/core/src/view/element.ts @@ -31,7 +31,6 @@ export function anchorDef( return { // will bet set by the view definition index: undefined, - reverseChildIndex: undefined, parent: undefined, renderParent: undefined, bindingIndex: undefined, @@ -39,6 +38,7 @@ export function anchorDef( // regular values flags, childFlags: 0, + directChildFlags: 0, childMatchedQueries: 0, matchedQueries, matchedQueryIds, references, ngContentIndex, childCount, bindings: [], outputs: [], @@ -131,7 +131,6 @@ export function elementDef( return { // will bet set by the view definition index: undefined, - reverseChildIndex: undefined, parent: undefined, renderParent: undefined, bindingIndex: undefined, @@ -139,6 +138,7 @@ export function elementDef( // regular values flags, childFlags: 0, + directChildFlags: 0, childMatchedQueries: 0, matchedQueries, matchedQueryIds, references, ngContentIndex, childCount, bindings: bindingDefs, outputs: outputDefs, diff --git a/modules/@angular/core/src/view/ng_content.ts b/modules/@angular/core/src/view/ng_content.ts index 8acbd1898c..d22c4cb800 100644 --- a/modules/@angular/core/src/view/ng_content.ts +++ b/modules/@angular/core/src/view/ng_content.ts @@ -13,7 +13,6 @@ export function ngContentDef(ngContentIndex: number, index: number): NodeDef { return { // will bet set by the view definition index: undefined, - reverseChildIndex: undefined, parent: undefined, renderParent: undefined, bindingIndex: undefined, @@ -21,6 +20,7 @@ export function ngContentDef(ngContentIndex: number, index: number): NodeDef { // regular values flags: NodeFlags.TypeNgContent, childFlags: 0, + directChildFlags: 0, childMatchedQueries: 0, matchedQueries: {}, matchedQueryIds: 0, diff --git a/modules/@angular/core/src/view/provider.ts b/modules/@angular/core/src/view/provider.ts index 69e709d3ef..b5ee345852 100644 --- a/modules/@angular/core/src/view/provider.ts +++ b/modules/@angular/core/src/view/provider.ts @@ -94,7 +94,6 @@ export function _def( return { // will bet set by the view definition index: undefined, - reverseChildIndex: undefined, parent: undefined, renderParent: undefined, bindingIndex: undefined, @@ -102,6 +101,7 @@ export function _def( // regular values flags, childFlags: 0, + directChildFlags: 0, childMatchedQueries: 0, matchedQueries, matchedQueryIds, references, ngContentIndex: undefined, childCount, bindings, outputs, element: undefined, @@ -432,25 +432,43 @@ export function callLifecycleHooksChildrenFirst(view: ViewData, lifecycles: Node if (!(view.def.nodeFlags & lifecycles)) { return; } - const len = view.def.nodes.length; - for (let i = 0; i < len; i++) { - // We use the reverse child oreder to call providers of children first. - const nodeDef = view.def.reverseChildNodes[i]; - const nodeIndex = nodeDef.index; - if (nodeDef.flags & lifecycles) { - // a leaf - Services.setCurrentNode(view, nodeIndex); - callProviderLifecycles(asProviderData(view, nodeIndex).instance, nodeDef.flags & lifecycles); - } else if ((nodeDef.childFlags & lifecycles) === 0) { - // a parent with leafs - // no child matches one of the lifecycles, - // then skip the children + const nodes = view.def.nodes; + for (let i = 0; i < nodes.length; i++) { + const nodeDef = nodes[i]; + let parent = nodeDef.parent; + if (!parent && nodeDef.flags & lifecycles) { + // matching root node (e.g. a pipe) + callProviderLifecycles(view, i, nodeDef.flags & lifecycles); + } + if ((nodeDef.childFlags & lifecycles) === 0) { + // no child matches one of the lifecycles i += nodeDef.childCount; } + while (parent && (parent.flags & NodeFlags.TypeElement) && + i === parent.index + parent.childCount) { + // last child of an element + if (parent.directChildFlags & lifecycles) { + callElementProvidersLifecycles(view, parent, lifecycles); + } + parent = parent.parent; + } } } -function callProviderLifecycles(provider: any, lifecycles: NodeFlags) { +function callElementProvidersLifecycles(view: ViewData, elDef: NodeDef, lifecycles: NodeFlags) { + for (let i = elDef.index + 1; i <= elDef.index + elDef.childCount; i++) { + const nodeDef = view.def.nodes[i]; + if (nodeDef.flags & lifecycles) { + callProviderLifecycles(view, i, nodeDef.flags & lifecycles); + } + // only visit direct children + i += nodeDef.childCount; + } +} + +function callProviderLifecycles(view: ViewData, index: number, lifecycles: NodeFlags) { + const provider = asProviderData(view, index).instance; + Services.setCurrentNode(view, index); if (lifecycles & NodeFlags.AfterContentInit) { provider.ngAfterContentInit(); } diff --git a/modules/@angular/core/src/view/pure_expression.ts b/modules/@angular/core/src/view/pure_expression.ts index 819066d512..c35e1e6272 100644 --- a/modules/@angular/core/src/view/pure_expression.ts +++ b/modules/@angular/core/src/view/pure_expression.ts @@ -38,7 +38,6 @@ function _pureExpressionDef(flags: NodeFlags, propertyNames: string[]): NodeDef return { // will bet set by the view definition index: undefined, - reverseChildIndex: undefined, parent: undefined, renderParent: undefined, bindingIndex: undefined, @@ -46,6 +45,7 @@ function _pureExpressionDef(flags: NodeFlags, propertyNames: string[]): NodeDef // regular values flags, childFlags: 0, + directChildFlags: 0, childMatchedQueries: 0, matchedQueries: {}, matchedQueryIds: 0, diff --git a/modules/@angular/core/src/view/query.ts b/modules/@angular/core/src/view/query.ts index 40a721c6e7..b59b3ac103 100644 --- a/modules/@angular/core/src/view/query.ts +++ b/modules/@angular/core/src/view/query.ts @@ -26,7 +26,6 @@ export function queryDef( return { // will bet set by the view definition index: undefined, - reverseChildIndex: undefined, parent: undefined, renderParent: undefined, bindingIndex: undefined, @@ -34,6 +33,7 @@ export function queryDef( // regular values flags, childFlags: 0, + directChildFlags: 0, childMatchedQueries: 0, ngContentIndex: undefined, matchedQueries: {}, diff --git a/modules/@angular/core/src/view/text.ts b/modules/@angular/core/src/view/text.ts index 5d1bcb96b7..ae25850991 100644 --- a/modules/@angular/core/src/view/text.ts +++ b/modules/@angular/core/src/view/text.ts @@ -30,7 +30,6 @@ export function textDef(ngContentIndex: number, constants: string[]): NodeDef { return { // will bet set by the view definition index: undefined, - reverseChildIndex: undefined, parent: undefined, renderParent: undefined, bindingIndex: undefined, @@ -38,6 +37,7 @@ export function textDef(ngContentIndex: number, constants: string[]): NodeDef { // regular values flags, childFlags: 0, + directChildFlags: 0, childMatchedQueries: 0, matchedQueries: {}, matchedQueryIds: 0, diff --git a/modules/@angular/core/src/view/types.ts b/modules/@angular/core/src/view/types.ts index 9ffff090bb..ccda1440e6 100644 --- a/modules/@angular/core/src/view/types.ts +++ b/modules/@angular/core/src/view/types.ts @@ -33,11 +33,7 @@ export interface ViewDefinition { nodes: NodeDef[]; /** aggregated NodeFlags for all nodes **/ nodeFlags: NodeFlags; - /** - * Order: parents before children, but children in reverse order. - * Especially providers are after elements / anchors. - */ - reverseChildNodes: NodeDef[]; + rootNodeFlags: NodeFlags; lastRenderRootNode: NodeDef; bindingCount: number; outputCount: number; @@ -83,15 +79,16 @@ export const enum ViewFlags { export interface NodeDef { flags: NodeFlags; index: number; - reverseChildIndex: number; parent: NodeDef; renderParent: NodeDef; /** this is checked against NgContentDef.index to find matched nodes */ ngContentIndex: number; /** number of transitive children */ childCount: number; - /** aggregated NodeFlags for all children (does not include self) **/ + /** aggregated NodeFlags for all transitive children (does not include self) **/ childFlags: NodeFlags; + /** aggregated NodeFlags for all direct children (does not include self) **/ + directChildFlags: NodeFlags; bindingIndex: number; bindings: BindingDef[]; diff --git a/modules/@angular/core/src/view/view.ts b/modules/@angular/core/src/view/view.ts index 9813f0af9f..faa0509e94 100644 --- a/modules/@angular/core/src/view/view.ts +++ b/modules/@angular/core/src/view/view.ts @@ -33,6 +33,7 @@ export function viewDef( let viewBindingCount = 0; let viewDisposableCount = 0; let viewNodeFlags = 0; + let viewRootNodeFlags = 0; let viewMatchedQueries = 0; let currentParent: NodeDef = null; let currentElementHasPublicProviders = false; @@ -52,8 +53,6 @@ export function viewDef( node.parent = currentParent; node.bindingIndex = viewBindingCount; node.outputIndex = viewDisposableCount; - node.reverseChildIndex = - calculateReverseChildIndex(currentParent, i, node.childCount, nodes.length); // renderParent needs to account for ng-container! let currentRenderParent: NodeDef; @@ -74,7 +73,6 @@ export function viewDef( currentElementHasPublicProviders = false; currentElementHasPrivateProviders = false; } - reverseChildNodes[node.reverseChildIndex] = node; validateNode(currentParent, node, nodes.length); viewNodeFlags |= node.flags; @@ -84,10 +82,13 @@ export function viewDef( } if (currentParent) { currentParent.childFlags |= node.flags; + currentParent.directChildFlags |= node.flags; currentParent.childMatchedQueries |= node.matchedQueryIds; if (node.element && node.element.template) { currentParent.childMatchedQueries |= node.element.template.nodeMatchedQueries; } + } else { + viewRootNodeFlags |= node.flags; } viewBindingCount += node.bindings.length; @@ -136,8 +137,9 @@ export function viewDef( nodes[nodeIndex].element.handleEvent(view, eventName, event); return { nodeFlags: viewNodeFlags, + rootNodeFlags: viewRootNodeFlags, nodeMatchedQueries: viewMatchedQueries, flags, - nodes: nodes, reverseChildNodes, + nodes: nodes, updateDirectives: updateDirectives || NOOP, updateRenderer: updateRenderer || NOOP, handleEvent: handleEvent || NOOP, @@ -146,47 +148,6 @@ export function viewDef( }; } - - -function calculateReverseChildIndex( - currentParent: NodeDef, i: number, childCount: number, nodeCount: number) { - // Notes about reverse child order: - // - Every node is directly before its children, in dfs and reverse child order. - // - node.childCount contains all children, in dfs and reverse child order. - // - In dfs order, every node is before its first child - // - In reverse child order, every node is before its last child - - // Algorithm, main idea: - // - In reverse child order, the ranges for each child + its transitive children are mirrored - // regarding their position inside of their parent - - // Visualization: - // Given the following tree: - // Nodes: n0 - // n1 n2 - // n11 n12 n21 n22 - // dfs: 0 1 2 3 4 5 6 - // result: 0 4 6 5 1 3 2 - // - // Example: - // Current node = 1 - // 1) lastChildIndex = 3 - // 2) lastChildOffsetRelativeToParentInDfsOrder = 2 - // 3) parentEndIndexInReverseChildOrder = 6 - // 4) result = 4 - let lastChildOffsetRelativeToParentInDfsOrder: number; - let parentEndIndexInReverseChildOrder: number; - if (currentParent) { - const lastChildIndex = i + childCount; - lastChildOffsetRelativeToParentInDfsOrder = lastChildIndex - currentParent.index - 1; - parentEndIndexInReverseChildOrder = currentParent.reverseChildIndex + currentParent.childCount; - } else { - lastChildOffsetRelativeToParentInDfsOrder = i + childCount; - parentEndIndexInReverseChildOrder = nodeCount - 1; - } - return parentEndIndexInReverseChildOrder - lastChildOffsetRelativeToParentInDfsOrder; -} - function validateNode(parent: NodeDef, node: NodeDef, nodeCount: number) { const template = node.element && node.element.template; if (template) { diff --git a/modules/@angular/core/test/linker/change_detection_integration_spec.ts b/modules/@angular/core/test/linker/change_detection_integration_spec.ts index e969431f72..91790b3d90 100644 --- a/modules/@angular/core/test/linker/change_detection_integration_spec.ts +++ b/modules/@angular/core/test/linker/change_detection_integration_spec.ts @@ -906,12 +906,13 @@ function createTests({viewEngine}: {viewEngine: boolean}) { it('should be called in reverse order so the child is always notified before the parent', fakeAsync(() => { const ctx = createCompFixture( - '
'); + '
'); ctx.detectChanges(false); expect(directiveLog.filter(['ngAfterContentChecked'])).toEqual([ - 'child.ngAfterContentChecked', 'parent.ngAfterContentChecked' + 'child.ngAfterContentChecked', 'parent.ngAfterContentChecked', + 'sibling.ngAfterContentChecked' ]); })); }); @@ -1018,12 +1019,12 @@ function createTests({viewEngine}: {viewEngine: boolean}) { it('should be called in reverse order so the child is always notified before the parent', fakeAsync(() => { const ctx = createCompFixture( - '
'); + '
'); ctx.detectChanges(false); expect(directiveLog.filter(['ngAfterViewChecked'])).toEqual([ - 'child.ngAfterViewChecked', 'parent.ngAfterViewChecked' + 'child.ngAfterViewChecked', 'parent.ngAfterViewChecked', 'sibling.ngAfterViewChecked' ]); })); }); @@ -1061,13 +1062,13 @@ function createTests({viewEngine}: {viewEngine: boolean}) { it('should be called in reverse order so the child is always notified before the parent', fakeAsync(() => { const ctx = createCompFixture( - '
'); + '
'); ctx.detectChanges(false); ctx.destroy(); expect(directiveLog.filter(['ngOnDestroy'])).toEqual([ - 'child.ngOnDestroy', 'parent.ngOnDestroy' + 'child.ngOnDestroy', 'parent.ngOnDestroy', 'sibling.ngOnDestroy' ]); })); diff --git a/modules/@angular/core/test/view/view_def_spec.ts b/modules/@angular/core/test/view/view_def_spec.ts index c8753252da..98f2d9d026 100644 --- a/modules/@angular/core/test/view/view_def_spec.ts +++ b/modules/@angular/core/test/view/view_def_spec.ts @@ -11,69 +11,6 @@ import {filterQueryId} from '@angular/core/src/view/util'; export function main() { describe('viewDef', () => { - describe('reverseChild order', () => { - function reverseChildOrder(viewDef: ViewDefinition): number[] { - return viewDef.reverseChildNodes.map(node => node.index); - } - - it('should reverse child order for root nodes', () => { - const vd = viewDef(ViewFlags.None, [ - textDef(null, ['a']), // level 0, index 0 - textDef(null, ['a']), // level 0, index 0 - ]); - - expect(reverseChildOrder(vd)).toEqual([1, 0]); - }); - - it('should reverse child order for one level, one root', () => { - const vd = viewDef(ViewFlags.None, [ - elementDef(NodeFlags.None, null, null, 2, 'span'), // level 0, index 0 - textDef(null, ['a']), // level 1, index 1 - textDef(null, ['a']), // level 1, index 2 - ]); - - expect(reverseChildOrder(vd)).toEqual([0, 2, 1]); - }); - - it('should reverse child order for 1 level, 2 roots', () => { - const vd = viewDef(ViewFlags.None, [ - elementDef(NodeFlags.None, null, null, 2, 'span'), // level 0, index 0 - textDef(null, ['a']), // level 1, index 1 - textDef(null, ['a']), // level 1, index 2 - elementDef(NodeFlags.None, null, null, 1, 'span'), // level 0, index 3 - textDef(null, ['a']), // level 1, index 4 - ]); - - expect(reverseChildOrder(vd)).toEqual([3, 4, 0, 2, 1]); - }); - - it('should reverse child order for 2 levels', () => { - const vd = viewDef(ViewFlags.None, [ - elementDef(NodeFlags.None, null, null, 4, 'span'), // level 0, index 0 - elementDef(NodeFlags.None, null, null, 1, 'span'), // level 1, index 1 - textDef(null, ['a']), // level 2, index 2 - elementDef(NodeFlags.None, null, null, 1, 'span'), // level 1, index 3 - textDef(null, ['a']), // level 2, index 4 - ]); - - expect(reverseChildOrder(vd)).toEqual([0, 3, 4, 1, 2]); - }); - - it('should reverse child order for mixed levels', () => { - const vd = viewDef(ViewFlags.None, [ - textDef(null, ['a']), // level 0, index 0 - elementDef(NodeFlags.None, null, null, 5, 'span'), // level 0, index 1 - textDef(null, ['a']), // level 1, index 2 - elementDef(NodeFlags.None, null, null, 1, 'span'), // level 1, index 3 - textDef(null, ['a']), // level 2, index 4 - elementDef(NodeFlags.None, null, null, 1, 'span'), // level 1, index 5 - textDef(null, ['a']), // level 2, index 6 - textDef(null, ['a']), // level 0, index 7 - ]); - - expect(reverseChildOrder(vd)).toEqual([7, 1, 5, 6, 3, 4, 2, 0]); - }); - }); describe('parent', () => { function parents(viewDef: ViewDefinition): number[] { @@ -122,6 +59,10 @@ export function main() { return viewDef.nodes.map(node => node.childFlags); } + function directChildFlags(viewDef: ViewDefinition): number[] { + return viewDef.nodes.map(node => node.directChildFlags); + } + it('should calculate childFlags for one level', () => { const vd = viewDef(ViewFlags.None, [ elementDef(NodeFlags.None, null, null, 1, 'span'), @@ -131,6 +72,10 @@ export function main() { expect(childFlags(vd)).toEqual([ NodeFlags.TypeDirective | NodeFlags.AfterContentChecked, NodeFlags.None ]); + + expect(directChildFlags(vd)).toEqual([ + NodeFlags.TypeDirective | NodeFlags.AfterContentChecked, NodeFlags.None + ]); }); it('should calculate childFlags for two levels', () => { @@ -144,6 +89,11 @@ export function main() { NodeFlags.TypeElement | NodeFlags.TypeDirective | NodeFlags.AfterContentChecked, NodeFlags.TypeDirective | NodeFlags.AfterContentChecked, NodeFlags.None ]); + + expect(directChildFlags(vd)).toEqual([ + NodeFlags.TypeElement, NodeFlags.TypeDirective | NodeFlags.AfterContentChecked, + NodeFlags.None + ]); }); it('should calculate childFlags for one level, multiple roots', () => { @@ -160,6 +110,12 @@ export function main() { NodeFlags.TypeDirective | NodeFlags.AfterContentInit | NodeFlags.AfterViewChecked, NodeFlags.None, NodeFlags.None ]); + + expect(directChildFlags(vd)).toEqual([ + NodeFlags.TypeDirective | NodeFlags.AfterContentChecked, NodeFlags.None, + NodeFlags.TypeDirective | NodeFlags.AfterContentInit | NodeFlags.AfterViewChecked, + NodeFlags.None, NodeFlags.None + ]); }); it('should calculate childFlags for multiple levels', () => { @@ -178,6 +134,13 @@ export function main() { NodeFlags.TypeDirective | NodeFlags.AfterContentInit | NodeFlags.AfterViewInit, NodeFlags.None, NodeFlags.None ]); + + expect(directChildFlags(vd)).toEqual([ + NodeFlags.TypeElement, NodeFlags.TypeDirective | NodeFlags.AfterContentChecked, + NodeFlags.None, + NodeFlags.TypeDirective | NodeFlags.AfterContentInit | NodeFlags.AfterViewInit, + NodeFlags.None, NodeFlags.None + ]); }); });