Skip to content

Commit 17ddab9

Browse files
crisbetoAndrewKushnir
authored andcommitted
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
1 parent 4f65f47 commit 17ddab9

File tree

6 files changed

+116
-15
lines changed

6 files changed

+116
-15
lines changed

packages/compiler-cli/test/compliance/r3_compiler_compliance_spec.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1551,6 +1551,43 @@ describe('compiler compliance', () => {
15511551
const result = compile(files, angularFiles);
15521552
expectEmit(result.source, SimpleComponentDefinition, 'Incorrect MyApp definition');
15531553
});
1554+
1555+
it('should capture the node name of ng-content with a structural directive', () => {
1556+
const files = {
1557+
app: {
1558+
'spec.ts': `
1559+
import {Component, Directive, NgModule, TemplateRef} from '@angular/core';
1560+
1561+
@Component({selector: 'simple', template: '<ng-content *ngIf="showContent"></ng-content>'})
1562+
export class SimpleComponent {}
1563+
`
1564+
}
1565+
};
1566+
1567+
const SimpleComponentDefinition = `
1568+
SimpleComponent.ɵcmp = $r3$.ɵɵdefineComponent({
1569+
type: SimpleComponent,
1570+
selectors: [["simple"]],
1571+
ngContentSelectors: $c0$,
1572+
decls: 1,
1573+
vars: 1,
1574+
consts: [[4, "ngIf"]],
1575+
template: function SimpleComponent_Template(rf, ctx) {
1576+
if (rf & 1) {
1577+
i0.ɵɵprojectionDef();
1578+
i0.ɵɵtemplate(0, SimpleComponent_ng_content_0_Template, 1, 0, "ng-content", 0);
1579+
}
1580+
if (rf & 2) {
1581+
i0.ɵɵproperty("ngIf", ctx.showContent);
1582+
}
1583+
},
1584+
encapsulation: 2
1585+
});`;
1586+
1587+
const result = compile(files, angularFiles);
1588+
expectEmit(
1589+
result.source, SimpleComponentDefinition, 'Incorrect SimpleComponent definition');
1590+
});
15541591
});
15551592

15561593
describe('queries', () => {

packages/compiler/src/render3/r3_ast.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,8 @@ export class Template implements Node {
104104
}
105105

106106
export class Content implements Node {
107+
readonly name = 'ng-content';
108+
107109
constructor(
108110
public selector: string, public attributes: TextAttribute[],
109111
public sourceSpan: ParseSourceSpan, public i18n?: I18nMeta) {}

packages/compiler/src/render3/r3_template_transform.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -233,10 +233,10 @@ class HtmlAstToIvyAst implements html.Visitor {
233233

234234
// TODO(pk): test for this case
235235
parsedElement = new t.Template(
236-
(parsedElement as t.Element).name, hoistedAttrs.attributes, hoistedAttrs.inputs,
237-
hoistedAttrs.outputs, templateAttrs, [parsedElement], [/* no references */],
238-
templateVariables, element.sourceSpan, element.startSourceSpan, element.endSourceSpan,
239-
i18n);
236+
(parsedElement as t.Element | t.Content).name, hoistedAttrs.attributes,
237+
hoistedAttrs.inputs, hoistedAttrs.outputs, templateAttrs, [parsedElement],
238+
[/* no references */], templateVariables, element.sourceSpan, element.startSourceSpan,
239+
element.endSourceSpan, i18n);
240240
}
241241
if (isI18nRootElement) {
242242
this.inI18nBlock = false;

packages/core/src/render3/instructions/element.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ function elementStartFirstCreatePass(
3737

3838
const hasDirectives =
3939
resolveDirectives(tView, lView, tNode, getConstant<string[]>(tViewConsts, localRefsIndex));
40-
ngDevMode && logUnknownElementError(tView, lView, native, tNode, hasDirectives);
40+
ngDevMode && logUnknownElementError(tView, native, tNode, hasDirectives);
4141

4242
if (tNode.attrs !== null) {
4343
computeStaticStyling(tNode, tNode.attrs, false);
@@ -178,7 +178,7 @@ export function ɵɵelement(
178178
}
179179

180180
function logUnknownElementError(
181-
tView: TView, lView: LView, element: RElement, tNode: TNode, hasDirectives: boolean): void {
181+
tView: TView, element: RElement, tNode: TNode, hasDirectives: boolean): void {
182182
const schemas = tView.schemas;
183183

184184
// If `schemas` is set to `null`, that's an indication that this Component was compiled in AOT
@@ -202,7 +202,7 @@ function logUnknownElementError(
202202
(typeof customElements !== 'undefined' && tagName.indexOf('-') > -1 &&
203203
!customElements.get(tagName));
204204

205-
if (isUnknown && !matchingSchemas(tView, lView, tagName)) {
205+
if (isUnknown && !matchingSchemas(tView, tagName)) {
206206
let message = `'${tagName}' is not a known element:\n`;
207207
message += `1. If '${
208208
tagName}' is an Angular component, then verify that it is part of this module.\n`;

packages/core/src/render3/instructions/shared.ts

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -979,7 +979,7 @@ export function elementPropertyInternal<T>(
979979

980980
if (ngDevMode) {
981981
validateAgainstEventProperties(propName);
982-
if (!validateProperty(tView, lView, element, propName, tNode)) {
982+
if (!validateProperty(tView, element, propName, tNode)) {
983983
// Return here since we only log warnings for unknown properties.
984984
logUnknownPropertyError(propName, tNode);
985985
return;
@@ -996,10 +996,10 @@ export function elementPropertyInternal<T>(
996996
(element as RElement).setProperty ? (element as any).setProperty(propName, value) :
997997
(element as any)[propName] = value;
998998
}
999-
} else if (tNode.type === TNodeType.Container) {
999+
} else if (tNode.type === TNodeType.Container || tNode.type === TNodeType.ElementContainer) {
10001000
// If the node is a container and the property didn't
10011001
// match any of the inputs or schemas we should throw.
1002-
if (ngDevMode && !matchingSchemas(tView, lView, tNode.tagName)) {
1002+
if (ngDevMode && !matchingSchemas(tView, tNode.tagName)) {
10031003
logUnknownPropertyError(propName, tNode);
10041004
}
10051005
}
@@ -1057,8 +1057,7 @@ export function setNgReflectProperties(
10571057
}
10581058

10591059
function validateProperty(
1060-
tView: TView, lView: LView, element: RElement|RComment, propName: string,
1061-
tNode: TNode): boolean {
1060+
tView: TView, element: RElement|RComment, propName: string, tNode: TNode): boolean {
10621061
// If `schemas` is set to `null`, that's an indication that this Component was compiled in AOT
10631062
// mode where this check happens at compile time. In JIT mode, `schemas` is always present and
10641063
// defined as an array (as an empty array in case `schemas` field is not defined) and we should
@@ -1067,8 +1066,7 @@ function validateProperty(
10671066

10681067
// The property is considered valid if the element matches the schema, it exists on the element
10691068
// or it is synthetic, and we are in a browser context (web worker nodes should be skipped).
1070-
if (matchingSchemas(tView, lView, tNode.tagName) || propName in element ||
1071-
isAnimationProp(propName)) {
1069+
if (matchingSchemas(tView, tNode.tagName) || propName in element || isAnimationProp(propName)) {
10721070
return true;
10731071
}
10741072

@@ -1077,7 +1075,7 @@ function validateProperty(
10771075
return typeof Node === 'undefined' || Node === null || !(element instanceof Node);
10781076
}
10791077

1080-
export function matchingSchemas(tView: TView, lView: LView, tagName: string|null): boolean {
1078+
export function matchingSchemas(tView: TView, tagName: string|null): boolean {
10811079
const schemas = tView.schemas;
10821080

10831081
if (schemas !== null) {

packages/core/test/acceptance/ng_module_spec.ts

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,70 @@ describe('NgModule', () => {
287287
}).toThrowError(/'custom' is not a known element/);
288288
});
289289

290+
onlyInIvy('unknown element check logs an error message rather than throwing')
291+
.it('should report unknown property bindings on ng-content', () => {
292+
@Component({template: `<ng-content *unknownProp="123"></ng-content>`})
293+
class App {
294+
}
295+
296+
TestBed.configureTestingModule({declarations: [App]});
297+
const spy = spyOn(console, 'error');
298+
const fixture = TestBed.createComponent(App);
299+
fixture.detectChanges();
300+
301+
expect(spy.calls.mostRecent()?.args[0])
302+
.toMatch(
303+
/Can't bind to 'unknownProp' since it isn't a known property of 'ng-content'/);
304+
});
305+
306+
modifiedInIvy('unknown element error thrown instead of warning')
307+
.it('should throw for unknown property bindings on ng-content', () => {
308+
@Component({template: `<ng-content *unknownProp="123"></ng-content>`})
309+
class App {
310+
}
311+
312+
TestBed.configureTestingModule({declarations: [App]});
313+
314+
expect(() => {
315+
const fixture = TestBed.createComponent(App);
316+
fixture.detectChanges();
317+
})
318+
.toThrowError(
319+
/Can't bind to 'unknownProp' since it isn't a known property of 'ng-content'/);
320+
});
321+
322+
onlyInIvy('unknown element check logs an error message rather than throwing')
323+
.it('should report unknown property bindings on ng-container', () => {
324+
@Component({template: `<ng-container [unknown-prop]="123"></ng-container>`})
325+
class App {
326+
}
327+
328+
TestBed.configureTestingModule({declarations: [App]});
329+
const spy = spyOn(console, 'error');
330+
const fixture = TestBed.createComponent(App);
331+
fixture.detectChanges();
332+
333+
expect(spy.calls.mostRecent()?.args[0])
334+
.toMatch(
335+
/Can't bind to 'unknown-prop' since it isn't a known property of 'ng-container'/);
336+
});
337+
338+
modifiedInIvy('unknown element error thrown instead of warning')
339+
.it('should throw for unknown property bindings on ng-container', () => {
340+
@Component({template: `<ng-container [unknown-prop]="123"></ng-container>`})
341+
class App {
342+
}
343+
344+
TestBed.configureTestingModule({declarations: [App]});
345+
346+
expect(() => {
347+
const fixture = TestBed.createComponent(App);
348+
fixture.detectChanges();
349+
})
350+
.toThrowError(
351+
/Can't bind to 'unknown-prop' since it isn't a known property of 'ng-container'/);
352+
});
353+
290354
onlyInIvy('test relies on Ivy-specific AOT format').describe('AOT-compiled components', () => {
291355
function createComponent(
292356
template: (rf: any) => void, vars: number, consts?: (number|string)[][]) {

0 commit comments

Comments
 (0)