Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngTransclude): 'cos things are never simple #14787

Closed
wants to merge 1 commit into from
Closed

fix(ngTransclude): 'cos things are never simple #14787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 45 additions & 30 deletions src/ng/directive/ngTransclude.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,41 +163,56 @@ var ngTranscludeDirective = ['$compile', function($compile) {
return {
restrict: 'EAC',
terminal: true,
link: function($scope, $element, $attrs, controller, $transclude) {
if ($attrs.ngTransclude === $attrs.$attr.ngTransclude) {
// If the attribute is of the form: `ng-transclude="ng-transclude"`
// then treat it like the default
$attrs.ngTransclude = '';
}
compile: function ngTranscludeCompile(tElement) {

// Remove and cache any original content to act as a fallback
var fallbackLinkFn = $compile(tElement.contents());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How common do you feel it is to use fallback content and have the user not provide their own? Wondering if it's worth calling $compile the first time the fallback content is needed rather than at compile time, but I have no idea how common this usage is to determine whether or not that'd be worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is faster than the situation we had before before your PR, since then we were compiling AND linking the fallback content, even if it was never used.
We could optimize to lazy compile the content, but I would like to see some numbers that show it makes any real difference before doing so.

tElement.empty();

return function ngTranscludePostLink($scope, $element, $attrs, controller, $transclude) {

if (!$transclude) {
throw ngTranscludeMinErr('orphan',
'Illegal use of ngTransclude directive in the template! ' +
'No parent directive that requires a transclusion found. ' +
'Element: {0}',
startingTag($element));
}

function ngTranscludeCloneAttachFn(clone, transcludedScope) {
if (clone.length) {
$element.empty();
$element.append(clone);
} else {
// 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 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();
// If the attribute is of the form: `ng-transclude="ng-transclude"` then treat it like the default
if ($attrs.ngTransclude === $attrs.$attr.ngTransclude) {
$attrs.ngTransclude = '';
}
}
var slotName = $attrs.ngTransclude || $attrs.ngTranscludeSlot;

if (!$transclude) {
throw ngTranscludeMinErr('orphan',
'Illegal use of ngTransclude directive in the template! ' +
'No parent directive that requires a transclusion found. ' +
'Element: {0}',
startingTag($element));
}
// If the slot is required and no transclusion content is provided then this call will throw an error
$transclude(ngTranscludeCloneAttachFn, null, slotName);

// If there is no slot name defined or the slot name is not optional
// then transclude the slot
var slotName = $attrs.ngTransclude || $attrs.ngTranscludeSlot;
$transclude(ngTranscludeCloneAttachFn, null, slotName);
// If the slot is optional and no transclusion content is provided then use the fallback content
if (slotName && !$transclude.isSlotFilled(slotName)) {
useFallbackContent();
}

function ngTranscludeCloneAttachFn(clone, transcludedScope) {
if (clone.length) {
$element.append(clone);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also release any fallback contents for GC (e.g. fallbackContents = null) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is more complicated. We need to compile this content into a linking function, which we will use to link and clone the fallback content, in the case that this appears inside an ng-repeat.

} else {
useFallbackContent();
// 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();
}
}

function useFallbackContent() {
// Since this is the fallback content rather than the transcluded content,
// we link against the scope of this directive rather than the transcluded scope
fallbackLinkFn($scope, function(clone) {
$element.append(clone);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this mean, that if the fallback content requires a parent controller, it won't be able to find it (since we append it to the DOM after it has been linked)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a suitable unit test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The append is being done in the cloneAttachFn which occurs prior to link, so it should be able to find any required controllers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh!

});
}
};
}
};
}];
129 changes: 87 additions & 42 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7970,17 +7970,52 @@ describe('$compile', function() {
});


it('should not compile the fallback content if transcluded content is provided', function() {
var contentsDidLink = false;
it('should clear the fallback content from the element during compile and before linking', function() {
module(function() {
directive('trans', function() {
return {
transclude: true,
template: '<div ng-transclude>fallback content</div>'
};
});
});
inject(function(log, $rootScope, $compile) {
element = jqLite('<div trans></div>');
var linkfn = $compile(element);
expect(element.html()).toEqual('<div ng-transclude=""></div>');
linkfn($rootScope);
$rootScope.$apply();
expect(sortedHtml(element.html())).toEqual('<div ng-transclude="">fallback content</div>');
});
});


it('should allow cloning of the fallback via ngRepeat', function() {
module(function() {
directive('trans', function() {
return {
transclude: true,
template: '<div ng-repeat="i in [0,1,2]"><div ng-transclude>{{i}}</div></div>'
};
});
});
inject(function(log, $rootScope, $compile) {
element = $compile('<div trans></div>')($rootScope);
$rootScope.$apply();
expect(element.text()).toEqual('012');
});
});


it('should not link the fallback content if transcluded content is provided', function() {
var linkSpy = jasmine.createSpy('postlink');

module(function() {
directive('inner', function() {
return {
restrict: 'E',
template: 'old stuff! ',
link: function() {
contentsDidLink = true;
}
link: linkSpy
};
});

Expand All @@ -7995,21 +8030,19 @@ describe('$compile', function() {
element = $compile('<div trans>unicorn!</div>')($rootScope);
$rootScope.$apply();
expect(sortedHtml(element.html())).toEqual('<div ng-transclude="">unicorn!</div>');
expect(contentsDidLink).toBe(false);
expect(linkSpy).not.toHaveBeenCalled();
});
});

it('should compile and link the fallback content if no transcluded content is provided', function() {
var contentsDidLink = false;
var linkSpy = jasmine.createSpy('postlink');

module(function() {
directive('inner', function() {
return {
restrict: 'E',
template: 'old stuff! ',
link: function() {
contentsDidLink = true;
}
link: linkSpy
};
});

Expand All @@ -8024,7 +8057,50 @@ describe('$compile', function() {
element = $compile('<div trans></div>')($rootScope);
$rootScope.$apply();
expect(sortedHtml(element.html())).toEqual('<div ng-transclude=""><inner>old stuff! </inner></div>');
expect(contentsDidLink).toBe(true);
expect(linkSpy).toHaveBeenCalled();
});
});

it('should compile and link the fallback content if an optional transclusion slot is not provided', function() {
var linkSpy = jasmine.createSpy('postlink');

module(function() {
directive('inner', function() {
return {
restrict: 'E',
template: 'old stuff! ',
link: linkSpy
};
});

directive('trans', function() {
return {
transclude: { optionalSlot: '?optional'},
template: '<div ng-transclude="optionalSlot"><inner></inner></div>'
};
});
});
inject(function(log, $rootScope, $compile) {
element = $compile('<div trans></div>')($rootScope);
$rootScope.$apply();
expect(sortedHtml(element.html())).toEqual('<div ng-transclude="optionalSlot"><inner>old stuff! </inner></div>');
expect(linkSpy).toHaveBeenCalled();
});
});

it('should cope if there is neither transcluded content nor fallback content', function() {
module(function() {
directive('trans', function() {
return {
transclude: true,
template: '<div ng-transclude></div>'
};
});
});
inject(function($rootScope, $compile) {
element = $compile('<div trans></div>')($rootScope);
$rootScope.$apply();
expect(sortedHtml(element.html())).toEqual('<div ng-transclude=""></div>');
});
});

Expand Down Expand Up @@ -9824,37 +9900,6 @@ describe('$compile', function() {
expect(element.children().eq(2).text()).toEqual('dorothy');
});
});

it('should not overwrite the contents of an `ng-transclude` element, if the matching optional slot is not filled', function() {
module(function() {
directive('minionComponent', function() {
return {
restrict: 'E',
scope: {},
transclude: {
minionSlot: 'minion',
bossSlot: '?boss'
},
template:
'<div class="boss" ng-transclude="bossSlot">default boss content</div>' +
'<div class="minion" ng-transclude="minionSlot">default minion content</div>' +
'<div class="other" ng-transclude>default content</div>'
};
});
});
inject(function($rootScope, $compile) {
element = $compile(
'<minion-component>' +
'<minion>stuart</minion>' +
'<span>dorothy</span>' +
'<minion>kevin</minion>' +
'</minion-component>')($rootScope);
$rootScope.$apply();
expect(element.children().eq(0).text()).toEqual('default boss content');
expect(element.children().eq(1).text()).toEqual('stuartkevin');
expect(element.children().eq(2).text()).toEqual('dorothy');
});
});
});


Expand Down