Skip to content

Commit 58b91e5

Browse files
feat(compiler): detect dangling property bindings
BREAKING CHANGE: compiler will throw on binding to non-existing properties. Till now it was possible to have a binding to a non-existing property, ex.: `<div [foo]="exp">`. From now on this is compilation error - any property binding needs to have at least one associated property: eaither on an HTML element or on any directive associated with a given element (directives' properites need to be declared using the `properties` field in the `@Directive` / `@Component` annotation).
1 parent dcc4bc2 commit 58b91e5

File tree

11 files changed

+64
-39
lines changed

11 files changed

+64
-39
lines changed

Diff for: modules/angular2/src/facade/collection.dart

+1
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,7 @@ void iterateListLike(iter, fn(item)) {
205205
}
206206

207207
class SetWrapper {
208+
static Set create() => new Set();
208209
static Set createFromList(List l) => new Set.from(l);
209210
static bool has(Set s, key) => s.contains(key);
210211
}

Diff for: modules/angular2/src/facade/collection.ts

+1
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,7 @@ var createSetFromList: {(lst: List<any>): Set<any>} = (function() {
281281
}
282282
})();
283283
export class SetWrapper {
284+
static create<T>(): Set<T> { return new Set(); }
284285
static createFromList<T>(lst: List<T>): Set<T> { return createSetFromList(lst); }
285286
static has<T>(s: Set<T>, key: T): boolean { return s.has(key); }
286287
}

Diff for: modules/angular2/src/render/dom/compiler/directive_parser.ts

+1
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ export class DirectiveParser implements CompileStep {
151151
var fullExpAstWithBindPipes = this._parser.addPipes(bindingAst, pipes);
152152
directiveBinderBuilder.bindProperty(dirProperty, fullExpAstWithBindPipes);
153153
}
154+
compileElement.bindElement().bindPropertyToDirective(dashCaseToCamelCase(elProp));
154155
}
155156

156157
_bindDirectiveEvent(eventName, action, compileElement, directiveBinderBuilder) {

Diff for: modules/angular2/src/render/dom/view/property_setter_factory.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ const CLASS_PREFIX = 'class.';
1818
const STYLE_PREFIX = 'style.';
1919

2020
export class PropertySetterFactory {
21-
private static _noopSetter(el, value) {}
21+
static noopSetter(el, value) {}
2222

2323
private _lazyPropertySettersCache: StringMap<string, Function> = StringMapWrapper.create();
2424
private _eagerPropertySettersCache: StringMap<string, Function> = StringMapWrapper.create();
@@ -69,7 +69,7 @@ export class PropertySetterFactory {
6969
if (DOM.hasProperty(protoElement, property)) {
7070
setterFn = reflector.setter(property);
7171
} else {
72-
setterFn = PropertySetterFactory._noopSetter;
72+
setterFn = PropertySetterFactory.noopSetter;
7373
}
7474
StringMapWrapper.set(this._eagerPropertySettersCache, property, setterFn);
7575
}

Diff for: modules/angular2/src/render/dom/view/proto_view_builder.ts

+21-4
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import {isPresent, isBlank, BaseException} from 'angular2/src/facade/lang';
1+
import {StringWrapper, isPresent, isBlank, BaseException} from 'angular2/src/facade/lang';
22
import {ListWrapper, MapWrapper, Set, SetWrapper, List} from 'angular2/src/facade/collection';
33
import {DOM} from 'angular2/src/dom/dom_adapter';
44

@@ -74,11 +74,21 @@ export class ProtoViewBuilder {
7474
});
7575

7676
MapWrapper.forEach(ebb.propertyBindings, (_, propertyName) => {
77-
MapWrapper.set(
78-
propertySetters, propertyName,
79-
setterFactory.createSetter(ebb.element, isPresent(ebb.componentId), propertyName));
77+
var propSetter =
78+
setterFactory.createSetter(ebb.element, isPresent(ebb.componentId), propertyName);
79+
80+
if (propSetter === PropertySetterFactory.noopSetter) {
81+
if (!SetWrapper.has(ebb.propertyBindingsToDirectives, propertyName)) {
82+
throw new BaseException(
83+
`Can't bind to '${propertyName}' since it isn't a know property of the '${StringWrapper.toLowerCase(DOM.nodeName(ebb.element))}' element`);
84+
}
85+
}
86+
87+
MapWrapper.set(propertySetters, propertyName, propSetter);
8088
});
8189

90+
91+
8292
var nestedProtoView =
8393
isPresent(ebb.nestedProtoView) ? ebb.nestedProtoView.build(setterFactory) : null;
8494
var nestedRenderProtoView =
@@ -151,6 +161,7 @@ export class ElementBinderBuilder {
151161
directives: List<DirectiveBuilder> = [];
152162
nestedProtoView: ProtoViewBuilder = null;
153163
propertyBindings: Map<string, ASTWithSource> = MapWrapper.create();
164+
propertyBindingsToDirectives: Set<string> = SetWrapper.create();
154165
variableBindings: Map<string, string> = MapWrapper.create();
155166
eventBindings: List<api.EventBinding> = [];
156167
eventBuilder: EventBuilder = new EventBuilder();
@@ -192,6 +203,12 @@ export class ElementBinderBuilder {
192203

193204
bindProperty(name, expression) { MapWrapper.set(this.propertyBindings, name, expression); }
194205

206+
bindPropertyToDirective(name: string) {
207+
// we are filling in a set of property names that are bound to a property
208+
// of at least one directive. This allows us to report "dangling" bindings.
209+
this.propertyBindingsToDirectives.add(name);
210+
}
211+
195212
bindVariable(name, value) {
196213
// When current is a view root, the variable bindings are set to the *nested* proto view.
197214
// The root view conceptually signifies a new "block scope" (the nested view), to which

Diff for: modules/angular2/test/core/compiler/integration_spec.ts

+31-26
Original file line numberDiff line numberDiff line change
@@ -191,22 +191,6 @@ export function main() {
191191
});
192192
}));
193193

194-
it('should ignore bindings to unknown properties',
195-
inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
196-
tb.overrideView(MyComp,
197-
new viewAnn.View({template: '<div unknown="{{ctxProp}}"></div>'}));
198-
199-
tb.createView(MyComp, {context: ctx})
200-
.then((view) => {
201-
202-
ctx.ctxProp = 'Some value';
203-
view.detectChanges();
204-
expect(DOM.hasProperty(view.rootNodes[0], 'unknown')).toBeFalsy();
205-
206-
async.done();
207-
});
208-
}));
209-
210194
it('should consume directive watch expression change.',
211195
inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
212196
var tpl = '<div>' +
@@ -444,7 +428,7 @@ export function main() {
444428
it('should assign a directive to a var-',
445429
inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
446430
tb.overrideView(MyComp, new viewAnn.View({
447-
template: '<p><div [export-dir] #localdir="dir"></div></p>',
431+
template: '<p><div export-dir #localdir="dir"></div></p>',
448432
directives: [ExportDir]
449433
}));
450434

@@ -1146,7 +1130,7 @@ export function main() {
11461130
it('should specify a location of an error that happened during change detection (element property)',
11471131
inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
11481132

1149-
tb.overrideView(MyComp, new viewAnn.View({template: '<div [prop]="a.b"></div>'}));
1133+
tb.overrideView(MyComp, new viewAnn.View({template: '<div [title]="a.b"></div>'}));
11501134

11511135
tb.createView(MyComp, {context: ctx})
11521136
.then((view) => {
@@ -1159,10 +1143,10 @@ export function main() {
11591143
it('should specify a location of an error that happened during change detection (directive property)',
11601144
inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
11611145

1162-
tb.overrideView(
1163-
MyComp,
1164-
new viewAnn.View(
1165-
{template: '<child-cmp [prop]="a.b"></child-cmp>', directives: [ChildComp]}));
1146+
tb.overrideView(MyComp, new viewAnn.View({
1147+
template: '<child-cmp [dir-prop]="a.b"></child-cmp>',
1148+
directives: [ChildComp]
1149+
}));
11661150

11671151
tb.createView(MyComp, {context: ctx})
11681152
.then((view) => {
@@ -1207,6 +1191,30 @@ export function main() {
12071191
});
12081192
}));
12091193

1194+
describe('Missing property bindings', () => {
1195+
it('should throw on bindings to unknown properties',
1196+
inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
1197+
tb.overrideView(MyComp,
1198+
new viewAnn.View({template: '<div unknown="{{ctxProp}}"></div>'}));
1199+
1200+
PromiseWrapper.catchError(tb.createView(MyComp, {context: ctx}), (e) => {
1201+
expect(e.message).toEqual(
1202+
`Can't bind to 'unknown' since it isn't a know property of the 'div' element`);
1203+
async.done();
1204+
return null;
1205+
});
1206+
}));
1207+
1208+
it('should not throw for property binding to a non-existing property when there is a matching directive property',
1209+
inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
1210+
tb.overrideView(
1211+
MyComp,
1212+
new viewAnn.View(
1213+
{template: '<div my-dir [elprop]="ctxProp"></div>', directives: [MyDir]}));
1214+
tb.createView(MyComp, {context: ctx}).then((val) => { async.done(); });
1215+
}));
1216+
});
1217+
12101218
// Disabled until a solution is found, refs:
12111219
// - https://github.com/angular/angular/issues/776
12121220
// - https://github.com/angular/angular/commit/81f3f32
@@ -1356,10 +1364,7 @@ class ComponentWithPipes {
13561364
prop: string;
13571365
}
13581366

1359-
@Component({
1360-
selector: 'child-cmp',
1361-
appInjector: [MyService],
1362-
})
1367+
@Component({selector: 'child-cmp', properties: ['dirProp'], appInjector: [MyService]})
13631368
@View({directives: [MyDir], template: '{{ctxProp}}'})
13641369
@Injectable()
13651370
class ChildComp {

Diff for: modules/angular2/test/core/directive_lifecycle_integration_spec.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ export function main() {
3636
tb.overrideView(
3737
MyComp,
3838
new viewAnn.View(
39-
{template: '<div [field]="123" [lifecycle]></div>', directives: [LifecycleDir]}));
39+
{template: '<div [field]="123" lifecycle></div>', directives: [LifecycleDir]}));
4040

4141
tb.createView(MyComp, {context: ctx})
4242
.then((view) => {

Diff for: modules/angular2/test/directives/ng_for_spec.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ export function main() {
205205

206206
it('should repeat over nested arrays with no intermediate element',
207207
inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
208-
var template = '<div><template [ng-for] #item [ng-for-of]="items">' +
208+
var template = '<div><template ng-for #item [ng-for-of]="items">' +
209209
'<div template="ng-for #subitem of item">' +
210210
'{{subitem}}-{{item.length}};' +
211211
'</div></template></div>';

Diff for: modules/angular2/test/directives/ng_switch_spec.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,8 @@ export function main() {
7979
'<template [ng-switch-when]="\'b\'"><li>when b1;</li></template>' +
8080
'<template [ng-switch-when]="\'a\'"><li>when a2;</li></template>' +
8181
'<template [ng-switch-when]="\'b\'"><li>when b2;</li></template>' +
82-
'<template [ng-switch-default]><li>when default1;</li></template>' +
83-
'<template [ng-switch-default]><li>when default2;</li></template>' +
82+
'<template ng-switch-default><li>when default1;</li></template>' +
83+
'<template ng-switch-default><li>when default2;</li></template>' +
8484
'</ul></div>';
8585

8686
tb.createView(TestComponent, {html: template})
@@ -108,7 +108,7 @@ export function main() {
108108
'<ul [ng-switch]="switchValue">' +
109109
'<template [ng-switch-when]="when1"><li>when 1;</li></template>' +
110110
'<template [ng-switch-when]="when2"><li>when 2;</li></template>' +
111-
'<template [ng-switch-default]><li>when default;</li></template>' +
111+
'<template ng-switch-default><li>when default;</li></template>' +
112112
'</ul></div>';
113113

114114
tb.createView(TestComponent, {html: template})

Diff for: modules/angular2/test/render/dom/shadow_dom_emulation_integration_spec.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -460,7 +460,7 @@ var conditionalContentComponent = DirectiveMetadata.create({
460460
});
461461

462462
var autoViewportDirective = DirectiveMetadata.create(
463-
{selector: '[auto]', id: '[auto]', type: DirectiveMetadata.DIRECTIVE_TYPE});
463+
{selector: '[auto]', id: 'auto', properties: ['auto'], type: DirectiveMetadata.DIRECTIVE_TYPE});
464464

465465
var tabComponent =
466466
DirectiveMetadata.create({selector: 'tab', id: 'tab', type: DirectiveMetadata.COMPONENT_TYPE});

Diff for: modules/benchmarks/src/largetable/largetable_benchmark.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ class LargetableComponent {
269269
@Component({selector: 'app'})
270270
@View({
271271
directives: [LargetableComponent],
272-
template: `<largetable [data]='data' [benchmarkType]='benchmarkType'></largetable>`
272+
template: `<largetable [data]='data' [benchmark-type]='benchmarkType'></largetable>`
273273
})
274274
class AppComponent {
275275
data;

0 commit comments

Comments
 (0)