From 2150a8f9d1373770a6a27258de1c1ffecb90fd21 Mon Sep 17 00:00:00 2001 From: Rado Kirov Date: Wed, 29 Jul 2015 16:16:45 -0700 Subject: [PATCH] feat(query): view query is properly updated when dom changes. Fixes a bug in view manager util where sibling injector is not correctly calculated. ViewQuery no longer includes the view's initiating component injector. Includes some refactoring of view methods and a removal of a polymorphic map call. Closes #3033 Closes #3439 --- .../src/change_detection/interfaces.ts | 2 + .../src/core/compiler/element_injector.ts | 123 +++++++++++------- .../src/core/compiler/view_manager_utils.ts | 5 +- .../core/compiler/query_integration_spec.ts | 49 ++++--- .../core/compiler/view_manager_utils_spec.ts | 3 +- 5 files changed, 110 insertions(+), 72 deletions(-) diff --git a/modules/angular2/src/change_detection/interfaces.ts b/modules/angular2/src/change_detection/interfaces.ts index 6b49661846..5068d4609b 100644 --- a/modules/angular2/src/change_detection/interfaces.ts +++ b/modules/angular2/src/change_detection/interfaces.ts @@ -3,6 +3,7 @@ import {CONST} from 'angular2/src/facade/lang'; import {Locals} from './parser/locals'; import {BindingRecord} from './binding_record'; import {DirectiveIndex, DirectiveRecord} from './directive_record'; +import {ChangeDetectorRef} from './change_detector_ref'; /** * Interface used by Angular to control the change detection strategy for an application. @@ -50,6 +51,7 @@ export interface ChangeDispatcher { export interface ChangeDetector { parent: ChangeDetector; mode: string; + ref: ChangeDetectorRef; addChild(cd: ChangeDetector): void; addShadowDomChild(cd: ChangeDetector): void; diff --git a/modules/angular2/src/core/compiler/element_injector.ts b/modules/angular2/src/core/compiler/element_injector.ts index eb8f82dce1..289e245f1c 100644 --- a/modules/angular2/src/core/compiler/element_injector.ts +++ b/modules/angular2/src/core/compiler/element_injector.ts @@ -421,7 +421,7 @@ class _Context { export class ElementInjector extends TreeNode implements DependencyProvider { private _host: ElementInjector; - private _preBuiltObjects = null; + private _preBuiltObjects: PreBuiltObjects = null; // Queries are added during construction or linking with a new parent. // They are removed only through unlinking. @@ -477,19 +477,35 @@ export class ElementInjector extends TreeNode implements Depend this._host = host; this._preBuiltObjects = preBuiltObjects; - this._reattachInjectors(imperativelyCreatedInjector); - this._strategy.hydrate(); - if (isPresent(host)) { this._addViewQueries(host); } + this._reattachInjectors(imperativelyCreatedInjector); + this._strategy.hydrate(); + this._addDirectivesToQueries(); this._addVarBindingsToQueries(); + // TODO(rado): optimize this call, if view queries are not moved around, + // simply appending to the query list is faster than updating. + this._updateViewQueries(); + this.hydrated = true; } + private _updateViewQueries() { + if (isPresent(this._query0) && this._query0.isViewQuery) { + this._query0.update(); + } + if (isPresent(this._query1) && this._query1.isViewQuery) { + this._query1.update(); + } + if (isPresent(this._query2) && this._query2.isViewQuery) { + this._query2.update(); + } + } + private _debugContext(): any { var p = this._preBuiltObjects; var index = p.elementRef.boundElementIndex - p.view.elementOffset; @@ -651,22 +667,19 @@ export class ElementInjector extends TreeNode implements Depend } private _addViewQueries(host: ElementInjector): void { - if (isPresent(host._query0) && host._query0.originator == host && - host._query0.query.isViewQuery) - this._addViewQuery(host._query0); - if (isPresent(host._query1) && host._query1.originator == host && - host._query1.query.isViewQuery) - this._addViewQuery(host._query1); - if (isPresent(host._query2) && host._query2.originator == host && - host._query2.query.isViewQuery) - this._addViewQuery(host._query2); + this._addViewQuery(host._query0, host); + this._addViewQuery(host._query1, host); + this._addViewQuery(host._query2, host); } - private _addViewQuery(queryRef: QueryRef): void { - // TODO(rado): Replace this.parent check with distanceToParent = 1 when - // https://github.com/angular/angular/issues/2707 is fixed. - if (!queryRef.query.descendants && isPresent(this.parent)) return; - this._assignQueryRef(queryRef); + private _addViewQuery(queryRef: QueryRef, host: ElementInjector): void { + if (isBlank(queryRef) || !queryRef.isViewQuery || this._hasQuery(queryRef)) return; + if (host._query0.originator == host) { + // TODO(rado): Replace this.parent check with distanceToParent = 1 when + // https://github.com/angular/angular/issues/2707 is fixed. + if (!queryRef.query.descendants && isPresent(this.parent)) return; + this._assignQueryRef(queryRef); + } } private _addVarBindingsToQueries(): void { @@ -694,6 +707,7 @@ export class ElementInjector extends TreeNode implements Depend private _addDirectivesToQuery(queryRef: QueryRef): void { if (isBlank(queryRef) || queryRef.query.isVarBindingQuery) return; + if (queryRef.isViewQuery && queryRef.originator == this) return; var matched = []; this.addDirectivesMatchingQuery(queryRef.query, matched); @@ -754,42 +768,37 @@ export class ElementInjector extends TreeNode implements Depend this._addParentQueries(); } + unlink(): void { + var parent = this.parent; + this.remove(); + this._removeParentQueries(parent); + } + private _addParentQueries(): void { if (isBlank(this.parent)) return; - if (isPresent(this.parent._query0) && !this.parent._query0.query.isViewQuery) { - this._addQueryToTree(this.parent._query0); - if (this.hydrated) this.parent._query0.update(); - } - if (isPresent(this.parent._query1) && !this.parent._query1.query.isViewQuery) { - this._addQueryToTree(this.parent._query1); - if (this.hydrated) this.parent._query1.update(); - } - if (isPresent(this.parent._query2) && !this.parent._query2.query.isViewQuery) { - this._addQueryToTree(this.parent._query2); - if (this.hydrated) this.parent._query2.update(); + this._addParentQuery(this.parent._query0); + this._addParentQuery(this.parent._query1); + this._addParentQuery(this.parent._query2); + } + + private _addParentQuery(query): void { + if (isPresent(query) && !this._hasQuery(query)) { + this._addQueryToTree(query); + if (this.hydrated) query.update(); } } - unlink(): void { - var queriesToUpdate = []; - if (isPresent(this.parent._query0)) { - this._pruneQueryFromTree(this.parent._query0); - queriesToUpdate.push(this.parent._query0); - } - if (isPresent(this.parent._query1)) { - this._pruneQueryFromTree(this.parent._query1); - queriesToUpdate.push(this.parent._query1); - } - if (isPresent(this.parent._query2)) { - this._pruneQueryFromTree(this.parent._query2); - queriesToUpdate.push(this.parent._query2); - } + private _removeParentQueries(parent: ElementInjector): void { + this._removeParentQuery(parent._query0); + this._removeParentQuery(parent._query1); + this._removeParentQuery(parent._query2); + } - this.remove(); - // TODO(rado): update should work on view queries too, however currently it - // is not implemented, so we filter to non-view queries. - var nonViewQueries = ListWrapper.filter(queriesToUpdate, (q) => !q.query.isViewQuery); - ListWrapper.forEach(nonViewQueries, (q) => q.update()); + private _removeParentQuery(query: QueryRef) { + if (isPresent(query)) { + this._pruneQueryFromTree(query); + query.update(); + } } private _pruneQueryFromTree(query: QueryRef): void { @@ -852,6 +861,12 @@ export class ElementInjector extends TreeNode implements Depend getHost(): ElementInjector { return this._host; } getBoundElementIndex(): number { return this._proto.index; } + + getRootViewInjectors(): ElementInjector[] { + var view = this._preBuiltObjects.view; + return view.getNestedView(view.elementOffset + this.getBoundElementIndex()) + .rootElementInjectors; + } } interface _ElementInjectorStrategy { @@ -1133,9 +1148,19 @@ export class QueryRef { constructor(public query: Query, public list: QueryList, public originator: ElementInjector) {} + get isViewQuery(): boolean { return this.query.isViewQuery; } + update(): void { var aggregator = []; - this.visit(this.originator, aggregator); + if (this.query.isViewQuery) { + // intentionally skipping originator for view queries. + var rootViewInjectors = this.originator.getRootViewInjectors(); + for (var i = 0; i < rootViewInjectors.length; i++) { + this.visit(rootViewInjectors[i], aggregator); + } + } else { + this.visit(this.originator, aggregator); + } this.list.reset(aggregator); } diff --git a/modules/angular2/src/core/compiler/view_manager_utils.ts b/modules/angular2/src/core/compiler/view_manager_utils.ts index a4ae21fe63..7d91fade95 100644 --- a/modules/angular2/src/core/compiler/view_manager_utils.ts +++ b/modules/angular2/src/core/compiler/view_manager_utils.ts @@ -120,13 +120,14 @@ export class AppViewManagerUtils { parentView.viewContainers[boundElementIndex] = viewContainer; } ListWrapper.insert(viewContainer.views, atIndex, view); + var elementInjector = contextView.elementInjectors[contextBoundElementIndex]; + var sibling; if (atIndex == 0) { - sibling = null; + sibling = elementInjector; } else { sibling = ListWrapper.last(viewContainer.views[atIndex - 1].rootElementInjectors); } - var elementInjector = contextView.elementInjectors[contextBoundElementIndex]; for (var i = view.rootElementInjectors.length - 1; i >= 0; i--) { if (isPresent(elementInjector.parent)) { view.rootElementInjectors[i].linkAfter(elementInjector.parent, sibling); diff --git a/modules/angular2/test/core/compiler/query_integration_spec.ts b/modules/angular2/test/core/compiler/query_integration_spec.ts index 3b2c116c9c..dbc70a69f3 100644 --- a/modules/angular2/test/core/compiler/query_integration_spec.ts +++ b/modules/angular2/test/core/compiler/query_integration_spec.ts @@ -355,7 +355,7 @@ export function main() { view.detectChanges(); - expect(q.query.map((d: TextDirective) => d.text)).toEqual(["self", "1", "2", "3"]); + expect(q.query.map((d: TextDirective) => d.text)).toEqual(["1", "2", "3"]); async.done(); }); @@ -407,26 +407,29 @@ export function main() { }); })); - /* TODO(rado): fix and reenable. it('should maintain directives in pre-order depth-first DOM order after dynamic insertion', - inject([TestComponentBuilder, AsyncTestCompleter], (tcb:TestComponentBuilder, async) => { - var template = ''; + inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => { + var template = ''; - tcb.overrideTemplate(MyComp, template) - .createAsync(MyComp) - .then((view) => { - var q:NeedsViewQueryOrder = view.componentViewChildren[0].getLocal("q"); + tcb.overrideTemplate(MyComp, template) + .createAsync(MyComp) + .then((view) => { + var q: NeedsViewQueryOrder = view.componentViewChildren[0].getLocal("q"); - view.detectChanges(); + view.detectChanges(); - expect(q.query.length).toBe(4); - expect(q.query.first.text).toEqual("1"); - expect(q.query.first.text).toEqual("4"); + expect(q.query.map((d: TextDirective) => d.text)).toEqual(["1", "2", "3", "4"]); - async.done(); - }); - }));*/ + q.list = ["-3", "2"]; + view.detectChanges(); + + + expect(q.query.map((d: TextDirective) => d.text)).toEqual(["1", "-3", "2", "4"]); + + async.done(); + }); + })); }); }); } @@ -551,17 +554,23 @@ class NeedsViewQueryNestedIf { } +// TODO(rado): once https://github.com/angular/angular/issues/3438 is resolved +// duplicate the test without InertDirective. @Component({selector: 'needs-view-query-order'}) @View({ - directives: [NgFor, TextDirective], - template: '
' + - '
' + - '
' + directives: [NgFor, TextDirective, InertDirective], + template: '
' + + '
' + + '
' }) @Injectable() class NeedsViewQueryOrder { query: QueryList; - constructor(@ViewQuery(TextDirective) query: QueryList) { this.query = query; } + list: string[]; + constructor(@ViewQuery(TextDirective, {descendants: true}) query: QueryList) { + this.query = query; + this.list = ['2', '3']; + } } @Component({selector: 'needs-tpl'}) diff --git a/modules/angular2/test/core/compiler/view_manager_utils_spec.ts b/modules/angular2/test/core/compiler/view_manager_utils_spec.ts index c6b353797d..e7a9531451 100644 --- a/modules/angular2/test/core/compiler/view_manager_utils_spec.ts +++ b/modules/angular2/test/core/compiler/view_manager_utils_spec.ts @@ -171,7 +171,8 @@ export function main() { createViews(2); utils.attachViewInContainer(parentView, 0, contextView, 1, 0, childView); expect(childView.rootElementInjectors[0].spy('linkAfter')) - .toHaveBeenCalledWith(contextView.elementInjectors[0], null); + .toHaveBeenCalledWith(contextView.elementInjectors[0], + contextView.elementInjectors[1]); }); });