WIP fix($compile): don't instantiate controllers twice for element transclud... #4654
Conversation
|
Thanks for the PR!
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
|
still needs tests... |
| templateDirective: templateDirective, | ||
| // Don't pass in: | ||
| // - controllerDirectives - otherwise we'll create duplicates controllers | ||
| // - newIsolateScopeDirective - combining templates with element transclusion |
|
Still didn't look into it properly, but here are some quick comments:
A few weeks ago, I changed the "templateUrl + replace" use case to only have one linkin function (nodeLinkFn). In theory I think we should do the same for "transclude element". At this point we have two - linking of all the directives "before" the transluding directive (all directives with higher priority) and then the rest on the translcluded element. So I don't think this is the proper fix. However I say LGTM, because it's better to "forget to complain about multiple directives with isolate scope" that the current behavior (instantiating multiple controllers). I need more brain power to suggest something better. |
|
@IgorMinar Let's talk about this in person. Monday? |
|
I don't think that there can be a single linking fn because with element transclusion we always deal with at least two nodes 1/ the original node that contained the element transclusion directive. this node got destroyed and was replaced with the comment so because of this, I believe that it's correct to have two linking fns. also because of how this all works, it doesn't make sense to to use template or template with e;lement transctiolsion directive. |
…lude directives This is a fix for regression introduced last week by faf5b98. Closes angular#4654
…lude directives This is a fix for regression introduced last week by faf5b98. Closes angular#4654
...e directives