Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix($rootScope): remove history event handler when app is torn down #9905

Closed
wants to merge 1 commit into from

Conversation

@randombk
Copy link
Contributor

@randombk randombk commented Nov 4, 2014

Remember the popstate and hashchange handler registered with window
when the application bootstraps, and remove it when the application
is torn down

Closes #9897

@mary-poppins
Copy link

@mary-poppins mary-poppins commented Nov 4, 2014

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.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@randombk randombk force-pushed the randombk:master branch from 615aa89 to 03118f6 Nov 4, 2014
@randombk
Copy link
Contributor Author

@randombk randombk commented Nov 4, 2014

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() {

This comment has been minimized.

@caitp

caitp Nov 4, 2014
Contributor

revert

@@ -282,9 +288,9 @@ function Browser(window, document, $log, $sniffer) {
// changed by push/replaceState

// html5 history api - popstate event
if ($sniffer.history) jqLite(window).on('popstate', cacheStateAndFireUrlChange);
if ($sniffer.history) jqLite(window).on('popstate', self.cacheStateAndFireUrlChange);

This comment has been minimized.

@caitp

caitp Nov 4, 2014
Contributor

revert

// hashchange event
jqLite(window).on('hashchange', cacheStateAndFireUrlChange);
jqLite(window).on('hashchange', self.cacheStateAndFireUrlChange);

This comment has been minimized.

@caitp

caitp Nov 4, 2014
Contributor

revert

// hashchange event
jqLite(window).on('hashchange', cacheStateAndFireUrlChange);
jqLite(window).on('hashchange', self.cacheStateAndFireUrlChange);

urlChangeInit = true;

This comment has been minimized.

@caitp

caitp Nov 4, 2014
Contributor

Add a new method named $$applicationDestroyed or something, which will do the unbinding. Call it from the destroy handler in $rootScope.

@@ -861,7 +861,14 @@ function $RootScopeProvider() {

this.$broadcast('$destroy');
this.$$destroyed = true;
if (this === $rootScope) return;

if (this === $rootScope) {

This comment has been minimized.

@caitp

caitp Nov 4, 2014
Contributor

if (this === $rootScope) $browser.$$applicationDestroyed(); (or whatever you want to call it --- please precede it with 2 dollar signs though)

@randombk randombk force-pushed the randombk:master branch from 03118f6 to 12f81ad Nov 4, 2014
@lgalfaso
Copy link
Member

@lgalfaso lgalfaso commented Nov 4, 2014

this is somehow a duplicate of #8005

@randombk
Copy link
Contributor Author

@randombk randombk commented Nov 4, 2014

You're right - the $shutdown service looks like a cleaner way to do this. Why was work on it stopped?

@caitp
Copy link
Contributor

@caitp caitp commented Nov 4, 2014

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

This comment has been minimized.

@caitp

caitp Nov 4, 2014
Contributor

private API, don't use doc annotations for this --- just a comment explaining what it does

@lgalfaso
Copy link
Member

@lgalfaso lgalfaso commented Nov 4, 2014

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 $timeout, $interval and provides a mechanism for third party modules to perform some tasks when shutting down an angular app

@randombk randombk force-pushed the randombk:master branch from 12f81ad to cb3c27b Nov 4, 2014
@randombk
Copy link
Contributor Author

@randombk randombk commented Nov 4, 2014

@lgalfaso I guess modules can just listen to the $destroy event on $rootScope for that

@randombk
Copy link
Contributor Author

@randombk randombk commented Nov 7, 2014

Is there still an issue with the CLA? I notice that this is still tagged cla:no

@caitp
Copy link
Contributor

@caitp caitp commented Nov 7, 2014

@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...

@caitp
Copy link
Contributor

@caitp caitp commented Nov 7, 2014

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);

This comment has been minimized.

@jbedard

jbedard Dec 1, 2014
Contributor

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 #9897
@randombk randombk force-pushed the randombk:master branch from cb3c27b to 365fd5d Dec 3, 2014
@googlebot
Copy link

@googlebot googlebot commented Dec 3, 2014

CLAs look good, thanks!

@googlebot googlebot removed the cla: no label Dec 3, 2014
@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Mar 9, 2015

I would rather see this event removal closer to where it is added, in $browser. Can't we put it in a $rootScope.$on('$destroy', function() { ... }) instead, which could be created in the $browser.onUrlChange() method?

@Narretz
Copy link
Contributor

@Narretz Narretz commented Mar 12, 2015

@petebacondarwin I can open a new PR with this change. Depends on #11241 though.

@randombk
Copy link
Contributor Author

@randombk randombk commented Mar 12, 2015

@petebacondarwin is $rootScope available from $browser.onUrlChange()? $browser is injected into $rootScope.

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Mar 20, 2015

How about putting the $rootScope.$on('$destroy', function() { ... }) inside the $location, which is where $browser.onUrlChange() is called?

For example, if onUrlChange returned a method that could be called to unbind itself then we could easily do that with little coupling there.

@randombk
Copy link
Contributor Author

@randombk randombk commented Mar 20, 2015

I can move the removal method call to where you suggest, but I think we should still keep $browser.$$applicationDestroyed(). $browser.onUrlChange() currently returns the registered handler, which makes sense. I know this is an internal api, but making it return a function that removes a different handler is fairly non-intuitive.

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Apr 2, 2015

Added some tests and merged. Thanks

netman92 added a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants