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(ivy): cache local names and support multiple locals with same value #22807

Closed
wants to merge 2 commits into from
Closed

fix(ivy): cache local names and support multiple locals with same value #22807

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
8190e69
test(ivy): correct export tests and add query test
kara Mar 14, 2018
1ca8c8d
fix(ivy): cache local names and support multiple locals with same value
kara Mar 15, 2018
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
56 changes: 30 additions & 26 deletions packages/core/src/render3/instructions.ts
View file
Open in desktop
Expand Up @@ -491,13 +491,11 @@ export function elementStart(
// accessed through their containers because they may be removed / re-added later.
node = createLNode(index, LNodeFlags.Element, native, componentView);

// TODO(misko): implement code which caches the local reference resolution
const queryName: string|null = hack_findQueryName(hostComponentDef, localRefs, '');

if (node.tNode == null) {
const localNames: (string | number)[]|null =
findMatchingLocalNames(hostComponentDef, localRefs, isHostElement ? index + 1 : -1, '');
ngDevMode && assertDataInRange(index - 1);
node.tNode = tData[index] =
createTNode(name, attrs || null, null, hostComponentDef ? null : queryName);
node.tNode = tData[index] = createTNode(name, attrs || null, null, localNames);
}

if (attrs) setUpAttributes(native, attrs);
Expand All @@ -508,7 +506,7 @@ export function elementStart(
// is not guaranteed. Must be refactored to take it into account.
const instance = hostComponentDef.n();
storeComponentIndex(index);
directiveCreate(++index, instance, hostComponentDef, queryName);
directiveCreate(++index, instance, hostComponentDef, null);
initChangeDetectorIfExisting(node.nodeInjector, instance);
}
hack_declareDirectives(index, directiveTypes, localRefs);
Expand All @@ -532,8 +530,8 @@ export function initChangeDetectorIfExisting(injector: LInjector | null, instanc
}

/**
* This function instantiates a directive with a correct queryName. It is a hack since we should
* compute the query value only once and store it with the template (rather than on each invocation)
* This function instantiates the given directives. It is a hack since it assumes the directives
* come in the correct order for DI.
*/
function hack_declareDirectives(
index: number, directiveTypes: DirectiveType<any>[] | null | undefined,
Expand All @@ -546,30 +544,35 @@ function hack_declareDirectives(
const directiveType = directiveTypes[i];
const directiveDef = currentView.tView.firstTemplatePass ? directiveType.ngDirectiveDef :
tData[index] as DirectiveDef<any>;
directiveCreate(
index, directiveDef.n(), directiveDef, hack_findQueryName(directiveDef, localRefs));
const localNames = currentView.tView.firstTemplatePass ?
findMatchingLocalNames(directiveDef, localRefs, index) :
null;

directiveCreate(index, directiveDef.n(), directiveDef, localNames);
}
}
}

/**
* This function returns the queryName for a directive. It is a hack since we should
* compute the query value only once and store it with the template (rather than on each invocation)
* Finds any local names that match the given directive's exportAs and returns them with directive
* index. If the directiveDef is null, it matches against the default '' value instead of
* exportAs.
*/
function hack_findQueryName(
directiveDef: DirectiveDef<any>| null, localRefs: string[] | null | undefined,
defaultExport?: string, ): string|null {
function findMatchingLocalNames(
directiveDef: DirectiveDef<any>| null, localRefs: string[] | null | undefined, index: number,
defaultExport?: string): (string | number)[]|null {
const exportAs = directiveDef && directiveDef.exportAs || defaultExport;
let matches: (string | number)[]|null = null;
if (exportAs != null && localRefs) {
for (let i = 0; i < localRefs.length; i = i + 2) {
const local = localRefs[i];
const toExportAs = localRefs[i | 1];
if (toExportAs === exportAs || toExportAs === defaultExport) {
return local;
(matches || (matches = [])).push(local, index);
}
}
}
return null;
return matches;
}

/**
Expand Down Expand Up @@ -801,15 +804,16 @@ export function elementProperty<T>(
* @param tagName
* @param attrs
* @param data
* @param localNames A list of local names and their matching indices
* @returns the TNode object
*/
function createTNode(
tagName: string | null, attrs: string[] | null, data: TContainer | null,
localName: string | null): TNode {
localNames: (string | number)[] | null): TNode {
return {
tagName: tagName,
attrs: attrs,
localNames: localName ? [localName, -1] : null,
localNames: localNames,
initialInputs: undefined,
inputs: undefined,
outputs: undefined,
Expand Down Expand Up @@ -1043,10 +1047,11 @@ export function textBinding<T>(index: number, value: T | NO_CHANGE): void {
* be created or retrieved out of order.
* @param directive The directive instance.
* @param directiveDef DirectiveDef object which contains information about the template.
* @param queryName Name under which the query can retrieve the directive instance.
* @param localNames Names under which a query can retrieve the directive instance
*/
export function directiveCreate<T>(
index: number, directive: T, directiveDef: DirectiveDef<T>, queryName?: string | null): T {
index: number, directive: T, directiveDef: DirectiveDef<T>,
localNames?: (string | number)[] | null): T {
let instance;
ngDevMode &&
assertNull(currentView.bindingStartIndex, 'directives should be created before any bindings');
Expand All @@ -1068,10 +1073,10 @@ export function directiveCreate<T>(

if (index >= tData.length) {
tData[index] = directiveDef !;
if (queryName) {
if (localNames) {
ngDevMode && assertNotNull(previousOrParentNode.tNode, 'previousOrParentNode.tNode');
const tNode = previousOrParentNode !.tNode !;
(tNode.localNames || (tNode.localNames = [])).push(queryName, index);
tNode.localNames = tNode.localNames ? tNode.localNames.concat(localNames) : localNames;
}
}

Expand Down Expand Up @@ -1197,9 +1202,8 @@ export function container(
const node = createLNode(index, LNodeFlags.Container, undefined, lContainer);

if (node.tNode == null) {
// TODO(misko): implement queryName caching
const queryName: string|null = hack_findQueryName(null, localRefs, '');
node.tNode = tData[index] = createTNode(tagName || null, attrs || null, [], queryName || null);
const localNames: (string | number)[]|null = findMatchingLocalNames(null, localRefs, -1, '');
node.tNode = tData[index] = createTNode(tagName || null, attrs || null, [], localNames);
}

// Containers are added to the current view tree instead of their embedded views
Expand Down
24 changes: 12 additions & 12 deletions packages/core/test/render3/exports_spec.ts
View file
Open in desktop
Expand Up @@ -17,7 +17,7 @@ describe('exports', () => {
/** <input value="one" #myInput> {{ myInput.value }} */
function Template(ctx: any, cm: boolean) {
if (cm) {
elementStart(0, 'input', ['value', 'one']);
elementStart(0, 'input', ['value', 'one'], null, ['myInput', '']);
elementEnd();
text(1);
}
Expand All @@ -33,7 +33,7 @@ describe('exports', () => {
/** <comp #myComp></comp> {{ myComp.name }} */
function Template(ctx: any, cm: boolean) {
if (cm) {
elementStart(0, MyComponent);
elementStart(0, MyComponent, null, null, ['myComp', '']);
elementEnd();
text(2);
}
Expand Down Expand Up @@ -78,7 +78,7 @@ describe('exports', () => {
/** <comp #myComp></comp> <div [myDir]="myComp"></div> */
function Template(ctx: any, cm: boolean) {
if (cm) {
elementStart(0, MyComponent);
elementStart(0, MyComponent, null, null, ['myComp', '']);
elementEnd();
elementStart(2, 'div', null, [MyDir]);
elementEnd();
Expand All @@ -95,7 +95,7 @@ describe('exports', () => {
/** <div someDir #myDir="someDir"></div> {{ myDir.name }} */
function Template(ctx: any, cm: boolean) {
if (cm) {
elementStart(0, 'div', null, [SomeDir]);
elementStart(0, 'div', null, [SomeDir], ['myDir', 'someDir']);
elementEnd();
text(2);
}
Expand All @@ -116,7 +116,7 @@ describe('exports', () => {
function Template(ctx: any, cm: boolean) {
if (cm) {
text(0);
elementStart(1, 'input', ['value', 'one']);
elementStart(1, 'input', ['value', 'one'], null, ['myInput', '']);
elementEnd();
}
let myInput = elementStart(1);
Expand All @@ -133,7 +133,7 @@ describe('exports', () => {
if (cm) {
elementStart(0, 'div');
elementEnd();
elementStart(1, 'input', ['value', 'one']);
elementStart(1, 'input', ['value', 'one'], null, ['myInput', '']);
elementEnd();
}
let myInput = elementStart(1);
Expand All @@ -149,7 +149,7 @@ describe('exports', () => {
if (cm) {
elementStart(0, 'div');
elementEnd();
elementStart(1, 'input', ['value', 'one']);
elementStart(1, 'input', ['value', 'one'], null, ['myInput', '']);
elementEnd();
}
let myInput = elementStart(1);
Expand All @@ -165,7 +165,7 @@ describe('exports', () => {
if (cm) {
elementStart(0, 'div');
elementEnd();
elementStart(1, 'input', ['type', 'checkbox', 'checked', 'true']);
elementStart(1, 'input', ['type', 'checkbox', 'checked', 'true'], null, ['myInput', '']);
elementEnd();
}
let myInput = elementStart(1);
Expand Down Expand Up @@ -206,7 +206,7 @@ describe('exports', () => {
if (cm) {
elementStart(0, 'div', null, [MyDir]);
elementEnd();
elementStart(2, MyComponent);
elementStart(2, MyComponent, null, null, ['myComp', '']);
elementEnd();
}
elementProperty(0, 'myDir', bind(load<MyComponent>(3)));
Expand All @@ -223,9 +223,9 @@ describe('exports', () => {
if (cm) {
text(0);
text(1);
elementStart(2, MyComponent);
elementStart(2, MyComponent, null, null, ['myComp', '']);
elementEnd();
elementStart(4, 'input', ['value', 'one']);
elementStart(4, 'input', ['value', 'one'], null, ['myInput', '']);
elementEnd();
}
let myInput = elementStart(4);
Expand Down Expand Up @@ -265,7 +265,7 @@ describe('exports', () => {
{
if (cm1) {
text(0);
elementStart(1, 'input', ['value', 'one']);
elementStart(1, 'input', ['value', 'one'], null, ['myInput', '']);
elementEnd();
}
let myInput = elementStart(1);
Expand Down
112 changes: 109 additions & 3 deletions packages/core/test/render3/query_spec.ts
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/
import {QUERY_READ_CONTAINER_REF, QUERY_READ_ELEMENT_REF, QUERY_READ_FROM_NODE, QUERY_READ_TEMPLATE_REF} from '../../src/render3/di';
import {QueryList, detectChanges} from '../../src/render3/index';
import {QueryList, defineComponent, detectChanges} from '../../src/render3/index';
import {container, containerRefreshEnd, containerRefreshStart, elementEnd, elementStart, embeddedViewEnd, embeddedViewStart, load} from '../../src/render3/instructions';
import {query, queryRefresh} from '../../src/render3/query';

Expand Down Expand Up @@ -191,6 +191,42 @@ describe('query', () => {
expect(qList.first.nativeElement).toEqual(elToQuery);
});

it('should query multiple locals on the same element', () => {
let elToQuery;

/**
* <div #foo #bar></div>
* <div></div>
* class Cmpt {
* @ViewChildren('foo') fooQuery;
* @ViewChildren('bar') barQuery;
* }
*/
const Cmpt = createComponent('cmpt', function(ctx: any, cm: boolean) {
let tmp: any;
if (cm) {
query(0, ['foo'], false, QUERY_READ_FROM_NODE);
query(1, ['bar'], false, QUERY_READ_FROM_NODE);
elToQuery = elementStart(2, 'div', null, null, ['foo', '', 'bar', '']);
elementEnd();
elementStart(3, 'div');
elementEnd();
}
queryRefresh(tmp = load<QueryList<any>>(0)) && (ctx.fooQuery = tmp as QueryList<any>);
queryRefresh(tmp = load<QueryList<any>>(1)) && (ctx.barQuery = tmp as QueryList<any>);
});

const cmptInstance = renderComponent(Cmpt);

const fooList = (cmptInstance.fooQuery as QueryList<any>);
expect(fooList.length).toBe(1);
expect(fooList.first.nativeElement).toEqual(elToQuery);

const barList = (cmptInstance.barQuery as QueryList<any>);
expect(barList.length).toBe(1);
expect(barList.first.nativeElement).toEqual(elToQuery);
});

it('should query for multiple elements and read ElementRef by default', () => {

let el1ToQuery;
Expand Down Expand Up @@ -372,7 +408,7 @@ describe('query', () => {

let childInstance;
/**
* <cmpt #foo></cmpt>
* <child #foo></child>
* class Cmpt {
* @ViewChildren('foo') query;
* }
Expand All @@ -394,6 +430,41 @@ describe('query', () => {
expect(qList.first).toBe(childInstance);
});

it('should read component instance with explicit exportAs', () => {
let childInstance: Child;

class Child {
static ngComponentDef = defineComponent({
type: Child,
tag: 'child',
factory: () => childInstance = new Child(),
template: (ctx: Child, cm: boolean) => {},
exportAs: 'child'
});
}

/**
* <child #foo="child"></child>
* class Cmpt {
* @ViewChildren('foo') query;
* }
*/
const Cmpt = createComponent('cmpt', function(ctx: any, cm: boolean) {
let tmp: any;
if (cm) {
query(0, ['foo'], true, QUERY_READ_FROM_NODE);
elementStart(1, Child, null, null, ['foo', 'child']);
elementEnd();
}
queryRefresh(tmp = load<QueryList<any>>(0)) && (ctx.query = tmp as QueryList<any>);
});

const cmptInstance = renderComponent(Cmpt);
const qList = (cmptInstance.query as QueryList<any>);
expect(qList.length).toBe(1);
expect(qList.first).toBe(childInstance !);
});

it('should read directive instance if element queried for has an exported directive with a matching name',
() => {
const Child = createDirective({exportAs: 'child'});
Expand Down Expand Up @@ -454,7 +525,42 @@ describe('query', () => {
expect(qList.last).toBe(child2Instance);
});

it('should match match on exported directive name and read a requested token', () => {
it('should read multiple locals exporting the same directive from a given element', () => {
const Child = createDirective({exportAs: 'child'});
let childInstance;

/**
* <div child #foo="child" #bar="child"></div>
* class Cmpt {
* @ViewChildren('foo') fooQuery;
* @ViewChildren('bar') barQuery;
* }
*/
const Cmpt = createComponent('cmpt', function(ctx: any, cm: boolean) {
let tmp: any;
if (cm) {
query(0, ['foo'], true, QUERY_READ_FROM_NODE);
query(1, ['bar'], true, QUERY_READ_FROM_NODE);
elementStart(2, 'div', null, [Child], ['foo', 'child', 'bar', 'child']);
{ childInstance = load(3); }
elementEnd();
}
queryRefresh(tmp = load<QueryList<any>>(0)) && (ctx.fooQuery = tmp as QueryList<any>);
queryRefresh(tmp = load<QueryList<any>>(1)) && (ctx.barQuery = tmp as QueryList<any>);
});

const cmptInstance = renderComponent(Cmpt);

const fooList = cmptInstance.fooQuery as QueryList<any>;
expect(fooList.length).toBe(1);
expect(fooList.first).toBe(childInstance);

const barList = cmptInstance.barQuery as QueryList<any>;
expect(barList.length).toBe(1);
expect(barList.first).toBe(childInstance);
});

it('should match on exported directive name and read a requested token', () => {
const Child = createDirective({exportAs: 'child'});

let div;
Expand Down