From 0ffa2f2e730f88beb2605ba6cae21b9868c857c3 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 13 Mar 2019 19:30:38 +0100 Subject: [PATCH] fix(ivy): unable to inherit view queries into component from directive (#29203) Fixes components not being able to inherit their view queries from a directive. This PR resolves FW-1146. PR Close #29203 --- .../src/ngtsc/annotations/src/component.ts | 19 +------- .../src/ngtsc/annotations/src/directive.ts | 12 ++++- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 45 ++++++++++++++++--- .../compiler/src/compiler_facade_interface.ts | 2 +- packages/compiler/src/jit_compiler_facade.ts | 2 +- packages/compiler/src/render3/view/api.ts | 10 ++--- .../compiler/src/render3/view/compiler.ts | 10 ++--- .../src/compiler/compiler_facade_interface.ts | 2 +- packages/core/src/render3/definition.ts | 6 +++ .../features/inherit_definition_feature.ts | 23 +++++----- .../core/src/render3/interfaces/definition.ts | 7 +++ packages/core/src/render3/jit/directive.ts | 3 +- packages/core/test/acceptance/query_spec.ts | 34 +++++++++++++- packages/core/test/render3/ivy/jit_spec.ts | 10 ----- 14 files changed, 122 insertions(+), 63 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index cbbf3ddf53..73efd42a6f 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -145,7 +145,7 @@ export class ComponentDecoratorHandler implements } // Next, read the `@Component`-specific fields. - const {decoratedElements, decorator: component, metadata} = directiveResult; + const {decorator: component, metadata} = directiveResult; // Go through the root directories for this project, and select the one with the smallest // relative path representation. @@ -223,22 +223,6 @@ export class ComponentDecoratorHandler implements }); } - // Construct the list of view queries. - const coreModule = this.isCore ? undefined : '@angular/core'; - const viewChildFromFields = queriesFromFields( - filterToMembersWithDecorator(decoratedElements, 'ViewChild', coreModule), this.reflector, - this.evaluator); - const viewChildrenFromFields = queriesFromFields( - filterToMembersWithDecorator(decoratedElements, 'ViewChildren', coreModule), this.reflector, - this.evaluator); - const viewQueries = [...viewChildFromFields, ...viewChildrenFromFields]; - - if (component.has('queries')) { - const queriesFromDecorator = extractQueriesFromDecorator( - component.get('queries') !, this.reflector, this.evaluator, this.isCore); - viewQueries.push(...queriesFromDecorator.view); - } - // Figure out the set of styles. The ordering here is important: external resources (styleUrls) // precede inline styles, and styles defined in the template override styles defined in the // component. @@ -288,7 +272,6 @@ export class ComponentDecoratorHandler implements meta: { ...metadata, template, - viewQueries, encapsulation, interpolation: template.interpolation, styles: styles || [], diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index 8c3c231d59..623db57a42 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -160,10 +160,20 @@ export function extractDirectiveMetadata( const queries = [...contentChildFromFields, ...contentChildrenFromFields]; + // Construct the list of view queries. + const viewChildFromFields = queriesFromFields( + filterToMembersWithDecorator(decoratedElements, 'ViewChild', coreModule), reflector, + evaluator); + const viewChildrenFromFields = queriesFromFields( + filterToMembersWithDecorator(decoratedElements, 'ViewChildren', coreModule), reflector, + evaluator); + const viewQueries = [...viewChildFromFields, ...viewChildrenFromFields]; + if (directive.has('queries')) { const queriesFromDecorator = extractQueriesFromDecorator(directive.get('queries') !, reflector, evaluator, isCore); queries.push(...queriesFromDecorator.content); + viewQueries.push(...queriesFromDecorator.view); } // Parse the selector. @@ -213,7 +223,7 @@ export function extractDirectiveMetadata( usesOnChanges, }, inputs: {...inputsFromMeta, ...inputsFromFields}, - outputs: {...outputsFromMeta, ...outputsFromFields}, queries, selector, + outputs: {...outputsFromMeta, ...outputsFromFields}, queries, viewQueries, selector, type: new WrappedNodeExpr(clazz.name !), typeArgumentCount: reflector.getGenericArityOfClass(clazz) || 0, typeSourceSpan: null !, usesInheritance, exportAs, providers diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 285e83cc4f..d55682106a 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -983,7 +983,7 @@ describe('ngtsc behavioral tests', () => { env.tsconfig({strictInjectionParameters: true}); env.write('test.ts', ` import {Injectable} from '@angular/core'; - + @Injectable() export class Test { constructor(private notInjectable: string) {} @@ -1000,7 +1000,7 @@ describe('ngtsc behavioral tests', () => { env.tsconfig({strictInjectionParameters: true}); env.write('test.ts', ` import {Injectable} from '@angular/core'; - + @Injectable() export class Test { constructor(private notInjectable: string) {} @@ -1017,7 +1017,7 @@ describe('ngtsc behavioral tests', () => { env.tsconfig({strictInjectionParameters: true}); env.write('test.ts', ` import {Injectable} from '@angular/core'; - + @Injectable({ providedIn: 'root', useValue: '42', @@ -1358,6 +1358,41 @@ describe('ngtsc behavioral tests', () => { expect(jsContents).toMatch(viewQueryRegExp(true)); }); + it('should generate queries for directives', () => { + env.tsconfig(); + env.write(`test.ts`, ` + import {Directive, ContentChild, ContentChildren, TemplateRef, ViewChild} from '@angular/core'; + + @Directive({ + selector: '[test]', + queries: { + 'mview': new ViewChild('test1'), + 'mcontent': new ContentChild('test2'), + } + }) + class FooCmp { + @ContentChild('bar', {read: TemplateRef}) child: any; + @ContentChildren(TemplateRef) children: any; + get aview(): any { return null; } + @ViewChild('accessor') set aview(value: any) {} + } + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents).toMatch(varRegExp('bar')); + expect(jsContents).toMatch(varRegExp('test1')); + expect(jsContents).toMatch(varRegExp('test2')); + expect(jsContents).toMatch(varRegExp('accessor')); + // match `i0.ɵcontentQuery(dirIndex, _c1, true, TemplateRef)` + expect(jsContents).toMatch(contentQueryRegExp('\\w+', true, 'TemplateRef')); + + // match `i0.ɵviewQuery(_c2, true, null)` + // Note that while ViewQuery doesn't necessarily make sense on a directive, because it doesn't + // have a view, we still need to handle it because a component could extend the directive. + expect(jsContents).toMatch(viewQueryRegExp(true)); + }); + it('should handle queries that use forwardRef', () => { env.tsconfig(); env.write(`test.ts`, ` @@ -1931,7 +1966,7 @@ describe('ngtsc behavioral tests', () => { it('should be able to compile an app using the factory shim', () => { env.tsconfig({'allowEmptyCodegenFiles': true}); - env.write('test.ts', ` + env.write('test.ts', ` export {MyModuleNgFactory} from './my-module.ngfactory'; `); @@ -2217,7 +2252,7 @@ describe('ngtsc behavioral tests', () => { import {Component} from '@angular/core'; import {Other} from './types'; import Default from './types'; - + @Component({selector: 'test', template: 'test'}) export class SomeCmp { constructor(arg: Default, other: Other) {} diff --git a/packages/compiler/src/compiler_facade_interface.ts b/packages/compiler/src/compiler_facade_interface.ts index f069c587cb..83945a8546 100644 --- a/packages/compiler/src/compiler_facade_interface.ts +++ b/packages/compiler/src/compiler_facade_interface.ts @@ -129,13 +129,13 @@ export interface R3DirectiveMetadataFacade { usesInheritance: boolean; exportAs: string[]|null; providers: Provider[]|null; + viewQueries: R3QueryMetadataFacade[]; } export interface R3ComponentMetadataFacade extends R3DirectiveMetadataFacade { template: string; preserveWhitespaces: boolean; animations: any[]|undefined; - viewQueries: R3QueryMetadataFacade[]; pipes: Map; directives: {selector: string, expression: any}[]; styles: string[]; diff --git a/packages/compiler/src/jit_compiler_facade.ts b/packages/compiler/src/jit_compiler_facade.ts index c7fbfe50db..7e50de5311 100644 --- a/packages/compiler/src/jit_compiler_facade.ts +++ b/packages/compiler/src/jit_compiler_facade.ts @@ -133,7 +133,6 @@ export class CompilerFacadeImpl implements CompilerFacade { ...convertDirectiveFacadeToMetadata(facade), selector: facade.selector || this.elementSchemaRegistry.getDefaultComponentElementName(), template, - viewQueries: facade.viewQueries.map(convertToR3QueryMetadata), wrapDirectivesAndPipesInClosure: false, styles: facade.styles || [], encapsulation: facade.encapsulation as any, @@ -235,6 +234,7 @@ function convertDirectiveFacadeToMetadata(facade: R3DirectiveMetadataFacade): R3 outputs: {...outputsFromMetadata, ...outputsFromType}, queries: facade.queries.map(convertToR3QueryMetadata), providers: facade.providers != null ? new WrappedNodeExpr(facade.providers) : null, + viewQueries: facade.viewQueries.map(convertToR3QueryMetadata), }; } diff --git a/packages/compiler/src/render3/view/api.ts b/packages/compiler/src/render3/view/api.ts index a164e887df..c6deb2b288 100644 --- a/packages/compiler/src/render3/view/api.ts +++ b/packages/compiler/src/render3/view/api.ts @@ -53,6 +53,11 @@ export interface R3DirectiveMetadata { */ queries: R3QueryMetadata[]; + /** + * Information about the view queries made by the directive. + */ + viewQueries: R3QueryMetadata[]; + /** * Mappings indicating how the directive interacts with its host element (host bindings, * listeners, etc). @@ -128,11 +133,6 @@ export interface R3ComponentMetadata extends R3DirectiveMetadata { nodes: t.Node[]; }; - /** - * Information about the view queries made by the component. - */ - viewQueries: R3QueryMetadata[]; - /** * A map of pipe names to an expression referencing the pipe type which are in the scope of the * compilation. diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index f9a8771bb5..ddce8582e2 100644 --- a/packages/compiler/src/render3/view/compiler.ts +++ b/packages/compiler/src/render3/view/compiler.ts @@ -69,6 +69,10 @@ function baseDirectiveFields( definitionMap.set('contentQueries', createContentQueriesFunction(meta, constantPool)); } + if (meta.viewQueries.length) { + definitionMap.set('viewQuery', createViewQueriesFunction(meta, constantPool)); + } + // Initialize hostVarsCount to number of bound host properties (interpolations illegal), // except 'style' and 'class' properties, since they should *not* allocate host var slots const hostVarsCount = Object.keys(meta.host.properties) @@ -228,10 +232,6 @@ export function compileComponentFromMetadata( directiveMatcher = matcher; } - if (meta.viewQueries.length) { - definitionMap.set('viewQuery', createViewQueriesFunction(meta, constantPool)); - } - // e.g. `template: function MyComponent_Template(_ctx, _cm) {...}` const templateTypeName = meta.name; const templateName = templateTypeName ? `${templateTypeName}_Template` : null; @@ -549,7 +549,7 @@ function createTypeForDef(meta: R3DirectiveMetadata, typeBase: o.ExternalReferen // Define and update any view queries function createViewQueriesFunction( - meta: R3ComponentMetadata, constantPool: ConstantPool): o.Expression { + meta: R3DirectiveMetadata, constantPool: ConstantPool): o.Expression { const createStatements: o.Statement[] = []; const updateStatements: o.Statement[] = []; const tempAllocator = temporaryAllocator(updateStatements, TEMPORARY_NAME); diff --git a/packages/core/src/compiler/compiler_facade_interface.ts b/packages/core/src/compiler/compiler_facade_interface.ts index 954cc0ad89..7e31746bb0 100644 --- a/packages/core/src/compiler/compiler_facade_interface.ts +++ b/packages/core/src/compiler/compiler_facade_interface.ts @@ -129,13 +129,13 @@ export interface R3DirectiveMetadataFacade { usesInheritance: boolean; exportAs: string[]|null; providers: Provider[]|null; + viewQueries: R3QueryMetadataFacade[]; } export interface R3ComponentMetadataFacade extends R3DirectiveMetadataFacade { template: string; preserveWhitespaces: boolean; animations: any[]|undefined; - viewQueries: R3QueryMetadataFacade[]; pipes: Map; directives: {selector: string, expression: any}[]; styles: string[]; diff --git a/packages/core/src/render3/definition.ts b/packages/core/src/render3/definition.ts index 5f6bc7eb5d..a887acb1df 100644 --- a/packages/core/src/render3/definition.ts +++ b/packages/core/src/render3/definition.ts @@ -597,6 +597,12 @@ export const defineDirective = defineComponent as any as(directiveDefinition: */ contentQueries?: ContentQueriesFunction; + /** + * Additional set of instructions specific to view query processing. This could be seen as a + * set of instructions to be inserted into the template function. + */ + viewQuery?: ViewQueriesFunction| null; + /** * Defines the name that can be used in the template to assign this directive to a variable. * diff --git a/packages/core/src/render3/features/inherit_definition_feature.ts b/packages/core/src/render3/features/inherit_definition_feature.ts index 201e78302b..4e6f3e6152 100644 --- a/packages/core/src/render3/features/inherit_definition_feature.ts +++ b/packages/core/src/render3/features/inherit_definition_feature.ts @@ -73,18 +73,17 @@ export function InheritDefinitionFeature(definition: DirectiveDef| Componen } // Merge View Queries - if (isComponentDef(definition) && isComponentDef(superDef)) { - const prevViewQuery = definition.viewQuery; - const superViewQuery = superDef.viewQuery; - if (superViewQuery) { - if (prevViewQuery) { - definition.viewQuery = (rf: RenderFlags, ctx: T): void => { - superViewQuery(rf, ctx); - prevViewQuery(rf, ctx); - }; - } else { - definition.viewQuery = superViewQuery; - } + const prevViewQuery = definition.viewQuery; + const superViewQuery = superDef.viewQuery; + + if (superViewQuery) { + if (prevViewQuery) { + definition.viewQuery = (rf: RenderFlags, ctx: T): void => { + superViewQuery(rf, ctx); + prevViewQuery(rf, ctx); + }; + } else { + definition.viewQuery = superViewQuery; } } diff --git a/packages/core/src/render3/interfaces/definition.ts b/packages/core/src/render3/interfaces/definition.ts index f60da2940b..2f285bad69 100644 --- a/packages/core/src/render3/interfaces/definition.ts +++ b/packages/core/src/render3/interfaces/definition.ts @@ -158,6 +158,13 @@ export interface DirectiveDef extends BaseDef { */ contentQueries: ContentQueriesFunction|null; + /** + * Query-related instructions for a directive. Note that while directives don't have a + * view and as such view queries won't necessarily do anything, there might be + * components that extend the directive. + */ + viewQuery: ViewQueriesFunction|null; + /** * Refreshes host bindings on the associated directive. */ diff --git a/packages/core/src/render3/jit/directive.ts b/packages/core/src/render3/jit/directive.ts index 86e19f6870..07387940d2 100644 --- a/packages/core/src/render3/jit/directive.ts +++ b/packages/core/src/render3/jit/directive.ts @@ -63,8 +63,6 @@ export function compileComponent(type: Type, metadata: Component): void { preserveWhitespaces: metadata.preserveWhitespaces || false, styles: metadata.styles || EMPTY_ARRAY, animations: metadata.animations, - viewQueries: - extractQueriesMetadata(type, getReflect().ownPropMetadata(type), isViewQuery), directives: [], changeDetection: metadata.changeDetection, pipes: new Map(), @@ -157,6 +155,7 @@ export function directiveMetadata(type: Type, metadata: Directive): R3Direc usesInheritance: !extendsDirectlyFromObject(type), exportAs: extractExportAs(metadata.exportAs), providers: metadata.providers || null, + viewQueries: extractQueriesMetadata(type, propMetadata, isViewQuery), }; } diff --git a/packages/core/test/acceptance/query_spec.ts b/packages/core/test/acceptance/query_spec.ts index 758b9ebcb9..a3e971ae9a 100644 --- a/packages/core/test/acceptance/query_spec.ts +++ b/packages/core/test/acceptance/query_spec.ts @@ -17,7 +17,8 @@ describe('query logic', () => { declarations: [ AppComp, QueryComp, SimpleCompA, SimpleCompB, StaticViewQueryComp, TextDirective, SubclassStaticViewQueryComp, StaticContentQueryComp, SubclassStaticContentQueryComp, - QueryCompWithChanges, StaticContentQueryDir + QueryCompWithChanges, StaticContentQueryDir, SuperDirectiveQueryTarget, SuperDirective, + SubComponent ] }); }); @@ -161,6 +162,17 @@ describe('query logic', () => { expect(secondComponent.setEvents).toEqual(['textDir set', 'foo set']); }); + it('should allow for view queries to be inherited from a directive', () => { + const fixture = TestBed.createComponent(SubComponent); + const comp = fixture.componentInstance; + fixture.detectChanges(); + + expect(comp.headers).toBeTruthy(); + expect(comp.headers.length).toBe(2); + expect(comp.headers.toArray().every(result => result instanceof SuperDirectiveQueryTarget)) + .toBe(true); + }); + }); describe('content queries', () => { @@ -484,7 +496,7 @@ class StaticViewQueryComp { template: `
- +
` @@ -564,3 +576,21 @@ export class QueryCompWithChanges { showing = false; } + +@Component({selector: 'query-target', template: ''}) +class SuperDirectiveQueryTarget { +} + +@Directive({selector: '[super-directive]'}) +class SuperDirective { + @ViewChildren(SuperDirectiveQueryTarget) headers !: QueryList; +} + +@Component({ + template: ` + One + Two + ` +}) +class SubComponent extends SuperDirective { +} diff --git a/packages/core/test/render3/ivy/jit_spec.ts b/packages/core/test/render3/ivy/jit_spec.ts index f0cfaab740..e6dae227f3 100644 --- a/packages/core/test/render3/ivy/jit_spec.ts +++ b/packages/core/test/render3/ivy/jit_spec.ts @@ -349,16 +349,6 @@ ivyEnabled && describe('render3 jit', () => { expect((TestDirective as any).ngDirectiveDef.contentQueries).not.toBeNull(); }); - it('should not pick up view queries from directives', () => { - @Directive({selector: '[test]'}) - class TestDirective { - @ViewChildren('foo') foos: QueryList|undefined; - } - - expect((TestDirective as any).ngDirectiveDef.contentQueries).toBeNull(); - expect((TestDirective as any).ngDirectiveDef.viewQuery).toBeNull(); - }); - it('should compile ViewChild query on a component', () => { @Component({selector: 'test', template: ''}) class TestComponent {