From bf641e1b4b47ab4ca77332bb27d48e96c2e3572b Mon Sep 17 00:00:00 2001 From: crisbeto Date: Wed, 15 Jul 2020 07:03:04 +0200 Subject: [PATCH] fix(core): incorrectly validating properties on ng-content and ng-container (#37773) Fixes the following issues related to how we validate properties during JIT: - The invalid property warning was printing `null` as the node name for `ng-content`. The problem is that when generating a template from `ng-content` we weren't capturing the node name. - We weren't running property validation on `ng-container` at all. This used to be supported on ViewEngine and seems like an oversight. In the process of making these changes, I found and cleaned up a few places where we were passing in `LView` unnecessarily. PR Close #37773 --- .../compliance/r3_compiler_compliance_spec.ts | 37 +++++++++++ packages/compiler/src/render3/r3_ast.ts | 2 + .../src/render3/r3_template_transform.ts | 8 +-- .../core/src/render3/instructions/element.ts | 6 +- .../core/src/render3/instructions/shared.ts | 14 ++-- .../core/test/acceptance/ng_module_spec.ts | 64 +++++++++++++++++++ 6 files changed, 116 insertions(+), 15 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 3020efab87..aeb7cca37f 100644 --- a/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts @@ -1551,6 +1551,43 @@ describe('compiler compliance', () => { const result = compile(files, angularFiles); expectEmit(result.source, SimpleComponentDefinition, 'Incorrect MyApp definition'); }); + + it('should capture the node name of ng-content with a structural directive', () => { + const files = { + app: { + 'spec.ts': ` + import {Component, Directive, NgModule, TemplateRef} from '@angular/core'; + + @Component({selector: 'simple', template: ''}) + export class SimpleComponent {} + ` + } + }; + + const SimpleComponentDefinition = ` + SimpleComponent.ɵcmp = $r3$.ɵɵdefineComponent({ + type: SimpleComponent, + selectors: [["simple"]], + ngContentSelectors: $c0$, + decls: 1, + vars: 1, + consts: [[4, "ngIf"]], + template: function SimpleComponent_Template(rf, ctx) { + if (rf & 1) { + i0.ɵɵprojectionDef(); + i0.ɵɵtemplate(0, SimpleComponent_ng_content_0_Template, 1, 0, "ng-content", 0); + } + if (rf & 2) { + i0.ɵɵproperty("ngIf", ctx.showContent); + } + }, + encapsulation: 2 + });`; + + const result = compile(files, angularFiles); + expectEmit( + result.source, SimpleComponentDefinition, 'Incorrect SimpleComponent definition'); + }); }); describe('queries', () => { diff --git a/packages/compiler/src/render3/r3_ast.ts b/packages/compiler/src/render3/r3_ast.ts index c8273dbe56..23ec4d6b69 100644 --- a/packages/compiler/src/render3/r3_ast.ts +++ b/packages/compiler/src/render3/r3_ast.ts @@ -104,6 +104,8 @@ export class Template implements Node { } export class Content implements Node { + readonly name = 'ng-content'; + constructor( public selector: string, public attributes: TextAttribute[], public sourceSpan: ParseSourceSpan, public i18n?: I18nMeta) {} diff --git a/packages/compiler/src/render3/r3_template_transform.ts b/packages/compiler/src/render3/r3_template_transform.ts index 7d1ffeba28..52a5e51406 100644 --- a/packages/compiler/src/render3/r3_template_transform.ts +++ b/packages/compiler/src/render3/r3_template_transform.ts @@ -233,10 +233,10 @@ class HtmlAstToIvyAst implements html.Visitor { // TODO(pk): test for this case parsedElement = new t.Template( - (parsedElement as t.Element).name, hoistedAttrs.attributes, hoistedAttrs.inputs, - hoistedAttrs.outputs, templateAttrs, [parsedElement], [/* no references */], - templateVariables, element.sourceSpan, element.startSourceSpan, element.endSourceSpan, - i18n); + (parsedElement as t.Element | t.Content).name, hoistedAttrs.attributes, + hoistedAttrs.inputs, hoistedAttrs.outputs, templateAttrs, [parsedElement], + [/* no references */], templateVariables, element.sourceSpan, element.startSourceSpan, + element.endSourceSpan, i18n); } if (isI18nRootElement) { this.inI18nBlock = false; diff --git a/packages/core/src/render3/instructions/element.ts b/packages/core/src/render3/instructions/element.ts index 83b460de40..2b9b82c73f 100644 --- a/packages/core/src/render3/instructions/element.ts +++ b/packages/core/src/render3/instructions/element.ts @@ -37,7 +37,7 @@ function elementStartFirstCreatePass( const hasDirectives = resolveDirectives(tView, lView, tNode, getConstant(tViewConsts, localRefsIndex)); - ngDevMode && logUnknownElementError(tView, lView, native, tNode, hasDirectives); + ngDevMode && logUnknownElementError(tView, native, tNode, hasDirectives); if (tNode.attrs !== null) { computeStaticStyling(tNode, tNode.attrs, false); @@ -178,7 +178,7 @@ export function ɵɵelement( } function logUnknownElementError( - tView: TView, lView: LView, element: RElement, tNode: TNode, hasDirectives: boolean): void { + tView: TView, element: RElement, tNode: TNode, hasDirectives: boolean): void { const schemas = tView.schemas; // If `schemas` is set to `null`, that's an indication that this Component was compiled in AOT @@ -202,7 +202,7 @@ function logUnknownElementError( (typeof customElements !== 'undefined' && tagName.indexOf('-') > -1 && !customElements.get(tagName)); - if (isUnknown && !matchingSchemas(tView, lView, tagName)) { + if (isUnknown && !matchingSchemas(tView, tagName)) { let message = `'${tagName}' is not a known element:\n`; message += `1. If '${ tagName}' is an Angular component, then verify that it is part of this module.\n`; diff --git a/packages/core/src/render3/instructions/shared.ts b/packages/core/src/render3/instructions/shared.ts index 0afae0d5e6..bb4f211799 100644 --- a/packages/core/src/render3/instructions/shared.ts +++ b/packages/core/src/render3/instructions/shared.ts @@ -979,7 +979,7 @@ export function elementPropertyInternal( if (ngDevMode) { validateAgainstEventProperties(propName); - if (!validateProperty(tView, lView, element, propName, tNode)) { + if (!validateProperty(tView, element, propName, tNode)) { // Return here since we only log warnings for unknown properties. logUnknownPropertyError(propName, tNode); return; @@ -996,10 +996,10 @@ export function elementPropertyInternal( (element as RElement).setProperty ? (element as any).setProperty(propName, value) : (element as any)[propName] = value; } - } else if (tNode.type === TNodeType.Container) { + } else if (tNode.type === TNodeType.Container || tNode.type === TNodeType.ElementContainer) { // If the node is a container and the property didn't // match any of the inputs or schemas we should throw. - if (ngDevMode && !matchingSchemas(tView, lView, tNode.tagName)) { + if (ngDevMode && !matchingSchemas(tView, tNode.tagName)) { logUnknownPropertyError(propName, tNode); } } @@ -1057,8 +1057,7 @@ export function setNgReflectProperties( } function validateProperty( - tView: TView, lView: LView, element: RElement|RComment, propName: string, - tNode: TNode): boolean { + tView: TView, element: RElement|RComment, propName: string, tNode: TNode): boolean { // If `schemas` is set to `null`, that's an indication that this Component was compiled in AOT // mode where this check happens at compile time. In JIT mode, `schemas` is always present and // defined as an array (as an empty array in case `schemas` field is not defined) and we should @@ -1067,8 +1066,7 @@ function validateProperty( // The property is considered valid if the element matches the schema, it exists on the element // or it is synthetic, and we are in a browser context (web worker nodes should be skipped). - if (matchingSchemas(tView, lView, tNode.tagName) || propName in element || - isAnimationProp(propName)) { + if (matchingSchemas(tView, tNode.tagName) || propName in element || isAnimationProp(propName)) { return true; } @@ -1077,7 +1075,7 @@ function validateProperty( return typeof Node === 'undefined' || Node === null || !(element instanceof Node); } -export function matchingSchemas(tView: TView, lView: LView, tagName: string|null): boolean { +export function matchingSchemas(tView: TView, tagName: string|null): boolean { const schemas = tView.schemas; if (schemas !== null) { diff --git a/packages/core/test/acceptance/ng_module_spec.ts b/packages/core/test/acceptance/ng_module_spec.ts index 4adbcbcfcb..c65218fd19 100644 --- a/packages/core/test/acceptance/ng_module_spec.ts +++ b/packages/core/test/acceptance/ng_module_spec.ts @@ -287,6 +287,70 @@ describe('NgModule', () => { }).toThrowError(/'custom' is not a known element/); }); + onlyInIvy('unknown element check logs an error message rather than throwing') + .it('should report unknown property bindings on ng-content', () => { + @Component({template: ``}) + class App { + } + + TestBed.configureTestingModule({declarations: [App]}); + const spy = spyOn(console, 'error'); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(spy.calls.mostRecent()?.args[0]) + .toMatch( + /Can't bind to 'unknownProp' since it isn't a known property of 'ng-content'/); + }); + + modifiedInIvy('unknown element error thrown instead of warning') + .it('should throw for unknown property bindings on ng-content', () => { + @Component({template: ``}) + class App { + } + + TestBed.configureTestingModule({declarations: [App]}); + + expect(() => { + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + }) + .toThrowError( + /Can't bind to 'unknownProp' since it isn't a known property of 'ng-content'/); + }); + + onlyInIvy('unknown element check logs an error message rather than throwing') + .it('should report unknown property bindings on ng-container', () => { + @Component({template: ``}) + class App { + } + + TestBed.configureTestingModule({declarations: [App]}); + const spy = spyOn(console, 'error'); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(spy.calls.mostRecent()?.args[0]) + .toMatch( + /Can't bind to 'unknown-prop' since it isn't a known property of 'ng-container'/); + }); + + modifiedInIvy('unknown element error thrown instead of warning') + .it('should throw for unknown property bindings on ng-container', () => { + @Component({template: ``}) + class App { + } + + TestBed.configureTestingModule({declarations: [App]}); + + expect(() => { + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + }) + .toThrowError( + /Can't bind to 'unknown-prop' since it isn't a known property of 'ng-container'/); + }); + onlyInIvy('test relies on Ivy-specific AOT format').describe('AOT-compiled components', () => { function createComponent( template: (rf: any) => void, vars: number, consts?: (number|string)[][]) {