From f0b0d64453b341b678724b57f394a5d08464751f Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Fri, 30 Nov 2018 15:01:37 -0800 Subject: [PATCH] fix(ivy): adding `projectDef` instructions to all templates where is present (FW-745) (#27384) Prior to this change `projectDef` instructions were placed to root templates only, thus the necessary information (selectors) in nested templates was missing. This update adds the logic to insert `projectDef` instructions to all templates where is present. PR Close #27384 --- .../compliance/r3_compiler_compliance_spec.ts | 76 ++++++++++++++++++- packages/compiler/src/render3/r3_ast.ts | 2 +- .../src/render3/r3_template_transform.ts | 24 +----- packages/compiler/src/render3/view/api.ts | 10 --- .../compiler/src/render3/view/compiler.ts | 9 +-- .../compiler/src/render3/view/template.ts | 74 ++++++++++-------- .../render3/r3_template_transform_spec.ts | 42 +++------- 7 files changed, 132 insertions(+), 105 deletions(-) diff --git a/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts b/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts index 8aaf4273a0..4afe687e2f 100644 --- a/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts @@ -1013,7 +1013,7 @@ describe('compiler compliance', () => { }); }); - it('should support content projection', () => { + it('should support content projection in root template', () => { const files = { app: { 'spec.ts': ` @@ -1061,10 +1061,10 @@ describe('compiler compliance', () => { });`; const ComplexComponentDefinition = ` - const $c1$ = [[["span", "title", "tofirst"]], [["span", "title", "tosecond"]]]; - const $c2$ = ["span[title=toFirst]", "span[title=toSecond]"]; const $c3$ = ["id","first"]; const $c4$ = ["id","second"]; + const $c1$ = [[["span", "title", "tofirst"]], [["span", "title", "tosecond"]]]; + const $c2$ = ["span[title=toFirst]", "span[title=toSecond]"]; … ComplexComponent.ngComponentDef = $r3$.ɵdefineComponent({ type: ComplexComponent, @@ -1095,6 +1095,76 @@ describe('compiler compliance', () => { result.source, ComplexComponentDefinition, 'Incorrect ComplexComponent definition'); }); + it('should support content projection in nested templates', () => { + const files = { + app: { + 'spec.ts': ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + template: \` +
+ +
+
+ No ng-content, no instructions generated. +
+ + '*' selector: + + \`, + }) + class Cmp {} + + @NgModule({ declarations: [Cmp] }) + class Module {} + ` + } + }; + const output = ` + const $_c0$ = [1, "ngIf"]; + const $_c1$ = ["id", "second"]; + const $_c2$ = [[["span", "title", "tofirst"]]]; + const $_c3$ = ["span[title=toFirst]"]; + function Cmp_div_Template_0(rf, ctx) { if (rf & 1) { + $r3$.ɵprojectionDef($_c2$, $_c3$); + $r3$.ɵelementStart(0, "div", $_c1$); + $r3$.ɵprojection(1, 1); + $r3$.ɵelementEnd(); + } } + const $_c4$ = ["id", "third"]; + function Cmp_div_Template_1(rf, ctx) { + if (rf & 1) { + $r3$.ɵelementStart(0, "div", $_c4$); + $r3$.ɵtext(1, " No ng-content, no instructions generated. "); + $r3$.ɵelementEnd(); + } + } + function Template_2(rf, ctx) { + if (rf & 1) { + $r3$.ɵprojectionDef(); + $r3$.ɵtext(0, " '*' selector: "); + $r3$.ɵprojection(1); + } + } + … + template: function Cmp_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵtemplate(0, Cmp_div_Template_0, 2, 0, null, $_c0$); + $r3$.ɵtemplate(1, Cmp_div_Template_1, 2, 0, null, $_c0$); + $r3$.ɵtemplate(2, Template_2, 2, 0); + } + if (rf & 2) { + $r3$.ɵelementProperty(0, "ngIf", $r3$.ɵbind(ctx.visible)); + $r3$.ɵelementProperty(1, "ngIf", $r3$.ɵbind(ctx.visible)); + } + } + `; + + const {source} = compile(files, angularFiles); + expectEmit(source, output, 'Invalid content projection instructions generated'); + }); + describe('queries', () => { const directive = { 'some.directive.ts': ` diff --git a/packages/compiler/src/render3/r3_ast.ts b/packages/compiler/src/render3/r3_ast.ts index 9501af0048..e524e157b1 100644 --- a/packages/compiler/src/render3/r3_ast.ts +++ b/packages/compiler/src/render3/r3_ast.ts @@ -83,7 +83,7 @@ export class Template implements Node { export class Content implements Node { constructor( - public selectorIndex: number, public attributes: TextAttribute[], + public selector: string, public attributes: TextAttribute[], public sourceSpan: ParseSourceSpan, public i18n?: I18nAST) {} visit(visitor: Visitor): Result { return visitor.visitContent(this); } } diff --git a/packages/compiler/src/render3/r3_template_transform.ts b/packages/compiler/src/render3/r3_template_transform.ts index 9bd4d4caa1..284b103b1f 100644 --- a/packages/compiler/src/render3/r3_template_transform.ts +++ b/packages/compiler/src/render3/r3_template_transform.ts @@ -45,16 +45,10 @@ const IDENT_PROPERTY_IDX = 9; const IDENT_EVENT_IDX = 10; const TEMPLATE_ATTR_PREFIX = '*'; -// Default selector used by `` if none specified -const DEFAULT_CONTENT_SELECTOR = '*'; // Result of the html AST to Ivy AST transformation export type Render3ParseResult = { nodes: t.Node[]; errors: ParseError[]; - // Any non default (empty or '*') selector found in the template - ngContentSelectors: string[]; - // Wether the template contains any `` - hasNgContent: boolean; }; export function htmlAstToRender3Ast( @@ -74,17 +68,11 @@ export function htmlAstToRender3Ast( return { nodes: ivyNodes, errors: allErrors, - ngContentSelectors: transformer.ngContentSelectors, - hasNgContent: transformer.hasNgContent, }; } class HtmlAstToIvyAst implements html.Visitor { errors: ParseError[] = []; - // Selectors for the `ng-content` tags. Only non `*` selectors are recorded here - ngContentSelectors: string[] = []; - // Any `` in the template ? - hasNgContent = false; constructor(private bindingParser: BindingParser) {} @@ -168,20 +156,12 @@ class HtmlAstToIvyAst implements html.Visitor { let parsedElement: t.Node|undefined; if (preparsedElement.type === PreparsedElementType.NG_CONTENT) { // `` - this.hasNgContent = true; - if (element.children && !element.children.every(isEmptyTextNode)) { this.reportError(` element cannot have content.`, element.sourceSpan); } - const selector = preparsedElement.selectAttr; - - let attributes: t.TextAttribute[] = - element.attrs.map(attribute => this.visitAttribute(attribute)); - - const selectorIndex = - selector === DEFAULT_CONTENT_SELECTOR ? 0 : this.ngContentSelectors.push(selector); - parsedElement = new t.Content(selectorIndex, attributes, element.sourceSpan, element.i18n); + const attrs: t.TextAttribute[] = element.attrs.map(attr => this.visitAttribute(attr)); + parsedElement = new t.Content(selector, attrs, element.sourceSpan, element.i18n); } else if (isTemplateElement) { // `` const attrs = this.extractAttributes(element.name, parsedProperties, i18nAttrsMeta); diff --git a/packages/compiler/src/render3/view/api.ts b/packages/compiler/src/render3/view/api.ts index 8fb10cc046..ae0929bcaf 100644 --- a/packages/compiler/src/render3/view/api.ts +++ b/packages/compiler/src/render3/view/api.ts @@ -123,16 +123,6 @@ export interface R3ComponentMetadata extends R3DirectiveMetadata { * Parsed nodes of the template. */ nodes: t.Node[]; - - /** - * Whether the template includes tags. - */ - hasNgContent: boolean; - - /** - * Selectors found in the tags in the template. - */ - ngContentSelectors: string[]; }; /** diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index 30a122e232..908a16569f 100644 --- a/packages/compiler/src/render3/view/compiler.ts +++ b/packages/compiler/src/render3/view/compiler.ts @@ -258,8 +258,7 @@ export function compileComponentFromMetadata( meta.viewQueries, directiveMatcher, directivesUsed, meta.pipes, pipesUsed, R3.namespaceHTML, meta.relativeContextFilePath, meta.i18nUseExternalIds); - const templateFunctionExpression = templateBuilder.buildTemplateFunction( - template.nodes, [], template.hasNgContent, template.ngContentSelectors); + const templateFunctionExpression = templateBuilder.buildTemplateFunction(template.nodes, []); // e.g. `consts: 2` definitionMap.set('consts', o.literal(templateBuilder.getConstCount())); @@ -371,11 +370,7 @@ export function compileComponentFromRender2( const meta: R3ComponentMetadata = { ...directiveMetadataFromGlobalMetadata(component, outputCtx, reflector), selector: component.selector, - template: { - nodes: render3Ast.nodes, - hasNgContent: render3Ast.hasNgContent, - ngContentSelectors: render3Ast.ngContentSelectors, - }, + template: {nodes: render3Ast.nodes}, directives: [], pipes: typeMapToExpressionMap(pipeTypeByName, outputCtx), viewQueries: queriesFromGlobalMetadata(component.viewQueries, outputCtx), diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 29ca6fbe0b..ff3eabe2cc 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -58,6 +58,8 @@ export function renderFlagCheckIfStmt( return o.ifStmt(o.variable(RENDER_FLAGS).bitwiseAnd(o.literal(flags), null, false), statements); } +// Default selector used by `` if none specified +const DEFAULT_CONTENT_SELECTOR = '*'; export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver { private _dataIndex = 0; private _bindingContext = 0; @@ -102,6 +104,12 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver private fileBasedI18nSuffix: string; + // Whether the template includes tags. + private _hasNgContent: boolean = false; + + // Selectors found in the tags in the template. + private _ngContentSelectors: string[] = []; + constructor( private constantPool: ConstantPool, parentBindingScope: BindingScope, private level = 0, private contextName: string|null, private i18nContext: I18nContext|null, @@ -154,9 +162,7 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver }); } - buildTemplateFunction( - nodes: t.Node[], variables: t.Variable[], hasNgContent: boolean = false, - ngContentSelectors: string[] = [], i18n?: i18n.AST): o.FunctionExpr { + buildTemplateFunction(nodes: t.Node[], variables: t.Variable[], i18n?: i18n.AST): o.FunctionExpr { if (this._namespace !== R3.namespaceHTML) { this.creationInstruction(null, this._namespace); } @@ -164,22 +170,6 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver // Create variable bindings variables.forEach(v => this.registerContextVariables(v)); - // Output a `ProjectionDef` instruction when some `` are present - if (hasNgContent) { - const parameters: o.Expression[] = []; - - // Only selectors with a non-default value are generated - if (ngContentSelectors.length > 1) { - const r3Selectors = ngContentSelectors.map(s => core.parseSelectorToR3Selector(s)); - // `projectionDef` needs both the parsed and raw value of the selectors - const parsed = this.constantPool.getConstLiteral(asLiteral(r3Selectors), true); - const unParsed = this.constantPool.getConstLiteral(asLiteral(ngContentSelectors), true); - parameters.push(parsed, unParsed); - } - - this.creationInstruction(null, R3.projectionDef, parameters); - } - // Initiate i18n context in case: // - this template has parent i18n context // - or the template has i18n meta associated with it, @@ -198,6 +188,26 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver // resolving bindings. We also count bindings in this pass as we walk bound expressions. t.visitAll(this, nodes); + // Output a `ProjectionDef` instruction when some `` are present + if (this._hasNgContent) { + const parameters: o.Expression[] = []; + + // Only selectors with a non-default value are generated + if (this._ngContentSelectors.length) { + const r3Selectors = this._ngContentSelectors.map(s => core.parseSelectorToR3Selector(s)); + // `projectionDef` needs both the parsed and raw value of the selectors + const parsed = this.constantPool.getConstLiteral(asLiteral(r3Selectors), true); + const unParsed = + this.constantPool.getConstLiteral(asLiteral(this._ngContentSelectors), true); + parameters.push(parsed, unParsed); + } + + // Since we accumulate ngContent selectors while processing template elements, + // we *prepend* `projectionDef` to creation instructions block, to put it before + // any `projection` instructions + this.creationInstruction(null, R3.projectionDef, parameters, /* prepend */ true); + } + // Add total binding count to pure function count so pure function instructions are // generated with the correct slot offset when update instructions are processed. this._pureFunctionSlots += this._bindingSlots; @@ -399,8 +409,11 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver } visitContent(ngContent: t.Content) { + this._hasNgContent = true; const slot = this.allocateDataSlot(); - const selectorIndex = ngContent.selectorIndex; + let selectorIndex = ngContent.selector === DEFAULT_CONTENT_SELECTOR ? + 0 : + this._ngContentSelectors.push(ngContent.selector); const parameters: o.Expression[] = [o.literal(slot)]; const attributeAsList: string[] = []; @@ -724,7 +737,7 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver // template definition. e.g.
{{ foo }}
this._nestedTemplateFns.push(() => { const templateFunctionExpr = templateVisitor.buildTemplateFunction( - template.children, template.variables, false, [], template.i18n); + template.children, template.variables, template.i18n); this.constantPool.statements.push(templateFunctionExpr.toDeclStmt(templateName, null)); }); @@ -834,8 +847,8 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver // bindings. e.g. {{ foo }}
private instructionFn( fns: (() => o.Statement)[], span: ParseSourceSpan|null, reference: o.ExternalReference, - paramsOrFn: o.Expression[]|(() => o.Expression[])): void { - fns.push(() => { + paramsOrFn: o.Expression[]|(() => o.Expression[]), prepend: boolean = false): void { + fns[prepend ? 'unshift' : 'push'](() => { const params = Array.isArray(paramsOrFn) ? paramsOrFn : paramsOrFn(); return instruction(span, reference, params).toStmt(); }); @@ -856,8 +869,8 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver private creationInstruction( span: ParseSourceSpan|null, reference: o.ExternalReference, - paramsOrFn?: o.Expression[]|(() => o.Expression[])) { - this.instructionFn(this._creationCodeFns, span, reference, paramsOrFn || []); + paramsOrFn?: o.Expression[]|(() => o.Expression[]), prepend?: boolean) { + this.instructionFn(this._creationCodeFns, span, reference, paramsOrFn || [], prepend); } private updateInstruction( @@ -1398,14 +1411,14 @@ function interpolate(args: o.Expression[]): o.Expression { export function parseTemplate( template: string, templateUrl: string, options: {preserveWhitespaces?: boolean, interpolationConfig?: InterpolationConfig} = {}): - {errors?: ParseError[], nodes: t.Node[], hasNgContent: boolean, ngContentSelectors: string[]} { + {errors?: ParseError[], nodes: t.Node[]} { const {interpolationConfig, preserveWhitespaces} = options; const bindingParser = makeBindingParser(interpolationConfig); const htmlParser = new HtmlParser(); const parseResult = htmlParser.parse(template, templateUrl, true, interpolationConfig); if (parseResult.errors && parseResult.errors.length > 0) { - return {errors: parseResult.errors, nodes: [], hasNgContent: false, ngContentSelectors: []}; + return {errors: parseResult.errors, nodes: []}; } let rootNodes: html.Node[] = parseResult.rootNodes; @@ -1428,13 +1441,12 @@ export function parseTemplate( new I18nMetaVisitor(interpolationConfig, /* keepI18nAttrs */ false), rootNodes); } - const {nodes, hasNgContent, ngContentSelectors, errors} = - htmlAstToRender3Ast(rootNodes, bindingParser); + const {nodes, errors} = htmlAstToRender3Ast(rootNodes, bindingParser); if (errors && errors.length > 0) { - return {errors, nodes: [], hasNgContent: false, ngContentSelectors: []}; + return {errors, nodes: []}; } - return {nodes, hasNgContent, ngContentSelectors}; + return {nodes}; } /** diff --git a/packages/compiler/test/render3/r3_template_transform_spec.ts b/packages/compiler/test/render3/r3_template_transform_spec.ts index 477fac1be0..5e8a541b2a 100644 --- a/packages/compiler/test/render3/r3_template_transform_spec.ts +++ b/packages/compiler/test/render3/r3_template_transform_spec.ts @@ -39,7 +39,7 @@ class R3AstHumanizer implements t.Visitor { } visitContent(content: t.Content) { - this.result.push(['Content', content.selectorIndex]); + this.result.push(['Content', content.selector]); t.visitAll(this, content.attributes); } @@ -110,17 +110,15 @@ describe('R3 template transform', () => { it('should parse ngContent', () => { const res = parse(''); - expect(res.hasNgContent).toEqual(true); - expect(res.ngContentSelectors).toEqual(['a']); expectFromR3Nodes(res.nodes).toEqual([ - ['Content', 1], + ['Content', 'a'], ['TextAttribute', 'select', 'a'], ]); }); it('should parse ngContent when it contains WS only', () => { expectFromHtml(' \n ').toEqual([ - ['Content', 1], + ['Content', 'a'], ['TextAttribute', 'select', 'a'], ]); }); @@ -128,7 +126,7 @@ describe('R3 template transform', () => { it('should parse ngContent regardless the namespace', () => { expectFromHtml('').toEqual([ ['Element', ':svg:svg'], - ['Content', 1], + ['Content', 'a'], ['TextAttribute', 'select', 'a'], ]); }); @@ -377,30 +375,16 @@ describe('R3 template transform', () => { describe('ng-content', () => { it('should parse ngContent without selector', () => { const res = parse(''); - expect(res.hasNgContent).toEqual(true); - expect(res.ngContentSelectors).toEqual([]); expectFromR3Nodes(res.nodes).toEqual([ - ['Content', 0], - ]); - }); - - it('should parse ngContent with a * selector', () => { - const res = parse(''); - const selectors = ['']; - expect(res.hasNgContent).toEqual(true); - expect(res.ngContentSelectors).toEqual([]); - expectFromR3Nodes(res.nodes).toEqual([ - ['Content', 0], + ['Content', '*'], ]); }); it('should parse ngContent with a specific selector', () => { const res = parse(''); const selectors = ['', 'tag[attribute]']; - expect(res.hasNgContent).toEqual(true); - expect(res.ngContentSelectors).toEqual(['tag[attribute]']); expectFromR3Nodes(res.nodes).toEqual([ - ['Content', 1], + ['Content', selectors[1]], ['TextAttribute', 'select', selectors[1]], ]); }); @@ -408,24 +392,20 @@ describe('R3 template transform', () => { it('should parse ngContent with a selector', () => { const res = parse( ''); - const selectors = ['', 'a', 'b']; - expect(res.hasNgContent).toEqual(true); - expect(res.ngContentSelectors).toEqual(['a', 'b']); + const selectors = ['*', 'a', 'b']; expectFromR3Nodes(res.nodes).toEqual([ - ['Content', 1], + ['Content', selectors[1]], ['TextAttribute', 'select', selectors[1]], - ['Content', 0], - ['Content', 2], + ['Content', selectors[0]], + ['Content', selectors[2]], ['TextAttribute', 'select', selectors[2]], ]); }); it('should parse ngProjectAs as an attribute', () => { const res = parse(''); - expect(res.hasNgContent).toEqual(true); - expect(res.ngContentSelectors).toEqual([]); expectFromR3Nodes(res.nodes).toEqual([ - ['Content', 0], + ['Content', '*'], ['TextAttribute', 'ngProjectAs', 'a'], ]); });