refactor($compile): detect and log bad directive 'restrict' values on directive factory init

Author: jbedard

benchmarks/js-bp/app.js

@@ -0,0 +1,256 @@
+var app = angular.module('jsBenchmark', []);
+
+function runner(func, n) {
+  n = n || 5000;
+  return function theRunner() {
+    for (var i=0; i<n; i++) {
+      func();
+    }
+  };
+}
+
+/* copy */
+app.controller('DataController', ['$compile', function($compile) {
+  var COPY_BASIC = {
+    i: 123,
+    str: "foo",
+    date: new Date(),
+    sub: {event: "foo"}
+  };
+  var COPY_ARRAY_BASIC = [123, "foo", new Date(), {event: "foo"}];
+  var COPY_ARRAY_SIMPLE = [123, "foo", new Date(), /foo/];
+
+  var BIG_ARRAY_OBJECTS = new Array(50).join(" ").split(" ").map(function(s, i) {
+    return {
+      i: i,
+      s: s + i,
+      o: {foo: "bar", date: new Date()},
+      a: [/foo/, new Int32Array()]
+    };
+  });
+
+  var BIG_COMPLICATED_ARRAY_OBJECTS = new Array(50).join(" ").split(" ").reduce(function(a, s, i) {
+    var o = {};
+
+    o.i = i;
+    o.s = s + i;
+    o.d = (i % 5 === 1) ? a[i-1].d : new Date();
+    o.re = (i % 5 === 2) ? a[i-2].re : /foobar/i;
+    o.a = (i % 5 === 3) ? a[i-3].a : ['foo', 123];
+
+    if (i % 5 === 0) {
+      o.o = o;
+    } else {
+      o.o = {foo: "bar"};
+    }
+    if (i % 4 === 1) {
+      o.p = a[i-1];
+    }
+
+    a.push(o);
+    if (i % 20 === 0) {
+      a.push(o);
+    }
+
+    return a;
+  }, []);
+
+  function copy(what) {
+    for (var i=0; i<1000; i++) {
+      angular.copy(what);
+    }
+  }
+
+  benchmarkSteps.push({
+    name: 'large array of objects',
+    fn: copy.bind(null, BIG_ARRAY_OBJECTS)
+  });
+  benchmarkSteps.push({
+    name: 'large array of over complicated objects',
+    fn: copy.bind(null, BIG_COMPLICATED_ARRAY_OBJECTS)
+  });
+  benchmarkSteps.push({
+    name: 'basic + date + 1 recurse',
+    fn: copy.bind(null, COPY_BASIC)
+  });
+  benchmarkSteps.push({
+    name: 'basic array + date + 1 recurse',
+    fn: copy.bind(null, COPY_ARRAY_BASIC)
+  });
+  benchmarkSteps.push({
+    name: 'simple',
+    fn: copy.bind(null, COPY_ARRAY_SIMPLE)
+  });
+}]);
+// */
+
+/* Q * /
+app.controller('DataController', ['$q', function($q) {
+  var P = $q.resolve(42);
+  var rP = function() { return P; };
+
+  benchmarkSteps.push({
+    name: 'resolve(1)',
+    fn: runner(function() {
+      return $q.resolve(1);
+    })
+  });
+  benchmarkSteps.push({
+    name: 'reject(1)',
+    fn: runner(function() {
+      return $q.reject(1);
+    })
+  });
+  benchmarkSteps.push({
+    name: 'defer.resolve(1)',
+    fn: runner(function() {
+      return $q.defer().resolve(1);
+    })
+  });
+  benchmarkSteps.push({
+    name: 'defer.reject(1)',
+    fn: runner(function() {
+      return $q.defer().reject(1);      //3206 => 3285 (because notify gets wrapped twice now??)
+    })
+  });
+
+  benchmarkSteps.push({
+    name: 'resolve(P)',
+    fn: runner(function() {
+      return $q.resolve(P);             //way down, but still high retained memory
+    })
+  });
+  benchmarkSteps.push({
+    name: 'defer.resolve(P)',
+    fn: runner(function() {
+      return $q.defer().resolve(P);     //way down, but still high retained memory
+    })
+  });
+
+  benchmarkSteps.push({
+    name: '.then()',
+    fn: runner(function() {
+      return P.then();
+    })
+  });
+  benchmarkSteps.push({
+    name: '.then(rP)',
+    fn: runner(function() {
+      return P.then(rP);
+    })
+  });
+
+  benchmarkSteps.push({
+    name: 'Q(noop)',
+    fn: runner(function() {
+      return $q(angular.noop);
+    })
+  });
+}]);
+// */
+
+/* $compile * /
+app.controller('DataController', ['$compile', '$rootScope', function($compile, $rootScope) {
+  var HTML = document.querySelector(".container-fluid").innerHTML;
+
+
+  var i=0;
+  HTML = HTML.replace(/(<[a-z]+)/g, function(a, r) {
+    if (r % 5) {
+      r += ' ng-if="B"';
+    } else if (r % 11) {
+      r += ' ng-repeat="b in A"';
+    } else if (r % 2) {
+      r += ' ng-model="b"'
+    }
+
+    i++;
+    return r;
+  });
+
+  HTML = HTML.replace(/></g, " ng-bind='C'><");
+
+
+  var step;
+  benchmarkSteps.push({
+    name: '$compile',
+    fn: function() {
+      step = $compile(HTML);
+    }
+  });
+  benchmarkSteps.push({
+    name: 'link',
+    fn: function() {
+      step = step(angular.extend(scope = $rootScope.$new(), {
+        A: [1,2,3,4,5],
+        C: 123
+      }));
+    }
+  });
+  benchmarkSteps.push({
+    name: 'remove',
+    fn: function() {
+      step.remove();
+      step = null;
+
+      scope.$destroy();
+      scope = null;
+    }
+  });
+}]);
+// */
+
+
+/* compiler * /
+app.directive('templateUrl', ['$templateCache', function($templateCache) {
+  $templateCache.put('template', '<span>');
+
+  var VAL = 0;
+
+  return {
+    templateUrl: 'template',
+    compile: function($el, $at) {
+      $el.text("{{val}}");
+      return function($scope, $el) {
+        $scope.val = VAL++;
+      };
+    }
+  };
+}]);
+
+app.controller('DataController', ['$compile', '$rootScope', function($compile, $rootScope) {
+  var HTML = '<aside><div ng-repeat="i in a track by $index"><template-url/></div></aside>';
+
+  var step, scope;
+
+  benchmarkSteps.push({
+    name: '$compile',
+    fn: function() {
+      step = angular.element(HTML);
+      angular.element(document.body).append(step);
+      step = $compile(step);
+    }
+  });
+
+  benchmarkSteps.push({
+    name: 'link',
+    fn: function() {
+      scope = $rootScope.$new();
+      scope.a = new Array(5000).join(" ").split(" ");
+      step = step(scope);
+      $rootScope.$apply();
+    }
+  });
+
+  benchmarkSteps.push({
+    name: '$destroy',
+    fn: function() {
+      scope.$destroy();
+      step.remove();
+      scope = step = null;
+    }
+  });
+}]);
+// */
+
+angular.bootstrap(document.querySelector("[ng-app-foo]"), ["jsBenchmark"]);

benchmarks/js-bp/bp.conf.js

@@ -0,0 +1,15 @@
+module.exports = function(config) {
+  config.set({
+    scripts: [{
+      id: 'jquery',
+      src: 'jquery-noop.js'
+    },
+    {
+      id: 'angular',
+      src: '/build/angular.js'
+    },
+    {
+      src: 'app.js',
+    }]
+  });
+};

benchmarks/js-bp/jquery-noop.js

@@ -0,0 +1 @@
+//Override me with ?jquery=/bower_components/jquery/dist/jquery.js

benchmarks/js-bp/main.html

@@ -0,0 +1,3 @@
+<div ng-app-foo>
+  <div ng-controller="DataController as ctrl"></div>
+</div>

docs/content/error/$compile/badrestrict.ngdoc

@@ -0,0 +1,12 @@
+@ngdoc error
+@name $compile:badrestrict
+@fullName Invalid Directive Restrict
+@description
+
+This error occurs when the restrict property of a directive is not valid.
+
+The directive restrict property must be a string with possible character values being:
+* E (element)
+* A (attribute)
+* C (class)
+* M (comment)

src/ng/compile.js

@@ -969,6 +969,17 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
     return require;
   }
 
+  function getDirectiveRestrict(directive, name) {
+    if (directive.restrict != null && !(isString(directive.restrict) && /[EACM]/.test(directive.restrict))) {
+      throw $compileMinErr('badrestrict',
+          "Restrict property '{0}' of directive '{1}' is invalid",
+          directive.restrict,
+          name);
+    }
+
+    return directive.restrict || 'EA';
+  }
+
   /**
    * @ngdoc method
    * @name $compileProvider#directive
@@ -989,6 +1000,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
     if (isString(name)) {
       assertValidDirectiveName(name);
       assertArg(directiveFactory, 'directiveFactory');
+
       if (!hasDirectives.hasOwnProperty(name)) {
         hasDirectives[name] = [];
         $provide.factory(name + Suffix, ['$injector', '$exceptionHandler',
@@ -1006,7 +1018,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
                 directive.index = index;
                 directive.name = directive.name || name;
                 directive.require = getDirectiveRequire(directive);
-                directive.restrict = directive.restrict || 'EA';
+                directive.restrict = getDirectiveRestrict(directive, name);
                 directive.$$moduleName = directiveFactory.$$moduleName;
                 directives.push(directive);
               } catch (e) {

test/ng/compileSpec.js

@@ -5558,6 +5558,43 @@ describe('$compile', function() {
     });
 
 
+    it('should throw badrestrict on first compilation when restrict is invalid', function() {
+      module(function($compileProvider, $exceptionHandlerProvider) {
+        $compileProvider.directive('invalidRestrictBadString', valueFn({restrict: '"'}));
+        $compileProvider.directive('invalidRestrictFalse', valueFn({restrict: false}));
+        $compileProvider.directive('invalidRestrictTrue', valueFn({restrict: true}));
+        $compileProvider.directive('invalidRestrictObject', valueFn({restrict: {}}));
+        $compileProvider.directive('invalidRestrictZero', valueFn({restrict: 0}));
+
+        // We need to test with the exceptionHandler not rethrowing...
+        $exceptionHandlerProvider.mode('log');
+      });
+
+      inject(function($exceptionHandler, $compile, $rootScope) {
+        $compile('<div invalid-restrict-false>')($rootScope);
+        expect($exceptionHandler.errors.length).toBe(1);
+        expect($exceptionHandler.errors[0]).toMatch(/\$compile.*badrestrict.*'false'/);
+
+        $compile('<div invalid-restrict-true>')($rootScope);
+        expect($exceptionHandler.errors.length).toBe(2);
+        expect($exceptionHandler.errors[1]).toMatch(/\$compile.*badrestrict.*'true'/);
+
+        $compile('<div invalid-restrict-bad-string>')($rootScope);
+        $compile('<div invalid-restrict-bad-string>')($rootScope);
+        expect($exceptionHandler.errors.length).toBe(3);
+        expect($exceptionHandler.errors[2]).toMatch(/\$compile.*badrestrict.*'\"'/);
+
+        $compile('<div invalid-restrict-bad-string invalid-restrict-object>')($rootScope);
+        expect($exceptionHandler.errors.length).toBe(4);
+        expect($exceptionHandler.errors[3]).toMatch(/\$compile.*badrestrict.*'{}'/);
+
+        $compile('<div invalid-restrict-object invalid-restrict-zero>')($rootScope);
+        expect($exceptionHandler.errors.length).toBe(5);
+        expect($exceptionHandler.errors[4]).toMatch(/\$compile.*badrestrict.*'0'/);
+      });
+    });
+
+
     it('should throw noident when missing controllerAs directive property', function() {
       module(function($compileProvider) {
         $compileProvider.directive('noIdent', valueFn({