fix($compile): allow multiple directives on a single element even if one works with a multi-element range #4160

jankuca · 2013-09-25T22:36:42Z

mary-poppins · 2013-09-25T22:36:44Z

Thanks for the PR!

Contributor signed CLA now or in the past
- If you just signed, leave a comment here with your real name
PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

jeffbcross · 2013-09-26T21:23:55Z

I added an inline comment to remove an iit, but could you also write a shorter first line of the commit message and a more descriptive commit message describing the behavior that was being observed before this fix?

jeffbcross · 2013-09-26T21:51:41Z

CI is happy: http://ci.angularjs.org/job/angular.js-jeff/65/console

… element The problem was in keeping the values of `attrNameStart` and `attrNameEnd` between directive loop iterations which lead to the compiler looking for multi-element ranges for any directives that happened to be in the directive list after one that was applied on a range. For instance, having a ng-repeat-start and ng-class on a single element with ng-repeat being resolved first made the compiler look for an ng-repeat-end for both ng-repeat and ng-class because the `attrNameEnd` was not reset to a falsy value before the second iteration. As the result, an exception saying the block end element could not be found and the second directive was not actually applied. Closes angular#4002

rodyhaddad · 2013-09-26T22:30:17Z

src/ng/compile.js

-              if ((index = ngAttrName.lastIndexOf('Start')) != -1 && index == ngAttrName.length - 5) {
+
+              var directiveNName = ngAttrName.replace(/(Start|End)$/, '');
+              if (ngAttrName === directiveNName + 'Start') {


I get that attr{Start,End}Name should be set back to false, but why change this condition and involve a regex?
What's wrong with using lastIndexOf here?

If this way is the way to go, then remove the index variable (L626) since it's no longer used, and also cache the regex

IgorMinar · 2013-09-27T19:52:17Z

landed as 6a8edc1

rodyhaddad reviewed Sep 26, 2013
View reviewed changes

IgorMinar closed this Sep 27, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($compile): allow multiple directives on a single element even if one works with a multi-element range #4160

fix($compile): allow multiple directives on a single element even if one works with a multi-element range #4160

jankuca commented Sep 25, 2013

mary-poppins commented Sep 25, 2013

jeffbcross commented Sep 26, 2013

jeffbcross commented Sep 26, 2013

rodyhaddad Sep 26, 2013

IgorMinar commented Sep 27, 2013

fix($compile): allow multiple directives on a single element even if one works with a multi-element range #4160

fix($compile): allow multiple directives on a single element even if one works with a multi-element range #4160

Conversation

jankuca commented Sep 25, 2013

mary-poppins commented Sep 25, 2013

jeffbcross commented Sep 26, 2013

jeffbcross commented Sep 26, 2013

rodyhaddad Sep 26, 2013

Choose a reason for hiding this comment

IgorMinar commented Sep 27, 2013