Skip to content

Commit

Permalink
fix(ivy): warn instead of throwing for unknown properties (angular#32463
Browse files Browse the repository at this point in the history 
)

Logs a warning instead of throwing when running into a binding to an unknown property in JIT mode. Since we aren't using a schema for the runtime validation anymore, this allows us to support browsers where properties are unsupported.

PR Close angular#32463
  • Loading branch information
@crisbeto @arnehoek
crisbeto authored and arnehoek committed Sep 26, 2019
1 parent fc9d1a0 commit 4c5774f
Show file tree
Hide file tree
Showing 6 changed files with 125 additions and 103 deletions.
48 changes: 21 additions & 27 deletions packages/core/src/render3/instructions/shared.ts
View file
Expand Up @@ -883,14 +883,17 @@ export function elementPropertyInternal<T>(

if (ngDevMode) {
validateAgainstEventProperties(propName);
validateAgainstUnknownProperties(lView, element, propName, tNode);
if (!validateProperty(lView, element, propName, tNode)) {
// Return here since we only log warnings for unknown properties.
warnAboutUnknownProperty(propName, tNode);
return;
}
ngDevMode.rendererSetProperty++;
}

const renderer = loadRendererFn ? loadRendererFn(tNode, lView) : lView[RENDERER];
// It is assumed that the sanitizer is only added when the compiler determines that the
// property
// is risky, so sanitization can be done without further checks.
// property is risky, so sanitization can be done without further checks.
value = sanitizer != null ? (sanitizer(value, tNode.tagName || '', propName) as any) : value;
if (isProceduralRenderer(renderer)) {
renderer.setProperty(element as RElement, propName, value);
Expand All @@ -902,7 +905,7 @@ export function elementPropertyInternal<T>(
// If the node is a container and the property didn't
// match any of the inputs or schemas we should throw.
if (ngDevMode && !matchingSchemas(lView, tNode.tagName)) {
throw createUnknownPropertyError(propName, tNode);
warnAboutUnknownProperty(propName, tNode);
}
}
}
Expand Down Expand Up @@ -940,22 +943,13 @@ export function setNgReflectProperty(
}
}

function validateAgainstUnknownProperties(
hostView: LView, element: RElement | RComment, propName: string, tNode: TNode) {
// If the tag matches any of the schemas we shouldn't throw.
if (matchingSchemas(hostView, tNode.tagName)) {
return;
}

// If prop is not a known property of the HTML element...
if (!(propName in element) &&
// and we are in a browser context... (web worker nodes should be skipped)
typeof Node === 'function' && element instanceof Node &&
// and isn't a synthetic animation property...
propName[0] !== ANIMATION_PROP_PREFIX) {
// ... it is probably a user error and we should throw.
throw createUnknownPropertyError(propName, tNode);
}
function validateProperty(
hostView: LView, element: RElement | RComment, propName: string, tNode: TNode): boolean {
// 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).
return matchingSchemas(hostView, tNode.tagName) || propName in element ||
propName[0] === ANIMATION_PROP_PREFIX || typeof Node !== 'function' ||
!(element instanceof Node);
}

function matchingSchemas(hostView: LView, tagName: string | null): boolean {
Expand All @@ -975,13 +969,13 @@ function matchingSchemas(hostView: LView, tagName: string | null): boolean {
}

/**
* Creates an error that should be thrown when encountering an unknown property on an element.
* @param propName Name of the invalid property.
* @param tNode Node on which we encountered the error.
*/
function createUnknownPropertyError(propName: string, tNode: TNode): Error {
return new Error(
`Template error: Can't bind to '${propName}' since it isn't a known property of '${tNode.tagName}'.`);
* Logs a warning that a property is not supported on an element.
* @param propName Name of the invalid property.
* @param tNode Node on which we encountered the property.
*/
function warnAboutUnknownProperty(propName: string, tNode: TNode): void {
console.warn(
`Can't bind to '${propName}' since it isn't a known property of '${tNode.tagName}'.`);
}

/**
Expand Down
86 changes: 59 additions & 27 deletions packages/core/test/acceptance/ng_module_spec.ts
View file
Expand Up @@ -9,6 +9,7 @@
import {CommonModule} from '@angular/common';
import {Component, NO_ERRORS_SCHEMA, NgModule} from '@angular/core';
import {TestBed} from '@angular/core/testing';
import {modifiedInIvy, onlyInIvy} from '@angular/private/testing';

describe('NgModule', () => {
@Component({template: 'hello'})
Expand Down Expand Up @@ -76,33 +77,64 @@ describe('NgModule', () => {
});

describe('schemas', () => {
it('should throw on unknown props if NO_ERRORS_SCHEMA is absent', () => {
@Component({
selector: 'my-comp',
template: `
<ng-container *ngIf="condition">
<div [unknown-prop]="true"></div>
</ng-container>
`,
})
class MyComp {
condition = true;
}

@NgModule({
imports: [CommonModule],
declarations: [MyComp],
})
class MyModule {
}

TestBed.configureTestingModule({imports: [MyModule]});

expect(() => {
const fixture = TestBed.createComponent(MyComp);
fixture.detectChanges();
}).toThrowError(/Can't bind to 'unknown-prop' since it isn't a known property of 'div'/);
});
onlyInIvy('Unknown property warning logged instead of throwing an error')
.it('should throw on unknown props if NO_ERRORS_SCHEMA is absent', () => {
@Component({
selector: 'my-comp',
template: `
<ng-container *ngIf="condition">
<div [unknown-prop]="true"></div>
</ng-container>
`,
})
class MyComp {
condition = true;
}

@NgModule({
imports: [CommonModule],
declarations: [MyComp],
})
class MyModule {
}

TestBed.configureTestingModule({imports: [MyModule]});

const spy = spyOn(console, 'warn');
const fixture = TestBed.createComponent(MyComp);
fixture.detectChanges();
expect(spy.calls.mostRecent().args[0])
.toMatch(/Can't bind to 'unknown-prop' since it isn't a known property of 'div'/);
});

modifiedInIvy('Unknown properties throw an error instead of logging a warning')
.it('should throw on unknown props if NO_ERRORS_SCHEMA is absent', () => {
@Component({
selector: 'my-comp',
template: `
<ng-container *ngIf="condition">
<div [unknown-prop]="true"></div>
</ng-container>
`,
})
class MyComp {
condition = true;
}

@NgModule({
imports: [CommonModule],
declarations: [MyComp],
})
class MyModule {
}

TestBed.configureTestingModule({imports: [MyModule]});

expect(() => {
const fixture = TestBed.createComponent(MyComp);
fixture.detectChanges();
}).toThrowError(/Can't bind to 'unknown-prop' since it isn't a known property of 'div'/);
});

it('should not throw on unknown props if NO_ERRORS_SCHEMA is present', () => {
@Component({
Expand Down
45 changes: 28 additions & 17 deletions packages/core/test/linker/integration_spec.ts
View file
Expand Up @@ -1630,7 +1630,7 @@ function declareTests(config?: {useJit: boolean}) {
});

describe('Property bindings', () => {
modifiedInIvy('Unknown property error thrown during update mode, not creation mode')
modifiedInIvy('Unknown property error throws an error instead of logging a warning')
.it('should throw on bindings to unknown properties', () => {
TestBed.configureTestingModule({declarations: [MyComp]});
const template = '<div unknown="{{ctxProp}}"></div>';
Expand All @@ -1644,35 +1644,46 @@ function declareTests(config?: {useJit: boolean}) {
}
});

onlyInIvy('Unknown property error thrown during update mode, not creation mode')
onlyInIvy('Unknown property warning logged instead of throwing an error')
.it('should throw on bindings to unknown properties', () => {
TestBed.configureTestingModule({declarations: [MyComp]});
const template = '<div unknown="{{ctxProp}}"></div>';
TestBed.overrideComponent(MyComp, {set: {template}});

const spy = spyOn(console, 'warn');
const fixture = TestBed.createComponent(MyComp);
fixture.detectChanges();
expect(spy.calls.mostRecent().args[0])
.toMatch(/Can't bind to 'unknown' since it isn't a known property of 'div'./);
});

modifiedInIvy('Unknown property error thrown instead of a warning')
.it('should throw on bindings to unknown properties', () => {
TestBed.configureTestingModule({imports: [CommonModule], declarations: [MyComp]});
const template = '<div *ngFor="let item in ctxArrProp">{{item}}</div>';
TestBed.overrideComponent(MyComp, {set: {template}});

try {
const fixture = TestBed.createComponent(MyComp);
fixture.detectChanges();
throw 'Should throw';
} catch (e) {
expect(e.message).toMatch(
/Template error: Can't bind to 'unknown' since it isn't a known property of 'div'./);
/Can't bind to 'ngForIn' since it isn't a known property of 'div'./);
}
});

it('should throw on bindings to unknown properties of containers', () => {
TestBed.configureTestingModule({imports: [CommonModule], declarations: [MyComp]});
const template = '<div *ngFor="let item in ctxArrProp">{{item}}</div>';
TestBed.overrideComponent(MyComp, {set: {template}});

try {
const fixture = TestBed.createComponent(MyComp);
fixture.detectChanges();
throw 'Should throw';
} catch (e) {
expect(e.message).toMatch(
/Can't bind to 'ngForIn' since it isn't a known property of 'div'./);
}
});
onlyInIvy('Unknown property logs a warning instead of throwing an error')
.it('should throw on bindings to unknown properties', () => {
TestBed.configureTestingModule({imports: [CommonModule], declarations: [MyComp]});
const template = '<div *ngFor="let item in ctxArrProp">{{item}}</div>';
TestBed.overrideComponent(MyComp, {set: {template}});
const spy = spyOn(console, 'warn');
const fixture = TestBed.createComponent(MyComp);
fixture.detectChanges();
expect(spy.calls.mostRecent().args[0])
.toMatch(/Can't bind to 'ngForIn' since it isn't a known property of 'div'./);
});

it('should not throw for property binding to a non-existing property when there is a matching directive property',
() => {
Expand Down
19 changes: 5 additions & 14 deletions packages/core/test/linker/ng_module_integration_spec.ts
View file
Expand Up @@ -92,13 +92,6 @@ class SomePipe {
class CompUsingModuleDirectiveAndPipe {
}

class DummyConsole implements Console {
public warnings: string[] = [];

log(message: string) {}
warn(message: string) { this.warnings.push(message); }
}

{
if (ivyEnabled) {
describe('ivy', () => { declareTests(); });
Expand All @@ -112,12 +105,8 @@ function declareTests(config?: {useJit: boolean}) {
describe('NgModule', () => {
let compiler: Compiler;
let injector: Injector;
let console: DummyConsole;

beforeEach(() => {
console = new DummyConsole();
TestBed.configureCompiler({...config, providers: [{provide: Console, useValue: console}]});
});
beforeEach(() => { TestBed.configureCompiler(config || {}); });

beforeEach(inject([Compiler, Injector], (_compiler: Compiler, _injector: Injector) => {
compiler = _compiler;
Expand Down Expand Up @@ -261,7 +250,7 @@ function declareTests(config?: {useJit: boolean}) {
expect(() => createModule(SomeModule)).toThrowError(/Can't bind to 'someUnknownProp'/);
});

onlyInIvy('Unknown property error thrown during update mode, not creation mode')
onlyInIvy('Unknown property warning logged, instead of throwing an error')
.it('should error on unknown bound properties on custom elements by default', () => {
@Component({template: '<some-element [someUnknownProp]="true"></some-element>'})
class ComponentUsingInvalidProperty {
Expand All @@ -271,8 +260,10 @@ function declareTests(config?: {useJit: boolean}) {
class SomeModule {
}

const spy = spyOn(console, 'warn');
const fixture = createComp(ComponentUsingInvalidProperty, SomeModule);
expect(() => fixture.detectChanges()).toThrowError(/Can't bind to 'someUnknownProp'/);
fixture.detectChanges();
expect(spy.calls.mostRecent().args[0]).toMatch(/Can't bind to 'someUnknownProp'/);
});

it('should not error on unknown bound properties on custom elements when using the CUSTOM_ELEMENTS_SCHEMA',
Expand Down
6 changes: 4 additions & 2 deletions packages/core/test/linker/security_integration_spec.ts
View file
Expand Up @@ -263,13 +263,15 @@ function declareTests(config?: {useJit: boolean}) {
.toThrowError(/Can't bind to 'xlink:href'/);
});

onlyInIvy('Unknown property error thrown during update mode, not creation mode')
onlyInIvy('Unknown property warning logged instead of throwing an error')
.it('should escape unsafe SVG attributes', () => {
const template = `<svg:circle [xlink:href]="ctxProp">Text</svg:circle>`;
TestBed.overrideComponent(SecuredComponent, {set: {template}});

const spy = spyOn(console, 'warn');
const fixture = TestBed.createComponent(SecuredComponent);
expect(() => fixture.detectChanges()).toThrowError(/Can't bind to 'xlink:href'/);
fixture.detectChanges();
expect(spy.calls.mostRecent().args[0]).toMatch(/Can't bind to 'xlink:href'/);
});

it('should escape unsafe HTML values', () => {
Expand Down
24 changes: 8 additions & 16 deletions packages/platform-browser/test/testing_public_spec.ts
View file
Expand Up @@ -922,7 +922,7 @@ Did you run and wait for 'resolveComponentResources()'?` :
});


modifiedInIvy(`Unknown property error thrown during update mode, not creation mode`)
modifiedInIvy(`Unknown property error thrown instead of logging a warning`)
.it('should error on unknown bound properties on custom elements by default', () => {
@Component({template: '<some-element [someUnknownProp]="true"></some-element>'})
class ComponentUsingInvalidProperty {
Expand All @@ -941,26 +941,18 @@ Did you run and wait for 'resolveComponentResources()'?` :
restoreJasmineIt();
});

onlyInIvy(`Unknown property error thrown during update mode, not creation mode`)
onlyInIvy(`Unknown property warning logged instead of an error`)
.it('should error on unknown bound properties on custom elements by default', () => {
@Component({template: '<some-element [someUnknownProp]="true"></some-element>'})
class ComponentUsingInvalidProperty {
}

const itPromise = patchJasmineIt();

expect(
() => it(
'should fail', withModule(
{declarations: [ComponentUsingInvalidProperty]},
() => {
const fixture =
TestBed.createComponent(ComponentUsingInvalidProperty);
fixture.detectChanges();
})))
.toThrowError(/Can't bind to 'someUnknownProp'/);

restoreJasmineIt();
const spy = spyOn(console, 'warn');
withModule({declarations: [ComponentUsingInvalidProperty]}, () => {
const fixture = TestBed.createComponent(ComponentUsingInvalidProperty);
fixture.detectChanges();
})();
expect(spy.calls.mostRecent().args[0]).toMatch(/Can't bind to 'someUnknownProp'/);
});
});

Expand Down

0 comments on commit 4c5774f

Please sign in to comment.