Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Commit 31d464f

Browse files
jbedardpetebacondarwin
authored andcommitted
perf($compile): validate directive.restrict property on directive init
This allows the removal of try/catch from addDirective to avoid V8 deopt. Previously the directive.restrict property was not validated. This would potentially cause exceptions on each compilation of the directive requiring a try/catch and potentially causing repeated errors. New validation when directive.restrict is specified: * must be a string * must contain at least one valid character (E, A, C, M) Cases which previously silently failed (now throw an error): * values with an indexOf method (such as strings, arrays) which returned returned -1 for all valid restrict characters Cases which previously worked unintentionally (now throw an error): * arrays with single-character strings of valid restrict characters PR (#13263)
1 parent 5c9399d commit 31d464f

File tree

3 files changed

+76
-17
lines changed

3 files changed

+76
-17
lines changed
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
@ngdoc error
2+
@name $compile:badrestrict
3+
@fullName Invalid Directive Restrict
4+
@description
5+
6+
This error occurs when the restrict property of a directive is not valid.
7+
8+
The directive restrict property must be a string including one of more of the following characters:
9+
* E (element)
10+
* A (attribute)
11+
* C (class)
12+
* M (comment)
13+
14+
For example:
15+
```javascript
16+
restrict: 'E'
17+
restrict: 'EAC'
18+
```

src/ng/compile.js

+26-17
Original file line numberDiff line numberDiff line change
@@ -1071,6 +1071,17 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
10711071
return require;
10721072
}
10731073

1074+
function getDirectiveRestrict(restrict, name) {
1075+
if (restrict && !(isString(restrict) && /[EACM]/.test(restrict))) {
1076+
throw $compileMinErr('badrestrict',
1077+
'Restrict property \'{0}\' of directive \'{1}\' is invalid',
1078+
restrict,
1079+
name);
1080+
}
1081+
1082+
return restrict || 'EA';
1083+
}
1084+
10741085
/**
10751086
* @ngdoc method
10761087
* @name $compileProvider#directive
@@ -1109,7 +1120,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
11091120
directive.index = index;
11101121
directive.name = directive.name || name;
11111122
directive.require = getDirectiveRequire(directive);
1112-
directive.restrict = directive.restrict || 'EA';
1123+
directive.restrict = getDirectiveRestrict(directive.restrict, name);
11131124
directive.$$moduleName = directiveFactory.$$moduleName;
11141125
directives.push(directive);
11151126
} catch (e) {
@@ -2899,24 +2910,22 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
28992910
if (hasDirectives.hasOwnProperty(name)) {
29002911
for (var directive, directives = $injector.get(name + Suffix),
29012912
i = 0, ii = directives.length; i < ii; i++) {
2902-
try {
2903-
directive = directives[i];
2904-
if ((isUndefined(maxPriority) || maxPriority > directive.priority) &&
2905-
directive.restrict.indexOf(location) !== -1) {
2906-
if (startAttrName) {
2907-
directive = inherit(directive, {$$start: startAttrName, $$end: endAttrName});
2908-
}
2909-
if (!directive.$$bindings) {
2910-
var bindings = directive.$$bindings =
2911-
parseDirectiveBindings(directive, directive.name);
2912-
if (isObject(bindings.isolateScope)) {
2913-
directive.$$isolateBindings = bindings.isolateScope;
2914-
}
2913+
directive = directives[i];
2914+
if ((isUndefined(maxPriority) || maxPriority > directive.priority) &&
2915+
directive.restrict.indexOf(location) !== -1) {
2916+
if (startAttrName) {
2917+
directive = inherit(directive, {$$start: startAttrName, $$end: endAttrName});
2918+
}
2919+
if (!directive.$$bindings) {
2920+
var bindings = directive.$$bindings =
2921+
parseDirectiveBindings(directive, directive.name);
2922+
if (isObject(bindings.isolateScope)) {
2923+
directive.$$isolateBindings = bindings.isolateScope;
29152924
}
2916-
tDirectives.push(directive);
2917-
match = directive;
29182925
}
2919-
} catch (e) { $exceptionHandler(e); }
2926+
tDirectives.push(directive);
2927+
match = directive;
2928+
}
29202929
}
29212930
}
29222931
return match;

test/ng/compileSpec.js

+32
Original file line numberDiff line numberDiff line change
@@ -5829,6 +5829,38 @@ describe('$compile', function() {
58295829
});
58305830

58315831

5832+
it('should throw badrestrict on first compilation when restrict is invalid', function() {
5833+
module(function($compileProvider, $exceptionHandlerProvider) {
5834+
$compileProvider.directive('invalidRestrictBadString', valueFn({restrict: '"'}));
5835+
$compileProvider.directive('invalidRestrictTrue', valueFn({restrict: true}));
5836+
$compileProvider.directive('invalidRestrictObject', valueFn({restrict: {}}));
5837+
$compileProvider.directive('invalidRestrictNumber', valueFn({restrict: 42}));
5838+
5839+
// We need to test with the exceptionHandler not rethrowing...
5840+
$exceptionHandlerProvider.mode('log');
5841+
});
5842+
5843+
inject(function($exceptionHandler, $compile, $rootScope) {
5844+
$compile('<div invalid-restrict-true>')($rootScope);
5845+
expect($exceptionHandler.errors.length).toBe(1);
5846+
expect($exceptionHandler.errors[0]).toMatch(/\$compile.*badrestrict.*'true'/);
5847+
5848+
$compile('<div invalid-restrict-bad-string>')($rootScope);
5849+
$compile('<div invalid-restrict-bad-string>')($rootScope);
5850+
expect($exceptionHandler.errors.length).toBe(2);
5851+
expect($exceptionHandler.errors[1]).toMatch(/\$compile.*badrestrict.*'"'/);
5852+
5853+
$compile('<div invalid-restrict-bad-string invalid-restrict-object>')($rootScope);
5854+
expect($exceptionHandler.errors.length).toBe(3);
5855+
expect($exceptionHandler.errors[2]).toMatch(/\$compile.*badrestrict.*'{}'/);
5856+
5857+
$compile('<div invalid-restrict-object invalid-restrict-number>')($rootScope);
5858+
expect($exceptionHandler.errors.length).toBe(4);
5859+
expect($exceptionHandler.errors[3]).toMatch(/\$compile.*badrestrict.*'42'/);
5860+
});
5861+
});
5862+
5863+
58325864
it('should throw noident when missing controllerAs directive property', function() {
58335865
module(function($compileProvider) {
58345866
$compileProvider.directive('noIdent', valueFn({

0 commit comments

Comments
 (0)