feat(compile): post-t'clusion linking now optional

[Wizek]

Request Type: feature

How to reproduce:

Component(s): $compile

Impact: small

Complexity: small

This issue is related to:

Detailed Description:

Can be controlled by returning an object with {alreadyLinked: true}
from $transclude.

Can you come up with a better name than alreadyLinked?

Related to #7270: While not a complete (automatic) solution to that issue, with this new feature the bug can at least be avoided.

Also, this is meant to be an advanced feature for those who want to do some more ninja-esqe stuff with transclusion. It is not meant to be used regularly or by novices who are new to $compile.

Other Comments:

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#7271)

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!

[Wizek]

Updated my comment with the issue template.

Also, the Travis build failed for a seemingly unrelated reason. Should I/Can I re-run it?

[lgalfaso]

@Wizek before you put any more work on this, it is my understating that there will be no features/refactors done to $compiler in the 1.3 branch. The main reason is that this is already super complex code and nobody wants to spend months trying to fix all possible regressions, uses and abuses of it

[caitp]

well, actually, I do want to spend time fixing bugs in the compiler, that's one of the most fun things working on angular! haha.

[lgalfaso]

@caitp fixing bugs is ok, but I do not think this is a bug

[Wizek]

@lgalfaso While technically this PRQ is a feature, this is the simplest way I could avoid the bug #7270 . So from my perspective this is needed to resolve a bug.

If not this, I am quite sure other attempts at fixing #7270 will be quite a bit more complex. :) Wouldn't you agree?

[Wizek]

@lgalfaso Let me rephrase that: There is currently no way of avoiding the bug described in #7270. Merging this PRQ would at least give the users a chance to avoid it. So, from this perspective this really is a bug-fix.

[lgalfaso]

@Wizek I understand this, but changing this behavior has side-effects. E.g. right now it is possible to do

transclude(function(clone) {
  element.append(clone.content());
  clone.removeData();
});

how does this change affect this?

[lgalfaso]

@Wizek well, the question is if #7270 is really a bug

[caitp]

@caitp fixing bugs is ok, but I do not think this is a bug

It's more this:

The main reason is that this is already super complex code and nobody wants to spend months trying to fix all possible regressions

More possible regressions -> more fun to be had! So long as they don't just get reverted, hah.

I think some variation of this would actually be benefitial to the framework, however I'm not too keen on the current implementation.

Basically if the collection passed to the attach function changes, that collection is what should be linked. But the point about possible regressions is sensible, even though I have fun fixing them, we do want to try to minimize them, and it would be a shame to merge a fix like this and have it reverted =(

[Wizek]

@lgalfaso What kind of attached data are you removing with clone.removeData();? I tried to see if Angular was attaching something useful, but I got undefined: http://jsbin.com/muxux/1/edit

Also, this change doesn't affect what is being passed into the function that you pass into $transclude, so whatever .data() might be attached would be there all the same, no?

[lgalfaso]

@Wizek clone.removeData(); is done so we do not leak memory (will have to look into building a better example) but the idea that in the inner function you can include the content and not the parent (and somehow do so in a way that we do not leak) is now possible

[Wizek]

@caitp Just to brainstorm a little: While usually you'd want to transclude elements as children, it is in theory possible to transclude them into an entirely different place, e.g. as children to body, right? Because if that is the case, those exotic cases are best handled by the implementer, and not trying to guess them in the framework, right?

Another idea I had, you may like this better: What if instead of the boolean the user could return an element which is to be linked further? Allows very similar level of control, a little bit less, though.

@@ -859,8 +859,13 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
           }
         }

-        if (cloneConnectFn) cloneConnectFn($linkNode, scope);
-        if (compositeLinkFn) compositeLinkFn(scope, $linkNode, $linkNode);
+        var overrides = {};
+        if (cloneConnectFn) {
+          angular.extend(overrides, cloneConnectFn($linkNode, scope));
+        }
+        if (compositeLinkFn) {
+          var node = overrides.transcludedElement || $linkNode;
+          compositeLinkFn(scope, node, node);
+        }
         return $linkNode;
       };
     }

[VegaFromLyra]

Came across this issue via codetriage.com. I am not familiar with the transclude functionality so just checking in if this change is still valid given there has not been any activity on it and #7270 for the past few months?

[Wizek]

@VegaFromLyra AFAIK it is still relevant.