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).
angular · Jun 18, 2015 · 72d3a53 · 72d3a53
1 parent 5beaf6d
commit 72d3a53
Show file tree

Hide file tree

Showing 12 changed files with 66 additions and 41 deletions.
diff --git a/modules/angular2/src/facade/collection.dart b/modules/angular2/src/facade/collection.dart
@@ -201,6 +201,7 @@ void iterateListLike(iter, fn(item)) {
 }
 
 class SetWrapper {
+  static Set create() => new Set();
   static Set createFromList(List l) => new Set.from(l);
   static bool has(Set s, key) => s.contains(key);
 }
diff --git a/modules/angular2/src/facade/collection.ts b/modules/angular2/src/facade/collection.ts
@@ -279,6 +279,7 @@ var createSetFromList: {(lst: List<any>): Set<any>} = (function() {
   }
 })();
 export class SetWrapper {
+  static create<T>(): Set<T> { return new Set(); }
   static createFromList<T>(lst: List<T>): Set<T> { return createSetFromList(lst); }
   static has<T>(s: Set<T>, key: T): boolean { return s.has(key); }
 }
diff --git a/modules/angular2/src/render/dom/compiler/directive_parser.ts b/modules/angular2/src/render/dom/compiler/directive_parser.ts
@@ -151,6 +151,7 @@ export class DirectiveParser implements CompileStep {
       var fullExpAstWithBindPipes = this._parser.addPipes(bindingAst, pipes);
       directiveBinderBuilder.bindProperty(dirProperty, fullExpAstWithBindPipes);
     }
+    compileElement.bindElement().bindPropertyToDirective(dashCaseToCamelCase(elProp));
   }
 
   _bindDirectiveEvent(eventName, action, compileElement, directiveBinderBuilder) {

diff --git a/modules/angular2/src/render/dom/view/property_setter_factory.ts b/modules/angular2/src/render/dom/view/property_setter_factory.ts
@@ -18,7 +18,7 @@ const CLASS_PREFIX = 'class.';
 const STYLE_PREFIX = 'style.';
 
 export class PropertySetterFactory {
-  private static _noopSetter(el, value) {}
+  static noopSetter(el, value) {}
 
   private _lazyPropertySettersCache: StringMap<string, Function> = StringMapWrapper.create();
   private _eagerPropertySettersCache: StringMap<string, Function> = StringMapWrapper.create();
@@ -69,7 +69,7 @@ export class PropertySetterFactory {
         if (DOM.hasProperty(protoElement, property)) {
           setterFn = reflector.setter(property);
         } else {
-          setterFn = PropertySetterFactory._noopSetter;
+          setterFn = PropertySetterFactory.noopSetter;
         }
         StringMapWrapper.set(this._eagerPropertySettersCache, property, setterFn);
       }

diff --git a/modules/angular2/src/render/dom/view/proto_view_builder.ts b/modules/angular2/src/render/dom/view/proto_view_builder.ts
@@ -1,4 +1,4 @@
-import {isPresent, isBlank, BaseException} from 'angular2/src/facade/lang';
+import {StringWrapper, isPresent, isBlank, BaseException} from 'angular2/src/facade/lang';
 import {ListWrapper, MapWrapper, Set, SetWrapper, List} from 'angular2/src/facade/collection';
 import {DOM} from 'angular2/src/dom/dom_adapter';
 
@@ -74,11 +74,21 @@ export class ProtoViewBuilder {
       });
 
       MapWrapper.forEach(ebb.propertyBindings, (_, propertyName) => {
-        MapWrapper.set(
-            propertySetters, propertyName,
-            setterFactory.createSetter(ebb.element, isPresent(ebb.componentId), propertyName));
+        var propSetter =
+            setterFactory.createSetter(ebb.element, isPresent(ebb.componentId), propertyName);
+
+        if (propSetter === PropertySetterFactory.noopSetter) {
+          if (!SetWrapper.has(ebb.propertyBindingsToDirectives, propertyName)) {
+            throw new BaseException(
+                `Can't bind to '${propertyName}' since it isn't a know property of the '${StringWrapper.toLowerCase(DOM.nodeName(ebb.element))}' element`);
+          }
+        }
+
+        MapWrapper.set(propertySetters, propertyName, propSetter);
       });
 
+
+
       var nestedProtoView =
           isPresent(ebb.nestedProtoView) ? ebb.nestedProtoView.build(setterFactory) : null;
       var nestedRenderProtoView =
@@ -150,6 +160,7 @@ export class ElementBinderBuilder {
   directives: List<DirectiveBuilder> = [];
   nestedProtoView: ProtoViewBuilder = null;
   propertyBindings: Map<string, ASTWithSource> = MapWrapper.create();
+  propertyBindingsToDirectives: Set<string> = SetWrapper.create();
   variableBindings: Map<string, string> = MapWrapper.create();
   eventBindings: List<api.EventBinding> = [];
   eventBuilder: EventBuilder = new EventBuilder();
@@ -191,6 +202,12 @@ export class ElementBinderBuilder {
 
   bindProperty(name, expression) { MapWrapper.set(this.propertyBindings, name, expression); }
 
+  bindPropertyToDirective(name: string) {
+    // we are filling in a set of property names that are bound to a property
+    // of at least one directive. This allows us to report "dangling" bindings.
+    this.propertyBindingsToDirectives.add(name);
+  }
+
   bindVariable(name, value) {
     // When current is a view root, the variable bindings are set to the *nested* proto view.
     // The root view conceptually signifies a new "block scope" (the nested view), to which

diff --git a/modules/angular2/test/core/compiler/integration_spec.ts b/modules/angular2/test/core/compiler/integration_spec.ts
@@ -191,22 +191,6 @@ export function main() {
                });
          }));
 
-      it('should ignore bindings to unknown properties',
-         inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
-           tb.overrideView(MyComp,
-                           new viewAnn.View({template: '<div unknown="{{ctxProp}}"></div>'}));
-
-           tb.createView(MyComp, {context: ctx})
-               .then((view) => {
-
-                 ctx.ctxProp = 'Some value';
-                 view.detectChanges();
-                 expect(DOM.hasProperty(view.rootNodes[0], 'unknown')).toBeFalsy();
-
-                 async.done();
-               });
-         }));
-
       it('should consume directive watch expression change.',
          inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
            var tpl = '<div>' +
@@ -444,7 +428,7 @@ export function main() {
         it('should assign a directive to a var-',
            inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
              tb.overrideView(MyComp, new viewAnn.View({
-               template: '<p><div [export-dir] #localdir="dir"></div></p>',
+               template: '<p><div export-dir #localdir="dir"></div></p>',
                directives: [ExportDir]
              }));
 
@@ -1146,7 +1130,7 @@ export function main() {
       it('should specify a location of an error that happened during change detection (element property)',
          inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
 
-           tb.overrideView(MyComp, new viewAnn.View({template: '<div [prop]="a.b"></div>'}));
+           tb.overrideView(MyComp, new viewAnn.View({template: '<div [title]="a.b"></div>'}));
 
            tb.createView(MyComp, {context: ctx})
                .then((view) => {
@@ -1159,10 +1143,10 @@ export function main() {
       it('should specify a location of an error that happened during change detection (directive property)',
          inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
 
-           tb.overrideView(
-               MyComp,
-               new viewAnn.View(
-                   {template: '<child-cmp [prop]="a.b"></child-cmp>', directives: [ChildComp]}));
+           tb.overrideView(MyComp, new viewAnn.View({
+             template: '<child-cmp [dir-prop]="a.b"></child-cmp>',
+             directives: [ChildComp]
+           }));
 
            tb.createView(MyComp, {context: ctx})
                .then((view) => {
@@ -1207,6 +1191,30 @@ export function main() {
          });
        }));
 
+    describe('Missing property bindings', () => {
+      it('should throw on bindings to unknown properties',
+         inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
+           tb.overrideView(MyComp,
+                           new viewAnn.View({template: '<div unknown="{{ctxProp}}"></div>'}));
+
+           PromiseWrapper.catchError(tb.createView(MyComp, {context: ctx}), (e) => {
+             expect(e.message).toEqual(
+                 `Can't bind to 'unknown' since it isn't a know property of the 'div' element`);
+             async.done();
+             return null;
+           });
+         }));
+
+      it('should not throw for property binding to a non-existing property when there is a matching directive property',
+         inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
+           tb.overrideView(
+               MyComp,
+               new viewAnn.View(
+                   {template: '<div my-dir [elprop]="ctxProp"></div>', directives: [MyDir]}));
+           tb.createView(MyComp, {context: ctx}).then((val) => { async.done(); });
+         }));
+    });
+
     // Disabled until a solution is found, refs:
     // - https://github.com/angular/angular/issues/776
     // - https://github.com/angular/angular/commit/81f3f32
@@ -1356,10 +1364,7 @@ class ComponentWithPipes {
   prop: string;
 }
 
-@Component({
-  selector: 'child-cmp',
-  appInjector: [MyService],
-})
+@Component({selector: 'child-cmp', properties: ['dirProp'], appInjector: [MyService]})
 @View({directives: [MyDir], template: '{{ctxProp}}'})
 @Injectable()
 class ChildComp {

diff --git a/modules/angular2/test/core/directive_lifecycle_integration_spec.ts b/modules/angular2/test/core/directive_lifecycle_integration_spec.ts
@@ -36,7 +36,7 @@ export function main() {
          tb.overrideView(
              MyComp,
              new viewAnn.View(
-                 {template: '<div [field]="123" [lifecycle]></div>', directives: [LifecycleDir]}));
+                 {template: '<div [field]="123" lifecycle></div>', directives: [LifecycleDir]}));
 
          tb.createView(MyComp, {context: ctx})
              .then((view) => {

diff --git a/modules/angular2/test/directives/ng_for_spec.ts b/modules/angular2/test/directives/ng_for_spec.ts
@@ -205,7 +205,7 @@ export function main() {
 
     it('should repeat over nested arrays with no intermediate element',
        inject([TestBed, AsyncTestCompleter], (tb: TestBed, async) => {
-         var template = '<div><template [ng-for] #item [ng-for-of]="items">' +
+         var template = '<div><template ng-for #item [ng-for-of]="items">' +
                         '<div template="ng-for #subitem of item">' +
                         '{{subitem}}-{{item.length}};' +
                         '</div></template></div>';

diff --git a/modules/angular2/test/directives/ng_switch_spec.ts b/modules/angular2/test/directives/ng_switch_spec.ts
@@ -79,8 +79,8 @@ export function main() {
                           '<template [ng-switch-when]="\'b\'"><li>when b1;</li></template>' +
                           '<template [ng-switch-when]="\'a\'"><li>when a2;</li></template>' +
                           '<template [ng-switch-when]="\'b\'"><li>when b2;</li></template>' +
-                          '<template [ng-switch-default]><li>when default1;</li></template>' +
-                          '<template [ng-switch-default]><li>when default2;</li></template>' +
+                          '<template ng-switch-default><li>when default1;</li></template>' +
+                          '<template ng-switch-default><li>when default2;</li></template>' +
                           '</ul></div>';
 
            tb.createView(TestComponent, {html: template})
@@ -108,7 +108,7 @@ export function main() {
                           '<ul [ng-switch]="switchValue">' +
                           '<template [ng-switch-when]="when1"><li>when 1;</li></template>' +
                           '<template [ng-switch-when]="when2"><li>when 2;</li></template>' +
-                          '<template [ng-switch-default]><li>when default;</li></template>' +
+                          '<template ng-switch-default><li>when default;</li></template>' +
                           '</ul></div>';
 
            tb.createView(TestComponent, {html: template})

diff --git a/modules/angular2/test/render/dom/shadow_dom_emulation_integration_spec.ts b/modules/angular2/test/render/dom/shadow_dom_emulation_integration_spec.ts
@@ -460,7 +460,7 @@ var conditionalContentComponent = DirectiveMetadata.create({
 });
 
 var autoViewportDirective = DirectiveMetadata.create(
-    {selector: '[auto]', id: '[auto]', type: DirectiveMetadata.DIRECTIVE_TYPE});
+    {selector: '[auto]', id: 'auto', properties: ['auto'], type: DirectiveMetadata.DIRECTIVE_TYPE});
 
 var tabComponent =
     DirectiveMetadata.create({selector: 'tab', id: 'tab', type: DirectiveMetadata.COMPONENT_TYPE});

diff --git a/modules/benchmarks/src/costs/index.ts b/modules/benchmarks/src/costs/index.ts
@@ -77,7 +77,7 @@ class DynamicDummy {
     </div>
 
     <div *ng-if="testingWithDirectives">
-      <dummy [dummy-decorator] *ng-for="#i of list"></dummy>
+      <dummy dummy-decorator *ng-for="#i of list"></dummy>
     </div>
 
     <div *ng-if="testingDynamicComponents">

diff --git a/modules/benchmarks/src/largetable/largetable_benchmark.ts b/modules/benchmarks/src/largetable/largetable_benchmark.ts
@@ -232,7 +232,7 @@ class CellData {
         </tbody>
         <tbody template="ng-switch-when 'interpolationAttr'">
           <tr template="ng-for #row of data">
-            <td template="ng-for #column of row" i="{{column.i}}" j="{{column.j}}">
+            <td template="ng-for #column of row" attr.i="{{column.i}}" attr.j="{{column.j}}">
               i,j attrs
             </td>
           </tr>
@@ -269,7 +269,7 @@ class LargetableComponent {
 @Component({selector: 'app'})
 @View({
   directives: [LargetableComponent],
-  template: `<largetable [data]='data' [benchmarkType]='benchmarkType'></largetable>`
+  template: `<largetable [data]='data' [benchmark-type]='benchmarkType'></largetable>`
 })
 class AppComponent {
   data;