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

ngMessages leak #16406

Merged
merged 1 commit into from
Jan 17, 2018
Merged

ngMessages leak #16406

merged 1 commit into from
Jan 17, 2018

Conversation

petebacondarwin
Copy link
Contributor

@petebacondarwin petebacondarwin commented Jan 12, 2018 •
edited
Loading

@petebacondarwin petebacondarwin force-pushed the ng-messages-mem-leak-16389 branch from b1896f7 to 48f384f Compare January 12, 2018 17:05
@petebacondarwin petebacondarwin mentioned this pull request Jan 12, 2018
Closed
@petebacondarwin petebacondarwin modified the milestones: 1.6.x, 1.6.9 Jan 12, 2018
Narretz
Narretz suggested changes Jan 13, 2018
View reviewed changes
Copy link
Contributor

@Narretz Narretz left a comment

Choose a reason for hiding this comment

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

Looks good! Always nice when a fix doesn't involve adding more code.

src/ngMessages/messages.js Outdated
@@ -719,6 +715,10 @@ function ngMessageDirectiveFactory() {
}
}
});

scope.$on('$destroy', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a comment here why this extra listener is necessary.
Btw, is this the new scope that belongs only to the message? Or the parent scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a comment explaining this.

This directive (and the ngMessages directive) do not create new scopes. So the scope we are listening to is the one that contains this ngMessage directive. E.g. the scope created by an ngIf or ngRepeat directive that wraps this ngMessage.

test/ngMessages/messagesSpec.js
'<div ng-if="show"><div ng-message="x">ERROR</div></div>' +
'</div>';

element = $compile(html)($rootScope);
Copy link
Contributor

Choose a reason for hiding this comment

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

There could be an expectation here that checks that the messge isn't initially attached / in the DOM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@Narretz Narretz mentioned this pull request Jan 15, 2018
Open
@petebacondarwin petebacondarwin force-pushed the ng-messages-mem-leak-16389 branch from 48f384f to cc0bdf1 Compare January 17, 2018 10:24
@petebacondarwin petebacondarwin force-pushed the ng-messages-mem-leak-16389 branch from cc0bdf1 to b4ecb72 Compare January 17, 2018 10:33
@Narretz
Copy link
Contributor

Narretz commented Jan 17, 2018

LGTM. I will merge once Travis has recovered - the builds are currently delayed because of an issue at their end

@Narretz Narretz merged commit 50ceb23 into angular:master Jan 17, 2018
Narretz pushed a commit to Narretz/angular.js that referenced this pull request Jan 17, 2018
@petebacondarwin @Narretz
fix(ngMessages): prevent memory leak from messages that are never att…
a079e34 
…ached

Closes angular#16389
Closes angular#16404
Closes angular#16406
Narretz pushed a commit that referenced this pull request Jan 17, 2018
@petebacondarwin @Narretz
fix(ngMessages): prevent memory leak from messages that are never att…
9d058de 
…ached

Closes #16389
Closes #16404
Closes #16406
@dependencies dependencies bot mentioned this pull request Feb 2, 2018
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Narretz Narretz Narretz approved these changes

Assignees
No one assigned
Labels
cla: yes component: ngMessages needs: review severity: memory leak type: bug
Projects
None yet
Milestone
1.6.9
Development

Successfully merging this pull request may close these issues.
3 participants
@petebacondarwin @Narretz @googlebot