-
Notifications
You must be signed in to change notification settings - Fork 27.5k
fix($rootScope): remove history event handler when app is torn down #9905
Conversation
I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS. Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match. If you signed the CLA as a corporation, please let us know the company's name. Thanks a bunch! PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR. |
Fixed. |
* Note: this method is used only by $rootScope to remove the handler when an app | ||
* is torn down. (See https://github.com/angular/angular.js/issues/9897) | ||
*/ | ||
self.cacheStateAndFireUrlChange = function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert
this is somehow a duplicate of #8005 |
You're right - the $shutdown service looks like a cleaner way to do this. Why was work on it stopped? |
it's not really much cleaner, it's much harder to land something like $shutdown because it's way more complicated. This patch is pretty simple, it's a good way to go. |
@@ -294,6 +294,19 @@ function Browser(window, document, $log, $sniffer) { | |||
}; | |||
|
|||
/** | |||
* @name $browser#$$applicationDestroyed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private API, don't use doc annotations for this --- just a comment explaining what it does
I agree with @caitp that the shutdown idea is more complex than this patch (with all that this implies). The only thing in favor is that it takes care of cleaning up |
@lgalfaso I guess modules can just listen to the $destroy event on $rootScope for that |
Is there still an issue with the CLA? I notice that this is still tagged cla:no |
@randombk the robot is having some issues with auto CLA updates lately :( I thought it was supposed to be resolved by now, but I guess not... |
I don't think we're going to get this in for this release, but it seems like a reasonable approach to me, so we can probably do this next time. I'm half expecting to hear complaints from people that their app no longer "works" because of this, though :( |
* NOTE: this api is intended for use only by $rootScope. | ||
*/ | ||
self.$$applicationDestroyed = function() { | ||
jqLite(window).unbind('popstate', cacheStateAndFireUrlChange); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
off
should probably be used instead of the legacy unbind
. Could also make it one line doing jqLite(window).off('popstate hashchange', cacheStateAndFireUrlChange);
Remember the popstate and hashchange handler registered with window when the application bootstraps, and remove it when the application is torn down Closes angular#9897
CLAs look good, thanks! |
I would rather see this event removal closer to where it is added, in |
@petebacondarwin I can open a new PR with this change. Depends on #11241 though. |
@petebacondarwin is $rootScope available from $browser.onUrlChange()? $browser is injected into $rootScope. |
How about putting the For example, if |
I can move the removal method call to where you suggest, but I think we should still keep |
Added some tests and merged. Thanks |
Remember the popstate and hashchange handler registered with window when the application bootstraps, and remove it when the application is torn down Closes angular#9897 Closes angular#9905
Remember the popstate and hashchange handler registered with window
when the application bootstraps, and remove it when the application
is torn down
Closes #9897