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
This commit is contained in:
parent
406f801b70
commit
bf641e1b4b
@ -1551,6 +1551,43 @@ describe('compiler compliance', () => {
|
|||||||
const result = compile(files, angularFiles);
|
const result = compile(files, angularFiles);
|
||||||
expectEmit(result.source, SimpleComponentDefinition, 'Incorrect MyApp definition');
|
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: '<ng-content *ngIf="showContent"></ng-content>'})
|
||||||
|
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', () => {
|
describe('queries', () => {
|
||||||
|
@ -104,6 +104,8 @@ export class Template implements Node {
|
|||||||
}
|
}
|
||||||
|
|
||||||
export class Content implements Node {
|
export class Content implements Node {
|
||||||
|
readonly name = 'ng-content';
|
||||||
|
|
||||||
constructor(
|
constructor(
|
||||||
public selector: string, public attributes: TextAttribute[],
|
public selector: string, public attributes: TextAttribute[],
|
||||||
public sourceSpan: ParseSourceSpan, public i18n?: I18nMeta) {}
|
public sourceSpan: ParseSourceSpan, public i18n?: I18nMeta) {}
|
||||||
|
@ -233,10 +233,10 @@ class HtmlAstToIvyAst implements html.Visitor {
|
|||||||
|
|
||||||
// TODO(pk): test for this case
|
// TODO(pk): test for this case
|
||||||
parsedElement = new t.Template(
|
parsedElement = new t.Template(
|
||||||
(parsedElement as t.Element).name, hoistedAttrs.attributes, hoistedAttrs.inputs,
|
(parsedElement as t.Element | t.Content).name, hoistedAttrs.attributes,
|
||||||
hoistedAttrs.outputs, templateAttrs, [parsedElement], [/* no references */],
|
hoistedAttrs.inputs, hoistedAttrs.outputs, templateAttrs, [parsedElement],
|
||||||
templateVariables, element.sourceSpan, element.startSourceSpan, element.endSourceSpan,
|
[/* no references */], templateVariables, element.sourceSpan, element.startSourceSpan,
|
||||||
i18n);
|
element.endSourceSpan, i18n);
|
||||||
}
|
}
|
||||||
if (isI18nRootElement) {
|
if (isI18nRootElement) {
|
||||||
this.inI18nBlock = false;
|
this.inI18nBlock = false;
|
||||||
|
@ -37,7 +37,7 @@ function elementStartFirstCreatePass(
|
|||||||
|
|
||||||
const hasDirectives =
|
const hasDirectives =
|
||||||
resolveDirectives(tView, lView, tNode, getConstant<string[]>(tViewConsts, localRefsIndex));
|
resolveDirectives(tView, lView, tNode, getConstant<string[]>(tViewConsts, localRefsIndex));
|
||||||
ngDevMode && logUnknownElementError(tView, lView, native, tNode, hasDirectives);
|
ngDevMode && logUnknownElementError(tView, native, tNode, hasDirectives);
|
||||||
|
|
||||||
if (tNode.attrs !== null) {
|
if (tNode.attrs !== null) {
|
||||||
computeStaticStyling(tNode, tNode.attrs, false);
|
computeStaticStyling(tNode, tNode.attrs, false);
|
||||||
@ -178,7 +178,7 @@ export function ɵɵelement(
|
|||||||
}
|
}
|
||||||
|
|
||||||
function logUnknownElementError(
|
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;
|
const schemas = tView.schemas;
|
||||||
|
|
||||||
// If `schemas` is set to `null`, that's an indication that this Component was compiled in AOT
|
// 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 &&
|
(typeof customElements !== 'undefined' && tagName.indexOf('-') > -1 &&
|
||||||
!customElements.get(tagName));
|
!customElements.get(tagName));
|
||||||
|
|
||||||
if (isUnknown && !matchingSchemas(tView, lView, tagName)) {
|
if (isUnknown && !matchingSchemas(tView, tagName)) {
|
||||||
let message = `'${tagName}' is not a known element:\n`;
|
let message = `'${tagName}' is not a known element:\n`;
|
||||||
message += `1. If '${
|
message += `1. If '${
|
||||||
tagName}' is an Angular component, then verify that it is part of this module.\n`;
|
tagName}' is an Angular component, then verify that it is part of this module.\n`;
|
||||||
|
@ -979,7 +979,7 @@ export function elementPropertyInternal<T>(
|
|||||||
|
|
||||||
if (ngDevMode) {
|
if (ngDevMode) {
|
||||||
validateAgainstEventProperties(propName);
|
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.
|
// Return here since we only log warnings for unknown properties.
|
||||||
logUnknownPropertyError(propName, tNode);
|
logUnknownPropertyError(propName, tNode);
|
||||||
return;
|
return;
|
||||||
@ -996,10 +996,10 @@ export function elementPropertyInternal<T>(
|
|||||||
(element as RElement).setProperty ? (element as any).setProperty(propName, value) :
|
(element as RElement).setProperty ? (element as any).setProperty(propName, value) :
|
||||||
(element as any)[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
|
// If the node is a container and the property didn't
|
||||||
// match any of the inputs or schemas we should throw.
|
// 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);
|
logUnknownPropertyError(propName, tNode);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@ -1057,8 +1057,7 @@ export function setNgReflectProperties(
|
|||||||
}
|
}
|
||||||
|
|
||||||
function validateProperty(
|
function validateProperty(
|
||||||
tView: TView, lView: LView, element: RElement|RComment, propName: string,
|
tView: TView, element: RElement|RComment, propName: string, tNode: TNode): boolean {
|
||||||
tNode: TNode): boolean {
|
|
||||||
// If `schemas` is set to `null`, that's an indication that this Component was compiled in AOT
|
// 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
|
// 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
|
// 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
|
// 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).
|
// 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 ||
|
if (matchingSchemas(tView, tNode.tagName) || propName in element || isAnimationProp(propName)) {
|
||||||
isAnimationProp(propName)) {
|
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -1077,7 +1075,7 @@ function validateProperty(
|
|||||||
return typeof Node === 'undefined' || Node === null || !(element instanceof Node);
|
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;
|
const schemas = tView.schemas;
|
||||||
|
|
||||||
if (schemas !== null) {
|
if (schemas !== null) {
|
||||||
|
@ -287,6 +287,70 @@ describe('NgModule', () => {
|
|||||||
}).toThrowError(/'custom' is not a known element/);
|
}).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: `<ng-content *unknownProp="123"></ng-content>`})
|
||||||
|
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: `<ng-content *unknownProp="123"></ng-content>`})
|
||||||
|
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: `<ng-container [unknown-prop]="123"></ng-container>`})
|
||||||
|
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: `<ng-container [unknown-prop]="123"></ng-container>`})
|
||||||
|
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', () => {
|
onlyInIvy('test relies on Ivy-specific AOT format').describe('AOT-compiled components', () => {
|
||||||
function createComponent(
|
function createComponent(
|
||||||
template: (rf: any) => void, vars: number, consts?: (number|string)[][]) {
|
template: (rf: any) => void, vars: number, consts?: (number|string)[][]) {
|
||||||
|
Loading…
x
Reference in New Issue
Block a user