From 81f3f32217936fa26b433ad8c8b8f26145d72865 Mon Sep 17 00:00:00 2001
From: Pawel Kozlowski <pkozlowski.opensource@gmail.com>
Date: Thu, 19 Mar 2015 20:56:19 +0100
Subject: [PATCH] refactor(DirectiveParser): remove checks for missing
 directives

Based on the discussion in #776 we can't reliably check if a given
element has a particular property at the compilation time. As such
the existing algorithm detecting "missing" directives can't be used.

We need to see if there is a different / better algorithm or maybe
those checks need to be moved later in the process (runtime). Leaving
integration tests in place (disabled) so we can come back to the
topic after unblocking the situation.

This commit effectivelly reverts 94e203b9df6c4b79ba30f1f08fc54ff919f722e1
---
 .../compiler/pipeline/directive_parser.js     |  69 +-----------
 .../pipeline/element_binder_builder.js        |  16 +--
 .../test/core/compiler/integration_spec.js    | 101 +++++++++---------
 3 files changed, 63 insertions(+), 123 deletions(-)

diff --git a/modules/angular2/src/core/compiler/pipeline/directive_parser.js b/modules/angular2/src/core/compiler/pipeline/directive_parser.js
index 864f2ceec2..2c60b0f7f2 100644
--- a/modules/angular2/src/core/compiler/pipeline/directive_parser.js
+++ b/modules/angular2/src/core/compiler/pipeline/directive_parser.js
@@ -1,5 +1,5 @@
 import {isPresent, isBlank, BaseException, assertionsEnabled, RegExpWrapper} from 'angular2/src/facade/lang';
-import {List, MapWrapper, StringMapWrapper} from 'angular2/src/facade/collection';
+import {List, MapWrapper} from 'angular2/src/facade/collection';
 import {DOM} from 'angular2/src/dom/dom_adapter';
 import {SelectorMatcher} from '../selector';
 import {CssSelector} from '../selector';
@@ -10,9 +10,6 @@ import {CompileStep} from './compile_step';
 import {CompileElement} from './compile_element';
 import {CompileControl} from './compile_control';
 
-import {isSpecialProperty} from './element_binder_builder';
-import {dashCaseToCamelCase, camelCaseToDashCase} from './util';
-
 var PROPERTY_BINDING_REGEXP = RegExpWrapper.create('^ *([^\\s\\|]+)');
 
 /**
@@ -60,53 +57,15 @@ export class DirectiveParser extends CompileStep {
     });
 
     // Note: We assume that the ViewSplitter already did its work, i.e. template directive should
-    // only be present on <template> elements any more!
+    // only be present on <template> elements!
     var isTemplateElement = DOM.isTemplateElement(current.element);
-    var matchedProperties; // StringMap - used in dev mode to store all properties that have been matched
 
     this._selectorMatcher.match(cssSelector, (selector, directive) => {
-      matchedProperties = updateMatchedProperties(matchedProperties, selector, directive);
-      checkDirectiveValidity(directive, current, isTemplateElement);
-      current.addDirective(directive);
+      current.addDirective(checkDirectiveValidity(directive, current, isTemplateElement));
     });
-
-    // raise error if some directives are missing
-    checkMissingDirectives(current, matchedProperties, isTemplateElement);
   }
 }
 
-// calculate all the properties that are used or interpreted by all directives
-// those properties correspond to the directive selectors and the directive bindings
-function updateMatchedProperties(matchedProperties, selector, directive) {
-  if (assertionsEnabled()) {
-    var attrs = selector.attrs;
-    if (!isPresent(matchedProperties)) {
-      matchedProperties = StringMapWrapper.create();
-    }
-    if (isPresent(attrs)) {
-      for (var idx = 0; idx<attrs.length; idx+=2) {
-        // attribute name is stored on even indexes
-        StringMapWrapper.set(matchedProperties, dashCaseToCamelCase(attrs[idx]), true);
-      }
-    }
-    // some properties can be used by the directive, so we need to register them
-    if (isPresent(directive.annotation) && isPresent(directive.annotation.bind)) {
-      var bindMap = directive.annotation.bind;
-      StringMapWrapper.forEach(bindMap, (value, key) => {
-        // value is the name of the property that is interpreted
-        // e.g. 'myprop' or 'myprop | double' when a pipe is used to transform the property
-
-        // keep the property name and remove the pipe
-        var bindProp = RegExpWrapper.firstMatch(PROPERTY_BINDING_REGEXP, value);
-        if (isPresent(bindProp) && isPresent(bindProp[1])) {
-          StringMapWrapper.set(matchedProperties, dashCaseToCamelCase(bindProp[1]), true);
-        }
-      });
-    }
-  }
-  return matchedProperties;
-}
-
 // check if the directive is compatible with the current element
 function checkDirectiveValidity(directive, current, isTemplateElement) {
   var isComponent = directive.annotation instanceof Component || directive.annotation instanceof DynamicComponent;
@@ -125,26 +84,6 @@ function checkDirectiveValidity(directive, current, isTemplateElement) {
   } else if (isComponent && alreadyHasComponent) {
     throw new BaseException(`Multiple component directives not allowed on the same element - check ${current.elementDescription}`);
   }
-}
 
-// validates that there is no missing directive - dev mode only
-function checkMissingDirectives(current, matchedProperties, isTemplateElement) {
-  if (assertionsEnabled()) {
-    var ppBindings=current.propertyBindings;
-    if (isPresent(ppBindings)) {
-      // check that each property corresponds to a real property or has been matched by a directive
-      MapWrapper.forEach(ppBindings, (expression, prop) => {
-        if (!DOM.hasProperty(current.element, prop) && !isSpecialProperty(prop)) {
-          if (!isPresent(matchedProperties) || !isPresent(StringMapWrapper.get(matchedProperties, prop))) {
-            throw new BaseException(`Missing directive to handle '${camelCaseToDashCase(prop)}' in ${current.elementDescription}`);
-          }
-        }
-      });
-    }
-    // template only store directives as attribute when they are not bound to expressions
-    // so we have to validate the expression case too (e.g. !if="condition")
-    if (isTemplateElement && !current.isViewRoot && !isPresent(current.viewportDirective)) {
-      throw new BaseException(`Missing directive to handle: ${current.elementDescription}`);
-    }
-  }
+  return directive;
 }
diff --git a/modules/angular2/src/core/compiler/pipeline/element_binder_builder.js b/modules/angular2/src/core/compiler/pipeline/element_binder_builder.js
index d252295d50..bd2059f251 100644
--- a/modules/angular2/src/core/compiler/pipeline/element_binder_builder.js
+++ b/modules/angular2/src/core/compiler/pipeline/element_binder_builder.js
@@ -93,12 +93,16 @@ function styleSetterFactory(styleName:string, stylesuffix:string) {
   return setterFn;
 }
 
-// tells if an attribute is handled by the ElementBinderBuilder step
-export function isSpecialProperty(propName:string) {
-  return StringWrapper.startsWith(propName, ATTRIBUTE_PREFIX)
-        || StringWrapper.startsWith(propName, CLASS_PREFIX)
-        || StringWrapper.startsWith(propName, STYLE_PREFIX)
-        || StringMapWrapper.contains(DOM.attrToPropMap, propName);
+const ROLE_ATTR = 'role';
+function roleSetter(element, value) {
+  if (isString(value)) {
+    DOM.setAttribute(element, ROLE_ATTR, value);
+  } else {
+    DOM.removeAttribute(element, ROLE_ATTR);
+    if (isPresent(value)) {
+      throw new BaseException("Invalid role attribute, only string values are allowed, got '" + stringify(value) + "'");
+    }
+  }
 }
 
 /**
diff --git a/modules/angular2/test/core/compiler/integration_spec.js b/modules/angular2/test/core/compiler/integration_spec.js
index a8f3c61faa..61c0187bc1 100644
--- a/modules/angular2/test/core/compiler/integration_spec.js
+++ b/modules/angular2/test/core/compiler/integration_spec.js
@@ -2,6 +2,7 @@ import {
   AsyncTestCompleter,
   beforeEach,
   ddescribe,
+  xdescribe,
   describe,
   el,
   expect,
@@ -566,61 +567,57 @@ export function main() {
       }));
     });
 
-    if (assertionsEnabled()) {
+    xdescribe('Missing directive checks', () => {
 
-      function expectCompileError(inlineTpl, errMessage, done) {
-        tplResolver.setTemplate(MyComp, new Template({inline: inlineTpl}));
-        PromiseWrapper.then(compiler.compile(MyComp),
-          (value) => {
-            throw new BaseException("Test failure: should not have come here as an exception was expected");
-          },
-          (err) => {
-            expect(err.message).toEqual(errMessage);
-            done();
-          }
-        );
+      if (assertionsEnabled()) {
+
+        function expectCompileError(inlineTpl, errMessage, done) {
+          tplResolver.setTemplate(MyComp, new Template({inline: inlineTpl}));
+          PromiseWrapper.then(compiler.compile(MyComp),
+            (value) => {
+              throw new BaseException("Test failure: should not have come here as an exception was expected");
+            },
+            (err) => {
+              expect(err.message).toEqual(errMessage);
+              done();
+            }
+          );
+        }
+
+        it('should raise an error if no directive is registered for a template with template bindings', inject([AsyncTestCompleter], (async) => {
+          expectCompileError(
+            '<div><div template="if: foo"></div></div>',
+            'Missing directive to handle \'if\' in <div template="if: foo">',
+            () => async.done()
+          );
+        }));
+
+        it('should raise an error for missing template directive (1)', inject([AsyncTestCompleter], (async) => {
+          expectCompileError(
+            '<div><template foo></template></div>',
+            'Missing directive to handle: <template foo>',
+            () => async.done()
+          );
+        }));
+
+        it('should raise an error for missing template directive (2)', inject([AsyncTestCompleter], (async) => {
+          expectCompileError(
+            '<div><template *if="condition"></template></div>',
+            'Missing directive to handle: <template *if="condition">',
+            () => async.done()
+          );
+        }));
+
+        it('should raise an error for missing template directive (3)', inject([AsyncTestCompleter], (async) => {
+          expectCompileError(
+            '<div *if="condition"></div>',
+            'Missing directive to handle \'if\' in MyComp: <div *if="condition">',
+            () => async.done()
+          );
+        }));
       }
+    });
 
-      it('should raise an error if no directive is registered for an unsupported DOM property', inject([AsyncTestCompleter], (async) => {
-        expectCompileError(
-          '<div [some-prop]="foo"></div>',
-          'Missing directive to handle \'some-prop\' in MyComp: <div [some-prop]="foo">',
-          () => async.done()
-        );
-      }));
-
-      it('should raise an error if no directive is registered for a template with template bindings', inject([AsyncTestCompleter], (async) => {
-        expectCompileError(
-          '<div><div template="if: foo"></div></div>',
-          'Missing directive to handle \'if\' in <div template="if: foo">',
-          () => async.done()
-        );
-      }));
-
-      it('should raise an error for missing template directive (1)', inject([AsyncTestCompleter], (async) => {
-        expectCompileError(
-          '<div><template foo></template></div>',
-          'Missing directive to handle: <template foo>',
-          () => async.done()
-        );
-      }));
-
-      it('should raise an error for missing template directive (2)', inject([AsyncTestCompleter], (async) => {
-        expectCompileError(
-          '<div><template *if="condition"></template></div>',
-          'Missing directive to handle: <template *if="condition">',
-          () => async.done()
-        );
-      }));
-
-      it('should raise an error for missing template directive (3)', inject([AsyncTestCompleter], (async) => {
-        expectCompileError(
-          '<div *if="condition"></div>',
-          'Missing directive to handle \'if\' in MyComp: <div *if="condition">',
-          () => async.done()
-        );
-      }));
-    }
   });
 }