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

refactor($compile): remove unnecessary try/catch #13263

Merged
merged 1 commit into from
Aug 30, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions docs/content/error/$compile/badrestrict.ngdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
@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 including one of more of the following characters:
* E (element)
* A (attribute)
* C (class)
* M (comment)

For example:
```javascript
restrict: 'E'
restrict: 'EAC'
```
43 changes: 26 additions & 17 deletions src/ng/compile.js
Original file line number Diff line number Diff line change
Expand Up @@ -1071,6 +1071,17 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
return require;
}

function getDirectiveRestrict(restrict, name) {
if (restrict && !(isString(restrict) && /[EACM]/.test(restrict))) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this to convert all falsey values to the default like previously.

Cases which previously logged errors per-compilation (of an attribute/element with the same name), but otherwise did nothing:

  • non-string truthy values

Cases which previously silently did nothing:

  • arrays without single-character E/A/C/M
  • strings with only invalid characters

Cases that previously worked unintentionally:

  • arrays with single-character entries of E/A/C/M (such as ['E', 'A'] but not ['EA'])

Now all of these cases get logged as errors the first time an attribute/element of that name gets compiled.

throw $compileMinErr('badrestrict',
'Restrict property \'{0}\' of directive \'{1}\' is invalid',
restrict,
name);
}

return restrict || 'EA';
}

/**
* @ngdoc method
* @name $compileProvider#directive
Expand Down Expand Up @@ -1109,7 +1120,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.restrict, name);
directive.$$moduleName = directiveFactory.$$moduleName;
directives.push(directive);
} catch (e) {
Expand Down Expand Up @@ -2905,24 +2916,22 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
if (hasDirectives.hasOwnProperty(name)) {
for (var directive, directives = $injector.get(name + Suffix),
i = 0, ii = directives.length; i < ii; i++) {
try {
directive = directives[i];
if ((isUndefined(maxPriority) || maxPriority > directive.priority) &&
directive.restrict.indexOf(location) !== -1) {
if (startAttrName) {
directive = inherit(directive, {$$start: startAttrName, $$end: endAttrName});
}
if (!directive.$$bindings) {
var bindings = directive.$$bindings =
parseDirectiveBindings(directive, directive.name);
if (isObject(bindings.isolateScope)) {
directive.$$isolateBindings = bindings.isolateScope;
}
directive = directives[i];
if ((isUndefined(maxPriority) || maxPriority > directive.priority) &&
directive.restrict.indexOf(location) !== -1) {
if (startAttrName) {
directive = inherit(directive, {$$start: startAttrName, $$end: endAttrName});
}
if (!directive.$$bindings) {
var bindings = directive.$$bindings =
parseDirectiveBindings(directive, directive.name);
if (isObject(bindings.isolateScope)) {
directive.$$isolateBindings = bindings.isolateScope;
}
tDirectives.push(directive);
match = directive;
}
} catch (e) { $exceptionHandler(e); }
tDirectives.push(directive);
match = directive;
}
}
}
return match;
Expand Down
32 changes: 32 additions & 0 deletions test/ng/compileSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -5869,6 +5869,38 @@ 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('invalidRestrictTrue', valueFn({restrict: true}));
$compileProvider.directive('invalidRestrictObject', valueFn({restrict: {}}));
$compileProvider.directive('invalidRestrictNumber', valueFn({restrict: 42}));

// We need to test with the exceptionHandler not rethrowing...
$exceptionHandlerProvider.mode('log');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can keep the rethrow mode and use the toThrowMinErr helper instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default in tests $exceptionHandler seems to throw. I had to change that to only log so that we can test that it only invokes $exceptionHandler once (on first usage of the directive) and any further use of the directive does not re-log the same error message.

});

inject(function($exceptionHandler, $compile, $rootScope) {
$compile('<div invalid-restrict-true>')($rootScope);
expect($exceptionHandler.errors.length).toBe(1);
expect($exceptionHandler.errors[0]).toMatch(/\$compile.*badrestrict.*'true'/);

$compile('<div invalid-restrict-bad-string>')($rootScope);
$compile('<div invalid-restrict-bad-string>')($rootScope);
expect($exceptionHandler.errors.length).toBe(2);
expect($exceptionHandler.errors[1]).toMatch(/\$compile.*badrestrict.*'"'/);

$compile('<div invalid-restrict-bad-string invalid-restrict-object>')($rootScope);
expect($exceptionHandler.errors.length).toBe(3);
expect($exceptionHandler.errors[2]).toMatch(/\$compile.*badrestrict.*'{}'/);

$compile('<div invalid-restrict-object invalid-restrict-number>')($rootScope);
expect($exceptionHandler.errors.length).toBe(4);
expect($exceptionHandler.errors[3]).toMatch(/\$compile.*badrestrict.*'42'/);
});
});


it('should throw noident when missing controllerAs directive property', function() {
module(function($compileProvider) {
$compileProvider.directive('noIdent', valueFn({
Expand Down