This repository was archived by the owner on Apr 12, 2024. It is now read-only.
fix(ngTransclude): correctly support fallback content #14775
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If the transclude function did not return content, then the transcluded scope that was created needs to be cleaned up in order to avoid a slight amount of unnecessary overhead since the additional scope is no longer needed. If the transcluded content did return content, the the fallback content should never have been linked in the first place as it was intended to be immediately removed. Fixes angular#14768 Fixes angular#14765
dcherman
force-pushed
the
ngTranscludeFix
branch
from
9b4db0c to
facad29
Compare
dcherman
changed the title
| // Since this is the fallback content rather than the transcluded content, | ||
| // we compile against the scope we were linked against rather than the transcluded | ||
| // scope since this is the directive's own content | ||
| $compile($element.contents())($scope); |
There was a problem hiding this comment.
Should probably check to see if any contents exist prior to calling $compile.
There was a problem hiding this comment.
This is not semantically necessary as the compile bails out if there is nothing to compile.
There was a problem hiding this comment.
although it could improve performance slightly
|
Could you please create new tests for this instead of modifying the old ones? |
| $element.append(clone); | ||
| // There is nothing linked against the transcluded scope since no content was available, | ||
| // so it should be safe to clean up the generated scope. | ||
| transcludedScope.$destroy(); |
There was a problem hiding this comment.
So, this is essentially fixing a leak, right ? Cool
|
I moved the tests into their own specs. |
petebacondarwin
pushed a commit
that referenced
this pull request
Jun 17, 2016
If the instance of the directive does provide transcluded content, then the fallback content should not be compiled and linked as it will never be used. If the instance of the directive does not provide transcluded content, then the transcluded scope that was created for this non-existent content is never used, so it should be destroy in order to clean up unwanted memory use and digests. Fixes #14768 Fixes #14765 Closes #14775
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the transclude function did not return content, then the transcluded
scope that was created needs to be cleaned up in order to avoid a slight
amount of unnecessary overhead since the additional scope is no longer
needed.
If the transcluded content did return content, the the fallback content
should never have been linked in the first place as it was intended to be
immediately removed.
Fixes #14768
Fixes #14765