fix(virtualRepeat): Memory leak #8056
Conversation
Delete all created elements when the scope is destroyed. Closes angular#8055
|
@jelbourn Please take a look. |
| @@ -501,6 +501,12 @@ function VirtualRepeatController($scope, $element, $attrs, $browser, $document, | |||
| this.blocks = {}; | |||
| /** @type {Array<!VirtualRepeatController.Block>} A pool of presently unused blocks. */ | |||
| this.pooledBlocks = []; | |||
|
|
|||
| $scope.$on('$destroy', angular.bind(this, function cleanup() { | |||
There was a problem hiding this comment.
Can you provide a unit test to validate that this works?
There was a problem hiding this comment.
Definitely needs a unit test to make ensure that this doesn't regress in the future.
ThomasBurleson
added
the
needs: unit tests
|
👍 |
jelbourn
added
the
g3: pr
| angular.forEach(this.blocks, removeBlock); | ||
| angular.forEach(this.pooledBlocks, removeBlock); | ||
| })); | ||
| function removeBlock(block) { block.element.remove(); } |
There was a problem hiding this comment.
element.remove() unfortunately does not work in IE11. It needs to be
if (element.parentNode) {
element.parentNode.removeChild(element);
}
There was a problem hiding this comment.
I would also make removeBlock a method on the controller's prototype.
There was a problem hiding this comment.
element.remove()unfortunately does not work in IE11.
Really? Please note that this is jqLite.remove().
There was a problem hiding this comment.
How about making cleanup() a method on the controller's prototype?
There was a problem hiding this comment.
Ah, I didn't realize it was a wrapped element. remove is fine, then.
cleanupBlocks() ? Just cleanup by itself isn't descriptive.
|
cc @kseamon |
Move cleanup code to cleanupBlocks(). Add unit tests. Closes angular#8055
Minor correction. Closes angular#8055
|
Please t |
|
Please take a look. |
| block.element.remove(); | ||
| } | ||
|
|
||
| angular.forEach(this.blocks, cleanupBlock); |
There was a problem hiding this comment.
This one is probably not needed (they should be removed along with the parent element). Doesn't hurt much to leave it in, though.
There was a problem hiding this comment.
@kseamon You are correct. Removed.
|
LGTM, overall |
|
LGTM |
ThomasBurleson
added
pr: merge ready
|
Thank you. |
Do not clean up used blocks. Closes angular#8055
|
👍 |
|
Thank you. \o/ |
Delete all created elements when the scope is destroyed.
Closes #8055