From 355ab5b3a66fce7e16f64af3d170bf58d37f50fc Mon Sep 17 00:00:00 2001 From: Rado Kirov Date: Wed, 10 Jun 2015 17:08:22 -0700 Subject: [PATCH] feat(query): adds support for descendants and more list apis. Additional clean up of query code. Closes: #1935 BREAKING CHANGE: By default Query only queries direct children. --- .../src/core/annotations_impl/annotations.ts | 7 +- .../angular2/src/core/annotations_impl/di.ts | 12 +- .../src/core/compiler/base_query_list.dart | 4 + .../src/core/compiler/base_query_list.ts | 14 +- .../src/core/compiler/element_injector.ts | 152 +++++++----------- .../angular2/src/core/compiler/query_list.ts | 4 +- .../core/compiler/query_integration_spec.ts | 59 +++++-- .../test/core/compiler/query_list_spec.ts | 14 ++ 8 files changed, 150 insertions(+), 116 deletions(-) diff --git a/modules/angular2/src/core/annotations_impl/annotations.ts b/modules/angular2/src/core/annotations_impl/annotations.ts index de1cb96891..91c9af5f9a 100644 --- a/modules/angular2/src/core/annotations_impl/annotations.ts +++ b/modules/angular2/src/core/annotations_impl/annotations.ts @@ -231,14 +231,13 @@ import {DEFAULT} from 'angular2/change_detection'; * * ### Injecting a live collection of descendant directives * - * Note: This is will be implemented in later release. () - * - * Similar to `@Query` above, but also includes the children of the child elements. + * By passing the descendant flag to `@Query` above, we can include the children of the child + * elements. * * ``` * @Directive({ selector: '[my-directive]' }) * class MyDirective { - * constructor(@QueryDescendents(Dependency) dependencies:QueryList) { + * constructor(@Query(Dependency, {descendants: true}) dependencies:QueryList) { * } * } * ``` diff --git a/modules/angular2/src/core/annotations_impl/di.ts b/modules/angular2/src/core/annotations_impl/di.ts index 8d297ac1d3..8c974ddec9 100644 --- a/modules/angular2/src/core/annotations_impl/di.ts +++ b/modules/angular2/src/core/annotations_impl/di.ts @@ -1,5 +1,6 @@ -import {CONST, stringify} from 'angular2/src/facade/lang'; +import {CONST, stringify, isPresent} from 'angular2/src/facade/lang'; import {DependencyAnnotation} from 'angular2/src/di/annotations_impl'; +import {resolveForwardRef} from 'angular2/di'; /** * Specifies that a constant attribute value should be injected. @@ -53,6 +54,13 @@ export class Attribute extends DependencyAnnotation { */ @CONST() export class Query extends DependencyAnnotation { - constructor(public directive: any) { super(); } + descendants: boolean; + constructor(private _directive: any, {descendants = false}: {descendants?: boolean} = {}) { + super(); + this.descendants = descendants; + } + + get directive() { return resolveForwardRef(this._directive); } + toString() { return `@Query(${stringify(this.directive)})`; } } diff --git a/modules/angular2/src/core/compiler/base_query_list.dart b/modules/angular2/src/core/compiler/base_query_list.dart index eacb4c3959..6c38e44d13 100644 --- a/modules/angular2/src/core/compiler/base_query_list.dart +++ b/modules/angular2/src/core/compiler/base_query_list.dart @@ -46,4 +46,8 @@ class BaseQueryList extends Object with IterableMixin { removeCallback(callback) { this._callbacks.remove(callback); } + + get length => this._results.length; + get first => this._results.first; + get last => this._results.last; } diff --git a/modules/angular2/src/core/compiler/base_query_list.ts b/modules/angular2/src/core/compiler/base_query_list.ts index d7d6d7147d..e87c07a014 100644 --- a/modules/angular2/src/core/compiler/base_query_list.ts +++ b/modules/angular2/src/core/compiler/base_query_list.ts @@ -1,4 +1,4 @@ -import {List, MapWrapper, ListWrapper} from 'angular2/src/facade/collection'; +import {List, ListWrapper, MapWrapper} from 'angular2/src/facade/collection'; /** * Injectable Objects that contains a live list of child directives in the light Dom of a directive. @@ -10,9 +10,9 @@ import {List, MapWrapper, ListWrapper} from 'angular2/src/facade/collection'; * @exportedAs angular2/view */ export class BaseQueryList { - _results: List; - _callbacks; - _dirty; + protected _results: List; + protected _callbacks; + protected _dirty; constructor() { this._results = []; @@ -43,4 +43,10 @@ export class BaseQueryList { onChange(callback) { ListWrapper.push(this._callbacks, callback); } removeCallback(callback) { ListWrapper.remove(this._callbacks, callback); } + + get length() { return this._results.length; } + + get first() { return ListWrapper.first(this._results); } + + get last() { return ListWrapper.last(this._results); } } diff --git a/modules/angular2/src/core/compiler/element_injector.ts b/modules/angular2/src/core/compiler/element_injector.ts index 1ca28cbf6e..2de8649b77 100644 --- a/modules/angular2/src/core/compiler/element_injector.ts +++ b/modules/angular2/src/core/compiler/element_injector.ts @@ -82,35 +82,6 @@ export class TreeNode> { if (isPresent(parent)) parent.addChild(this); } - _assertConsistency(): void { - this._assertHeadBeforeTail(); - this._assertTailReachable(); - this._assertPresentInParentList(); - } - - _assertHeadBeforeTail(): void { - if (isBlank(this._tail) && isPresent(this._head)) - throw new BaseException('null tail but non-null head'); - } - - _assertTailReachable(): void { - if (isBlank(this._tail)) return; - if (isPresent(this._tail._next)) throw new BaseException('node after tail'); - var p = this._head; - while (isPresent(p) && p != this._tail) p = p._next; - if (isBlank(p) && isPresent(this._tail)) throw new BaseException('tail not reachable.') - } - - _assertPresentInParentList(): void { - var p = this._parent; - if (isBlank(p)) { - return; - } - var cur = p._head; - while (isPresent(cur) && cur != this) cur = cur._next; - if (isBlank(cur)) throw new BaseException('node not reachable through parent.') - } - /** * Adds a child to the parent node. The child MUST NOT be a part of a tree. */ @@ -123,7 +94,6 @@ export class TreeNode> { } child._next = null; child._parent = this; - this._assertConsistency(); } /** @@ -131,7 +101,6 @@ export class TreeNode> { * The child MUST NOT be a part of a tree and the sibling must be present. */ addChildAfter(child: T, prevSibling: T): void { - this._assertConsistency(); if (isBlank(prevSibling)) { var prevHead = this._head; this._head = child; @@ -141,19 +110,16 @@ export class TreeNode> { this.addChild(child); return; } else { - prevSibling._assertPresentInParentList(); child._next = prevSibling._next; prevSibling._next = child; } child._parent = this; - this._assertConsistency(); } /** * Detaches a node from the parent's tree. */ remove(): void { - this._assertConsistency(); if (isBlank(this.parent)) return; var nextSibling = this._next; var prevSibling = this._findPrev(); @@ -165,10 +131,8 @@ export class TreeNode> { if (isBlank(nextSibling)) { this._parent._tail = prevSibling; } - this._parent._assertConsistency(); this._parent = null; this._next = null; - this._assertConsistency(); } /** @@ -217,14 +181,14 @@ export class DependencyWithVisibility extends Dependency { export class DirectiveDependency extends DependencyWithVisibility { constructor(key: Key, asPromise: boolean, lazy: boolean, optional: boolean, properties: List, - visibility: Visibility, public attributeName: string, public queryDirective) { + visibility: Visibility, public attributeName: string, public queryDecorator: Query) { super(key, asPromise, lazy, optional, properties, visibility); this._verify(); } _verify(): void { var count = 0; - if (isPresent(this.queryDirective)) count++; + if (isPresent(this.queryDecorator)) count++; if (isPresent(this.attributeName)) count++; if (count > 1) throw new BaseException( @@ -243,10 +207,7 @@ export class DirectiveDependency extends DependencyWithVisibility { return isPresent(p) ? p.attributeName : null; } - static _query(properties) { - var p = ListWrapper.find(properties, (p) => p instanceof Query); - return isPresent(p) ? resolveForwardRef(p.directive) : null; - } + static _query(properties) { return ListWrapper.find(properties, (p) => p instanceof Query); } } export class DirectiveBinding extends ResolvedBinding { @@ -706,6 +667,8 @@ export class ElementInjector extends TreeNode { private _query1: QueryRef; private _query2: QueryRef; + hydrated: boolean; + _strategy: _ElementInjectorStrategy; constructor(public _proto: ProtoElementInjector, parent: ElementInjector) { @@ -713,12 +676,14 @@ export class ElementInjector extends TreeNode { this._strategy = _proto._strategy.createElementInjectorStrategy(this); this._constructionCounter = 0; + this.hydrated = false; - this._inheritQueries(parent); this._buildQueries(); + this._addParentQueries(); } dehydrate(): void { + this.hydrated = false; this._host = null; this._preBuiltObjects = null; this._lightDomAppInjector = null; @@ -755,6 +720,7 @@ export class ElementInjector extends TreeNode { this._checkShadowDomAppInjector(this._shadowDomAppInjector); this._strategy.hydrate(); + this.hydrated = true; } private _createShadowDomAppInjector(componentDirective: DirectiveBinding, @@ -906,7 +872,7 @@ export class ElementInjector extends TreeNode { var dirDep = dep; if (isPresent(dirDep.attributeName)) return this._buildAttribute(dirDep); - if (isPresent(dirDep.queryDirective)) return this._findQuery(dirDep.queryDirective).list; + if (isPresent(dirDep.queryDecorator)) return this._findQuery(dirDep.queryDecorator).list; if (dirDep.key.id === StaticKeys.instance().changeDetectorRefId) { var componentView = this._preBuiltObjects.view.componentChildViews[this._proto.index]; return componentView.changeDetector.ref; @@ -942,67 +908,57 @@ export class ElementInjector extends TreeNode { _buildQueriesForDeps(deps: List): void { for (var i = 0; i < deps.length; i++) { var dep = deps[i]; - if (isPresent(dep.queryDirective)) { - this._createQueryRef(dep.queryDirective); + if (isPresent(dep.queryDecorator)) { + this._createQueryRef(dep.queryDecorator); } } } - private _createQueryRef(directive): void { + private _createQueryRef(query: Query): void { var queryList = new QueryList(); if (isBlank(this._query0)) { - this._query0 = new QueryRef(directive, queryList, this); + this._query0 = new QueryRef(query, queryList, this); } else if (isBlank(this._query1)) { - this._query1 = new QueryRef(directive, queryList, this); + this._query1 = new QueryRef(query, queryList, this); } else if (isBlank(this._query2)) { - this._query2 = new QueryRef(directive, queryList, this); + this._query2 = new QueryRef(query, queryList, this); } else throw new QueryError(); } private _addToQueries(obj, token): void { - if (isPresent(this._query0) && (this._query0.directive === token)) { + if (isPresent(this._query0) && (this._query0.query.directive === token)) { this._query0.list.add(obj); } - if (isPresent(this._query1) && (this._query1.directive === token)) { + if (isPresent(this._query1) && (this._query1.query.directive === token)) { this._query1.list.add(obj); } - if (isPresent(this._query2) && (this._query2.directive === token)) { + if (isPresent(this._query2) && (this._query2.query.directive === token)) { this._query2.list.add(obj); } } - // TODO(rado): unify with _addParentQueries. - private _inheritQueries(parent: ElementInjector): void { - if (isBlank(parent)) return; - if (isPresent(parent._query0)) { - this._query0 = parent._query0; - } - if (isPresent(parent._query1)) { - this._query1 = parent._query1; - } - if (isPresent(parent._query2)) { - this._query2 = parent._query2; - } - } - private _buildQueries(): void { if (isPresent(this._proto)) { this._strategy.buildQueries(); } } - private _findQuery(token): QueryRef { - if (isPresent(this._query0) && this._query0.directive === token) { + private _findQuery(query): QueryRef { + if (isPresent(this._query0) && this._query0.query === query) { return this._query0; } - if (isPresent(this._query1) && this._query1.directive === token) { + if (isPresent(this._query1) && this._query1.query === query) { return this._query1; } - if (isPresent(this._query2) && this._query2.directive === token) { + if (isPresent(this._query2) && this._query2.query === query) { return this._query2; } - throw new BaseException(`Cannot find query for directive ${token}.`); + throw new BaseException(`Cannot find query for directive ${query}.`); + } + + _hasQuery(query: QueryRef): boolean { + return this._query0 == query || this._query1 == query || this._query2 == query; } link(parent: ElementInjector): void { @@ -1016,38 +972,39 @@ export class ElementInjector extends TreeNode { } private _addParentQueries(): void { + if (isBlank(this.parent)) return; if (isPresent(this.parent._query0)) { this._addQueryToTree(this.parent._query0); - this.parent._query0.update(); + if (this.hydrated) this.parent._query0.update(); } if (isPresent(this.parent._query1)) { this._addQueryToTree(this.parent._query1); - this.parent._query1.update(); + if (this.hydrated) this.parent._query1.update(); } if (isPresent(this.parent._query2)) { this._addQueryToTree(this.parent._query2); - this.parent._query2.update(); + if (this.hydrated) this.parent._query2.update(); } } unlink(): void { - var queriesToUpDate = []; + var queriesToUpdate = []; if (isPresent(this.parent._query0)) { this._pruneQueryFromTree(this.parent._query0); - ListWrapper.push(queriesToUpDate, this.parent._query0); + ListWrapper.push(queriesToUpdate, this.parent._query0); } if (isPresent(this.parent._query1)) { this._pruneQueryFromTree(this.parent._query1); - ListWrapper.push(queriesToUpDate, this.parent._query1); + ListWrapper.push(queriesToUpdate, this.parent._query1); } if (isPresent(this.parent._query2)) { this._pruneQueryFromTree(this.parent._query2); - ListWrapper.push(queriesToUpDate, this.parent._query2); + ListWrapper.push(queriesToUpdate, this.parent._query2); } this.remove(); - ListWrapper.forEach(queriesToUpDate, (q) => q.update()); + ListWrapper.forEach(queriesToUpdate, (q) => q.update()); } private _pruneQueryFromTree(query: QueryRef): void { @@ -1060,12 +1017,24 @@ export class ElementInjector extends TreeNode { } } - private _addQueryToTree(query: QueryRef): void { - this._assignQueryRef(query); + private _addQueryToTree(queryRef: QueryRef): void { + if (queryRef.query.descendants == false) { + if (this == queryRef.originator) { + this._addQueryToTreeSelfAndRecurse(queryRef); + } else if (this.parent == queryRef.originator && this._proto.distanceToParent == 1) { + this._assignQueryRef(queryRef); + } + } else { + this._addQueryToTreeSelfAndRecurse(queryRef); + } + } + + private _addQueryToTreeSelfAndRecurse(queryRef: QueryRef): void { + this._assignQueryRef(queryRef); var child = this._head; while (isPresent(child)) { - child._addQueryToTree(query); + child._addQueryToTree(queryRef); child = child._next; } } @@ -1507,15 +1476,8 @@ class QueryError extends BaseException { } class QueryRef { - directive; - list: QueryList; - originator: ElementInjector; - - constructor(directive, list: QueryList, originator: ElementInjector) { - this.directive = directive; - this.list = list; - this.originator = originator; - } + constructor(public query: Query, public list: QueryList, + public originator: ElementInjector) {} update(): void { var aggregator = []; @@ -1524,9 +1486,9 @@ class QueryRef { } visit(inj: ElementInjector, aggregator): void { - if (isBlank(inj)) return; - if (inj.hasDirective(this.directive)) { - ListWrapper.push(aggregator, inj.get(this.directive)); + if (isBlank(inj) || !inj._hasQuery(this)) return; + if (inj.hasDirective(this.query.directive)) { + ListWrapper.push(aggregator, inj.get(this.query.directive)); } var child = inj._head; while (isPresent(child)) { diff --git a/modules/angular2/src/core/compiler/query_list.ts b/modules/angular2/src/core/compiler/query_list.ts index dca74910a3..9d0152c9cd 100644 --- a/modules/angular2/src/core/compiler/query_list.ts +++ b/modules/angular2/src/core/compiler/query_list.ts @@ -35,9 +35,9 @@ import {BaseQueryList} from './base_query_list'; * with `` * component's on `hydrate` and deregister on `dehydrate` event. While a reasonable approach, this * would only work - * partialy since `*ng-for` could rearange the list of `` components which would not be + * partialy since `*ng-for` could rearrange the list of `` components which would not be * reported to `` - * component and thus the list of `` componets would be out of sync with respect to the list + * component and thus the list of `` components would be out of sync with respect to the list * of `` elements. * * A preferred solution is to inject a `QueryList` which is a live list of directives in the diff --git a/modules/angular2/test/core/compiler/query_integration_spec.ts b/modules/angular2/test/core/compiler/query_integration_spec.ts index 4f299a4e4b..f503e6cfb9 100644 --- a/modules/angular2/test/core/compiler/query_integration_spec.ts +++ b/modules/angular2/test/core/compiler/query_integration_spec.ts @@ -13,7 +13,7 @@ import { import {TestBed} from 'angular2/src/test_lib/test_bed'; -import {Injectable} from 'angular2/di'; +import {Injectable, Optional} from 'angular2/di'; import {QueryList} from 'angular2/core'; import {Query, Component, Directive, View} from 'angular2/annotations'; @@ -25,10 +25,12 @@ export function main() { BrowserDomAdapter.makeCurrent(); describe('Query API', () => { - it('should contain all directives in the light dom', + it('should contain all direct child directives in the light dom', inject([TestBed, AsyncTestCompleter], (tb, async) => { var template = '
' + - '
' + + '
' + + '
' + + '
' + '
'; tb.createView(MyComp, {html: template}) @@ -40,11 +42,46 @@ export function main() { }); })); + it('should contain all directives in the light dom when descendants flag is used', + inject([TestBed, AsyncTestCompleter], (tb, async) => { + var template = '
' + + '
' + + '
' + + '
' + + '
'; + + tb.createView(MyComp, {html: template}) + .then((view) => { + view.detectChanges(); + expect(view.rootNodes).toHaveText('2|3|4|'); + + async.done(); + }); + })); + + it('should contain all directives in the light dom', + inject([TestBed, AsyncTestCompleter], (tb, async) => { + var template = '
' + + '
' + + '
'; + + tb.createView(MyComp, {html: template}) + .then((view) => { + view.detectChanges(); + expect(view.rootNodes).toHaveText('2|3|'); + + async.done(); + }); + })); + + // TODO(rado): The test below should be using descendants: false, + // but due to a bug with how injectors are hooked up query considers the + // directives to be distances 2 instead of direct children. it('should reflect dynamically inserted directives', inject([TestBed, AsyncTestCompleter], (tb, async) => { var template = '
' + - '
' + + '
' + '
'; tb.createView(MyComp, {html: template}) @@ -55,8 +92,6 @@ export function main() { view.context.shouldShow = true; view.detectChanges(); - // TODO(rado): figure out why the second tick is necessary. - view.detectChanges(); expect(view.rootNodes).toHaveText('2|3|'); async.done(); @@ -66,7 +101,7 @@ export function main() { it('should reflect moved directives', inject([TestBed, AsyncTestCompleter], (tb, async) => { var template = '
' + - '
' + + '
' + '
'; tb.createView(MyComp, {html: template}) @@ -102,10 +137,16 @@ class NeedsQuery { constructor(@Query(TextDirective) query: QueryList) { this.query = query; } } -var _constructiontext = 0; +@Component({selector: 'needs-query-desc'}) +@View({directives: [NgFor], template: '
{{dir.text}}|
'}) +@Injectable() +class NeedsQueryDesc { + query: QueryList; + constructor(@Query(TextDirective, {descendants: true}) query: QueryList) { this.query = query; } +} @Component({selector: 'my-comp'}) -@View({directives: [NeedsQuery, TextDirective, NgIf, NgFor]}) +@View({directives: [NeedsQuery, NeedsQueryDesc, TextDirective, NgIf, NgFor]}) @Injectable() class MyComp { shouldShow: boolean; diff --git a/modules/angular2/test/core/compiler/query_list_spec.ts b/modules/angular2/test/core/compiler/query_list_spec.ts index d5eb4ea0f9..2d26b2126a 100644 --- a/modules/angular2/test/core/compiler/query_list_spec.ts +++ b/modules/angular2/test/core/compiler/query_list_spec.ts @@ -30,6 +30,20 @@ export function main() { expect(log).toEqual('one again, two again'); }); + it('should support length', () => { + queryList.add('one'); + queryList.add('two'); + expect(queryList.length).toEqual(2); + }); + + it('should support first and last', () => { + queryList.add('one'); + queryList.add('two'); + queryList.add('three'); + expect(queryList.first).toEqual('one'); + expect(queryList.last).toEqual('three'); + }); + describe('simple observable interface', () => { it('should fire callbacks on change', () => { var fires = 0;