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

Commit

Permalink
fix($compile): throw error on invalid directive name
Browse files Browse the repository at this point in the history
Directive names must start with a lower case letter.
Previously the compiler would quietly fail.
This change adds an assertion that fails if this is not the case.

Closes #11281
Closes #11109
  • Loading branch information
wesleycho authored and petebacondarwin committed Mar 17, 2015
1 parent e57138d commit 170ff9a
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 0 deletions.
8 changes: 8 additions & 0 deletions docs/content/error/$compile/baddir.ngdoc
@@ -0,0 +1,8 @@
@ngdoc error
@name $compile:baddir
@fullName Invalid Directive Name
@description

This error occurs when the name of a directive is not valid.

Directives must start with a lowercase character.
9 changes: 9 additions & 0 deletions src/ng/compile.js
Expand Up @@ -790,6 +790,14 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
return bindings;
}

function assertValidDirectiveName(name) {
var letter = name.charAt(0);
if (!letter || letter !== lowercase(letter)) {
throw $compileMinErr('baddir', "Directive name '{0}' is invalid. The first character must be a lowercase letter", name);
}
return name;
}

/**
* @ngdoc method
* @name $compileProvider#directive
Expand All @@ -808,6 +816,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
this.directive = function registerDirective(name, directiveFactory) {
assertNotHasOwnProperty(name, 'directive');
if (isString(name)) {
assertValidDirectiveName(name);
assertArg(directiveFactory, 'directiveFactory');
if (!hasDirectives.hasOwnProperty(name)) {
hasDirectives[name] = [];
Expand Down
10 changes: 10 additions & 0 deletions test/ng/compileSpec.js
Expand Up @@ -147,6 +147,7 @@ describe('$compile', function() {


describe('configuration', function() {

it('should register a directive', function() {
module(function() {
directive('div', function(log) {
Expand Down Expand Up @@ -201,6 +202,15 @@ describe('$compile', function() {
});
inject(function($compile) {});
});

it('should throw an exception if a directive name starts with a non-lowercase letter', function() {
module(function() {
expect(function() {
directive('BadDirectiveName', function() { });
}).toThrowMinErr('$compile','baddir', "Directive name 'BadDirectiveName' is invalid. The first character must be a lowercase letter");
});
inject(function($compile) {});
});
});


Expand Down

2 comments on commit 170ff9a

@jtorbicki
Copy link
Contributor

Choose a reason for hiding this comment

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

@wesleycho The real name of the function assertValidDirectiveName should be assertFirstLetterLowercase. I'm not trying to troll here :) Just to maybe start some discussion whether the directive names should be validated or not. I've mention that a bit in this issue: #11397
Currently you could name a directive any of those example chars: < > " | (or single space). Would that be valid directive name? assertValidDirectiveName would say yes :) But then how would you use this directive in a html?

@petebacondarwin
Copy link
Member

Choose a reason for hiding this comment

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

@jtorbicki - it is funny that you say this because it was indeed called something like that in @wesleycho's original PR: #11281 (diff)

But the point is that we should probably make this method more strict as you suggest rather than rename it.

Please sign in to comment.