feat($compile): support multi-elements in all declaration styles

[rodyhaddad]

I've built on the work of #2783 to make multi-elements possible in all
directive decralation styles.

Basically #2783 was limited to directives declared via attributes.
With this commit, appending X-start and X-end would work
in Comments, Attributes, ClassNames and ElementNames

<table>
  <!-- directive: ng-repeat-start item in list -->
    <tr>If ngRepeat's 'restrict' get changed to include 'M'</tr>
    <tr>I get repeated</tr>
  <!-- ng-repeat-end -->

  <!-- the 'directive:' part for X-end is optional -->
</table>

I will be adding tests for the compileSpec very soon, just wanted to know if it looks good for you guys.

After I write the tests, I also want to add restrict: 'CMA' for the following directives:

• ng-repeat
• ng-show/ng-hide
• ng-if

[petebacondarwin]

@rodyhaddad - Thanks for this PR. I like that you have obviously read the commit guidelines and you have even added in the new error message docs.

Do you have a specific use case for this feature, which cannot be achieved with attributes?

I seem to remember there was some reason for only allowing attribute directives. I know that comment-based directives are inherently difficult and @IgorMinar spent a long time fighting with Internet Explorer to get a comment based repeater working to no avail. So I suspect that this is not a goer.

If you do want to push this PR forward then make sure that your tests exercise the full functionality correctly on Internet Explorer 8.

[petebacondarwin]

PR Checklist (Minor Feature)

• Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
• Feature improves existing core functionality
• API is compatible with existing Angular apis and relevant standards (if applicable)
• PR doesn't contain a breaking change
• PR contains unit tests
• PR contains e2e tests (if suitable)
• PR contains documentation update
• PR passes all tests on Travis (sanity)
• PR passes all tests on ci.angularjs.org (cross-browser compatibility)
• PR is rebased against recent master
• PR is squashed into one commit per logical change
• PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
• All changes requested in review have been implemented

[petebacondarwin]

Can you make sure you have signed the CLA too. Thanks

[rodyhaddad]

I've signed the CLA (Rodric Haddad)

---

Do you have a specific use case for this feature, which cannot be achieved with attributes?

Everything can be achieved with just attributes.
The only exception that I can think of is when trying to apply a directive on text (to repeat the text for example) -> comments comes in handy
All I did is make X-start and X-end work with all 4 forms of directive declaration. Just to keep things consistent

Also, I find this:

<table>
  <!-- directive: ng-repeat-start item in list -->
  <tr>
    <td>I get repeated</td>
  </tr>
  <tr>
    <td>I also get repeated</td>
  </tr>
  <!-- ng-repeat-end -->
</table>

A lot more elegant that this:

<table>
  <tr ng-repeat-start="item in list">
    <td>I get repeated</td>
  </tr>
  <tr ng-repeat-end>
    <td>I also get repeated</td>
  </tr>
</table>

The last <tr> could have a lot of nested children. You would have to scroll way up to see the ng-repeat-end. It doesn't feel very natural

---

I seem to remember there was some reason for only allowing attribute directives. I know that comment-based directives are inherently difficult and @IgorMinar spent a long time fighting with Internet Explorer to get a comment based repeater working to no avail. So I suspect that this is not a goer.

The only PRs I found that address this similar issue are the following

• comment-based repeater #1646 : I'm assuming this is the one you're talking about (closed)
  • The only issue with IE that I could find is that it doesn't allow comments within the select element (attributes can be used on options instead, I don't think it's a big deal)
  • The grouping logic is written in ngRepeat's code, making it non-generic.
  • Other directives who want the same feature need to copy the logic from ngRepeat (or roll out their own)
  • It only works with comments
• Transclude type multi-element, use this for ng-repeat #2531 : superseded by feat($compile): support directive virtual groups #2783 (closed)
  • It adds a new transclude: 'multi-element'
  • directives like ng-show that don't need to transclude won't work that easily
  • It only works with attributes
• feat($compile): support directive virtual groups #2783 : the only one that actually got merged
  • It adds generic X-start and X-end grouping, handled in $compile
  • It only works with attributes
  • It pretty much just makes that the element given to a directive may be a collection of DOM nodes
    • This means that if someone is just doing a $element.on(event, ...) or something similar, no changes need to be made to his/her directive 👍

---

As a side-note, Support for comment-based ngRepeat is in the 1.0 -> 1.2 roadmap, I don't know if this document is still considered valid

[rodyhaddad]

I have just finished adding tests for my changes, and everything seems to pass in IE8

[IgorMinar]

the comment based stuff doesn't work in certain elements in IE (e.g. inside select element).

I appreciate all the work you put into this, but I think that it unnecessarily complicates things. Sometimes giving people more options to do the same thing is not beneficial.

I'm going to close this PR for now. If you have a use-case that can't be implemented using the current attribute-based multi-element directive please post it here and reopen the PR.

thanks again!

[rodyhaddad]

This PR doesn't complicate things, it makes things more consistent.

Angular supports directives declared in 4 styles (EACM), but now, it only supports multi-node directives declared in 1 style (just attributes), which doesn't make much sense imho.

As a developer, I would expect *-start and *-end to work in classNames, comments and elementNames ( why wouldn't they? ). Specially since this limitation doesn't seem to be mentioned in the docs.

I can't think of a use-case where multi-node directives declared as comments/classNames/elementsNames can do the job and attributes couldn't, but comment-based repeaters are a lot nicer to work with:

<div class="row header">
   ...
</div>

<div ng-repeat-start="item in list" class="row">
  <div class="span3">
    {{ item }}
  </div>
  <div class="span9">
    {{ item }}
  </div>
</div>
<div ng-repeat-end class="row">
  <div class="span9">
    {{ item }}
  </div>
  <div class="span3">
    {{ item }}
  </div>
</div>

<div class="row footer">
   ...
</div>

Having the ng-repeat-end in the middle of the loop doesn't seem natural.
VS:

<div class="row header">
   ...
</div>

<!-- directive: ng-repeat-start item in list -->
<div class="row">
  <div class="span3">
    {{ item }}
  </div>
  <div class="span9">
    {{ item }}
  </div>
</div>
<div class="row">
  <div class="span9">
    {{ item }}
  </div>
  <div class="span3">
    {{ item }}
  </div>
</div>
<!-- ng-repeat-end -->

<div class="row footer">
   ...
</div>

I don't know about you, but the second one seems a lot easier to read.

---

The no comments in select elements in IE applies to all comment based directives, not just to multi-node directives. This problem has always existed and should be added to the Internet Explorer Compatibility page.

This PR doesn't change anything with that, it just makes comment based directives more useful :)
And as you said :

In practice this typically is not an issue because repeating over more than one element within a select element doesn't make a whole lot of sense.

The other issue with comment based multi-node directives is using them in conjunction with omitted tags (e.g. <tbody>)
This throws a $compile:uterdir Unterminated Directive error.
With the new error handling system, we'd just have to add a section to that error page, saying that you shouldn't use multi-node comment based directive, if you're also omitting tags. Wouldn't that be enough?

---

I still think this would be useful to many.
Even though it doesn't add new features (actually it does, just not for directives include in the ng module), it still makes writing html a lot more cleaner and nicer to read.
I don't know if the points I've made are good enough to convince you that it's nice to have.
My implementation works, and I've got the tests to prove it

...I don't see a button to open back the PR. If you want me to add to the docs the things I've talked about above, and then merge, you can open up the PR again for me to add new docs() commits.