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

fix($compile): Detect when a directive compile removes tElement #9202

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix($compile): Detect when a directive compile removes tElement #9202

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
20 changes: 18 additions & 2 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1067,10 +1067,21 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
function compileNodes(nodeList, transcludeFn, $rootElement, maxPriority, ignoreDirective,
previousCompileContext) {
var linkFns = [],
attrs, directives, nodeLinkFn, childNodes, childLinkFn, linkFnFound, nodeLinkFnFound;
attrs, directives, nodeLinkFn, childNodes, childLinkFn, linkFnFound, nodeLinkFnFound,
i, canary = '' + Math.random();

// While doing the main loop, there is the possibility that the element that we just compiled
// is removed from `nodeList` and not just replaced with a comment or another element.
// To detect this, we put a canary on all elements and remove it once the element is processed
Copy link
Contributor

Choose a reason for hiding this comment

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

you remove it before the element is processed and after the element is processed you check if the current node doesn't have canary flag any more. no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

// https://github.com/angular/angular.js/issues/9198
for (i = 0; i < nodeList.length; ++i) {
nodeList[i].ngCanary = canary;
}

for (var i = 0; i < nodeList.length; i++) {
i = 0;
while (i < nodeList.length) {
attrs = new Attributes();
nodeList[i].ngCanary = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is better --- We could still shrink this a bit, though, by adding if (i + 1 < nodeList.length) nodeList[i+1].ngCanary = canary;

This would let us remove the initial loop. It doesn't make a huge difference though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If nodeList[i] has a directive with -begin, then just marking the next node would not make it


// we must always refer to nodeList[i] since the nodes can be replaced underneath us.
directives = collectDirectives(nodeList[i], [], attrs, i === 0 ? maxPriority : undefined,
Expand Down Expand Up @@ -1102,6 +1113,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {

//use the previous context only for the first element in the virtual group
previousCompileContext = null;

// nodeList is a *live* list of elements, so there is a chance that the node we just compile
// was removed from nodeList (not just replaced with something else), and the node at
// nodeList[i] is a node that was not processed
if (nodeList[i].ngCanary != canary) i++;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this change because it makes the code much harder to follow and by monkey-patching DOM node object, we are increasing memory consumption (v8 doesn't like when these objects get monkey-patched).

could we just check if the element at the current index changed after we applied directives to the current node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checking if the element changed would make the code even harder to understand as there are many cases we have to handle

1/ The element is the same (simple)
2/ The element was replaced by a comment (transclude: element)
3/ Multiple elements were replaced by a single comment (transclude: element + multiElement)
4/ A single element was removed (during compile, the directive did tElement.remove())
5/ Multiple elements were removed (transclude: element + multiElement + tElement.remove())

It is possible to do all this without adding a canary, but the code will be slightly more complex

}

// return a linking function if we have found anything, null otherwise
Expand Down
21 changes: 21 additions & 0 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6337,6 +6337,27 @@ describe('$compile', function() {
});
});

it('should collect all directives even when a directive compilation removes a node', function() {
module(function($compileProvider) {
$compileProvider.directive('foo', function() {
return {
restrict: 'A',
compile: function(tElement){
tElement.detach();
},
multiElement: true
};
});
});
inject(function($compile, $rootScope) {
element = $compile('<div><div foo=""></div>{{"Hello world"}}</div>')($rootScope);
$rootScope.$digest();
expect(element.text()).toBe('Hello world');
element = $compile('<div><div foo-start=""></div><div foo-end></div>{{"Hello world"}}</div>')($rootScope);
$rootScope.$digest();
expect(element.text()).toBe('Hello world');
});
});

it('should throw error if unterminated (containing termination as a child)', function () {
module(function($compileProvider) {
Expand Down