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

Commit 11f2731

Browse files
jbedardNarretz
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 1e1fbc7 commit 11f2731

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) {
@@ -2912,24 +2923,22 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
29122923
if (hasDirectives.hasOwnProperty(name)) {
29132924
for (var directive, directives = $injector.get(name + Suffix),
29142925
i = 0, ii = directives.length; i < ii; i++) {
2915-
try {
2916-
directive = directives[i];
2917-
if ((isUndefined(maxPriority) || maxPriority > directive.priority) &&
2918-
directive.restrict.indexOf(location) !== -1) {
2919-
if (startAttrName) {
2920-
directive = inherit(directive, {$$start: startAttrName, $$end: endAttrName});
2921-
}
2922-
if (!directive.$$bindings) {
2923-
var bindings = directive.$$bindings =
2924-
parseDirectiveBindings(directive, directive.name);
2925-
if (isObject(bindings.isolateScope)) {
2926-
directive.$$isolateBindings = bindings.isolateScope;
2927-
}
2926+
directive = directives[i];
2927+
if ((isUndefined(maxPriority) || maxPriority > directive.priority) &&
2928+
directive.restrict.indexOf(location) !== -1) {
2929+
if (startAttrName) {
2930+
directive = inherit(directive, {$$start: startAttrName, $$end: endAttrName});
2931+
}
2932+
if (!directive.$$bindings) {
2933+
var bindings = directive.$$bindings =
2934+
parseDirectiveBindings(directive, directive.name);
2935+
if (isObject(bindings.isolateScope)) {
2936+
directive.$$isolateBindings = bindings.isolateScope;
29282937
}
2929-
tDirectives.push(directive);
2930-
match = directive;
29312938
}
2932-
} catch (e) { $exceptionHandler(e); }
2939+
tDirectives.push(directive);
2940+
match = directive;
2941+
}
29332942
}
29342943
}
29352944
return match;

test/ng/compileSpec.js

+32
Original file line numberDiff line numberDiff line change
@@ -5877,6 +5877,38 @@ describe('$compile', function() {
58775877
});
58785878

58795879

5880+
it('should throw badrestrict on first compilation when restrict is invalid', function() {
5881+
module(function($compileProvider, $exceptionHandlerProvider) {
5882+
$compileProvider.directive('invalidRestrictBadString', valueFn({restrict: '"'}));
5883+
$compileProvider.directive('invalidRestrictTrue', valueFn({restrict: true}));
5884+
$compileProvider.directive('invalidRestrictObject', valueFn({restrict: {}}));
5885+
$compileProvider.directive('invalidRestrictNumber', valueFn({restrict: 42}));
5886+
5887+
// We need to test with the exceptionHandler not rethrowing...
5888+
$exceptionHandlerProvider.mode('log');
5889+
});
5890+
5891+
inject(function($exceptionHandler, $compile, $rootScope) {
5892+
$compile('<div invalid-restrict-true>')($rootScope);
5893+
expect($exceptionHandler.errors.length).toBe(1);
5894+
expect($exceptionHandler.errors[0]).toMatch(/\$compile.*badrestrict.*'true'/);
5895+
5896+
$compile('<div invalid-restrict-bad-string>')($rootScope);
5897+
$compile('<div invalid-restrict-bad-string>')($rootScope);
5898+
expect($exceptionHandler.errors.length).toBe(2);
5899+
expect($exceptionHandler.errors[1]).toMatch(/\$compile.*badrestrict.*'"'/);
5900+
5901+
$compile('<div invalid-restrict-bad-string invalid-restrict-object>')($rootScope);
5902+
expect($exceptionHandler.errors.length).toBe(3);
5903+
expect($exceptionHandler.errors[2]).toMatch(/\$compile.*badrestrict.*'{}'/);
5904+
5905+
$compile('<div invalid-restrict-object invalid-restrict-number>')($rootScope);
5906+
expect($exceptionHandler.errors.length).toBe(4);
5907+
expect($exceptionHandler.errors[3]).toMatch(/\$compile.*badrestrict.*'42'/);
5908+
});
5909+
});
5910+
5911+
58805912
it('should throw noident when missing controllerAs directive property', function() {
58815913
module(function($compileProvider) {
58825914
$compileProvider.directive('noIdent', valueFn({

0 commit comments

Comments
 (0)