Skip to content

Commit 38524c4

Browse files
arturovtAndrewKushnir
arturovt
authored and
AndrewKushnir
committed
fix(common): cleanup location change listeners when the root view is removed (angular#40867)
In the new behavior Angular cleanups `popstate` and `hashchange` event listeners when the root view gets destroyed, thus event handlers are not added twice when the application is bootstrapped again. BREAKING CHANGE: Methods of the `PlatformLocation` class, namely `onPopState` and `onHashChange`, used to return `void`. Now those methods return functions that can be called to remove event handlers. PR Close angular#31546 PR Close angular#40867
1 parent ca721c2 commit 38524c4

File tree

9 files changed

+86
-32
lines changed

9 files changed

+86
-32
lines changed

‎goldens/public-api/common/common.d.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,11 +96,12 @@ export declare function getLocaleWeekEndRange(locale: string): [WeekDay, WeekDay
9696

9797
export declare function getNumberOfCurrencyDigits(code: string): number;
9898

99-
export declare class HashLocationStrategy extends LocationStrategy {
99+
export declare class HashLocationStrategy extends LocationStrategy implements OnDestroy {
100100
constructor(_platformLocation: PlatformLocation, _baseHref?: string);
101101
back(): void;
102102
forward(): void;
103103
getBaseHref(): string;
104+
ngOnDestroy(): void;
104105
onPopState(fn: LocationChangeListener): void;
105106
path(includeHash?: boolean): string;
106107
prepareExternalUrl(internal: string): string;
@@ -324,11 +325,12 @@ export declare enum NumberSymbol {
324325
CurrencyGroup = 13
325326
}
326327

327-
export declare class PathLocationStrategy extends LocationStrategy {
328+
export declare class PathLocationStrategy extends LocationStrategy implements OnDestroy {
328329
constructor(_platformLocation: PlatformLocation, href?: string);
329330
back(): void;
330331
forward(): void;
331332
getBaseHref(): string;
333+
ngOnDestroy(): void;
332334
onPopState(fn: LocationChangeListener): void;
333335
path(includeHash?: boolean): string;
334336
prepareExternalUrl(internal: string): string;
@@ -355,8 +357,8 @@ export declare abstract class PlatformLocation {
355357
abstract forward(): void;
356358
abstract getBaseHrefFromDOM(): string;
357359
abstract getState(): unknown;
358-
abstract onHashChange(fn: LocationChangeListener): void;
359-
abstract onPopState(fn: LocationChangeListener): void;
360+
abstract onHashChange(fn: LocationChangeListener): VoidFunction;
361+
abstract onPopState(fn: LocationChangeListener): VoidFunction;
360362
abstract pushState(state: any, title: string, url: string): void;
361363
abstract replaceState(state: any, title: string, url: string): void;
362364
}

‎goldens/public-api/common/testing/testing.d.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,8 @@ export declare class MockPlatformLocation implements PlatformLocation {
3333
forward(): void;
3434
getBaseHrefFromDOM(): string;
3535
getState(): unknown;
36-
onHashChange(fn: LocationChangeListener): void;
37-
onPopState(fn: LocationChangeListener): void;
36+
onHashChange(fn: LocationChangeListener): VoidFunction;
37+
onPopState(fn: LocationChangeListener): VoidFunction;
3838
pushState(state: any, title: string, newUrl: string): void;
3939
replaceState(state: any, title: string, newUrl: string): void;
4040
}

‎goldens/size-tracking/aio-payloads.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,4 +26,4 @@
2626
}
2727
}
2828
}
29-
}
29+
}

‎packages/common/src/location/hash_location_strategy.ts‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {Inject, Injectable, Optional} from '@angular/core';
9+
import {Inject, Injectable, OnDestroy, Optional} from '@angular/core';
10+
1011
import {APP_BASE_HREF, LocationStrategy} from './location_strategy';
1112
import {LocationChangeListener, PlatformLocation} from './platform_location';
1213
import {joinWithSlash, normalizeQueryParams} from './util';
@@ -32,8 +33,10 @@ import {joinWithSlash, normalizeQueryParams} from './util';
3233
* @publicApi
3334
*/
3435
@Injectable()
35-
export class HashLocationStrategy extends LocationStrategy {
36+
export class HashLocationStrategy extends LocationStrategy implements OnDestroy {
3637
private _baseHref: string = '';
38+
private _removeListenerFns: (() => void)[] = [];
39+
3740
constructor(
3841
private _platformLocation: PlatformLocation,
3942
@Optional() @Inject(APP_BASE_HREF) _baseHref?: string) {
@@ -43,9 +46,15 @@ export class HashLocationStrategy extends LocationStrategy {
4346
}
4447
}
4548

49+
ngOnDestroy(): void {
50+
while (this._removeListenerFns.length) {
51+
this._removeListenerFns.pop()!();
52+
}
53+
}
54+
4655
onPopState(fn: LocationChangeListener): void {
47-
this._platformLocation.onPopState(fn);
48-
this._platformLocation.onHashChange(fn);
56+
this._removeListenerFns.push(
57+
this._platformLocation.onPopState(fn), this._platformLocation.onHashChange(fn));
4958
}
5059

5160
getBaseHref(): string {

‎packages/common/src/location/location_strategy.ts‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {Inject, Injectable, InjectionToken, Optional, ɵɵinject} from '@angular/core';
9+
import {Inject, Injectable, InjectionToken, OnDestroy, Optional, ɵɵinject} from '@angular/core';
1010
import {DOCUMENT} from '../dom_tokens';
1111
import {LocationChangeListener, PlatformLocation} from './platform_location';
1212
import {joinWithSlash, normalizeQueryParams} from './util';
@@ -105,8 +105,9 @@ export const APP_BASE_HREF = new InjectionToken<string>('appBaseHref');
105105
* @publicApi
106106
*/
107107
@Injectable()
108-
export class PathLocationStrategy extends LocationStrategy {
108+
export class PathLocationStrategy extends LocationStrategy implements OnDestroy {
109109
private _baseHref: string;
110+
private _removeListenerFns: (() => void)[] = [];
110111

111112
constructor(
112113
private _platformLocation: PlatformLocation,
@@ -125,9 +126,15 @@ export class PathLocationStrategy extends LocationStrategy {
125126
this._baseHref = href;
126127
}
127128

129+
ngOnDestroy(): void {
130+
while (this._removeListenerFns.length) {
131+
this._removeListenerFns.pop()!();
132+
}
133+
}
134+
128135
onPopState(fn: LocationChangeListener): void {
129-
this._platformLocation.onPopState(fn);
130-
this._platformLocation.onHashChange(fn);
136+
this._removeListenerFns.push(
137+
this._platformLocation.onPopState(fn), this._platformLocation.onHashChange(fn));
131138
}
132139

133140
getBaseHref(): string {

‎packages/common/src/location/platform_location.ts‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,14 @@ import {DOCUMENT} from '../dom_tokens';
4040
export abstract class PlatformLocation {
4141
abstract getBaseHrefFromDOM(): string;
4242
abstract getState(): unknown;
43-
abstract onPopState(fn: LocationChangeListener): void;
44-
abstract onHashChange(fn: LocationChangeListener): void;
43+
/**
44+
* Returns a function that, when executed, removes the `popstate` event handler.
45+
*/
46+
abstract onPopState(fn: LocationChangeListener): VoidFunction;
47+
/**
48+
* Returns a function that, when executed, removes the `hashchange` event handler.
49+
*/
50+
abstract onHashChange(fn: LocationChangeListener): VoidFunction;
4551

4652
abstract get href(): string;
4753
abstract get protocol(): string;
@@ -122,12 +128,16 @@ export class BrowserPlatformLocation extends PlatformLocation {
122128
return getDOM().getBaseHref(this._doc)!;
123129
}
124130

125-
onPopState(fn: LocationChangeListener): void {
126-
getDOM().getGlobalEventTarget(this._doc, 'window').addEventListener('popstate', fn, false);
131+
onPopState(fn: LocationChangeListener): VoidFunction {
132+
const window = getDOM().getGlobalEventTarget(this._doc, 'window');
133+
window.addEventListener('popstate', fn, false);
134+
return () => window.removeEventListener('popstate', fn);
127135
}
128136

129-
onHashChange(fn: LocationChangeListener): void {
130-
getDOM().getGlobalEventTarget(this._doc, 'window').addEventListener('hashchange', fn, false);
137+
onHashChange(fn: LocationChangeListener): VoidFunction {
138+
const window = getDOM().getGlobalEventTarget(this._doc, 'window');
139+
window.addEventListener('hashchange', fn, false);
140+
return () => window.removeEventListener('hashchange', fn);
131141
}
132142

133143
get href(): string {

‎packages/common/testing/src/mock_platform_location.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -153,13 +153,15 @@ export class MockPlatformLocation implements PlatformLocation {
153153
return this.baseHref;
154154
}
155155

156-
onPopState(fn: LocationChangeListener): void {
156+
onPopState(fn: LocationChangeListener): VoidFunction {
157157
// No-op: a state stack is not implemented, so
158158
// no events will ever come.
159+
return () => {};
159160
}
160161

161-
onHashChange(fn: LocationChangeListener): void {
162-
this.hashUpdate.subscribe(fn);
162+
onHashChange(fn: LocationChangeListener): VoidFunction {
163+
const subscription = this.hashUpdate.subscribe(fn);
164+
return () => subscription.unsubscribe();
163165
}
164166

165167
get href(): string {

‎packages/platform-server/src/location.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,13 +70,15 @@ export class ServerPlatformLocation implements PlatformLocation {
7070
return getDOM().getBaseHref(this._doc)!;
7171
}
7272

73-
onPopState(fn: LocationChangeListener): void {
73+
onPopState(fn: LocationChangeListener): VoidFunction {
7474
// No-op: a state stack is not implemented, so
7575
// no events will ever come.
76+
return () => {};
7677
}
7778

78-
onHashChange(fn: LocationChangeListener): void {
79-
this._hashUpdate.subscribe(fn);
79+
onHashChange(fn: LocationChangeListener): VoidFunction {
80+
const subscription = this._hashUpdate.subscribe(fn);
81+
return () => subscription.unsubscribe();
8082
}
8183

8284
get url(): string {

‎packages/router/test/bootstrap.spec.ts‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,12 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {APP_BASE_HREF, DOCUMENT, Location, ɵgetDOM as getDOM} from '@angular/common';
9+
import {APP_BASE_HREF, DOCUMENT, ɵgetDOM as getDOM} from '@angular/common';
1010
import {ApplicationRef, Component, CUSTOM_ELEMENTS_SCHEMA, destroyPlatform, NgModule} from '@angular/core';
1111
import {inject} from '@angular/core/testing';
1212
import {BrowserModule} from '@angular/platform-browser';
1313
import {platformBrowserDynamic} from '@angular/platform-browser-dynamic';
1414
import {NavigationEnd, Resolve, Router, RouterModule} from '@angular/router';
15-
import {filter, first} from 'rxjs/operators';
1615

1716
describe('bootstrap', () => {
1817
if (isNode) return;
@@ -369,7 +368,30 @@ describe('bootstrap', () => {
369368
done();
370369
});
371370

372-
function waitForNavigationToComplete(router: Router): Promise<any> {
373-
return router.events.pipe(filter((e: any) => e instanceof NavigationEnd), first()).toPromise();
374-
}
371+
it('should cleanup "popstate" and "hashchange" listeners', async () => {
372+
@NgModule({
373+
imports: [BrowserModule, RouterModule.forRoot([])],
374+
declarations: [RootCmp],
375+
bootstrap: [RootCmp],
376+
providers: testProviders,
377+
})
378+
class TestModule {
379+
}
380+
381+
spyOn(window, 'addEventListener').and.callThrough();
382+
spyOn(window, 'removeEventListener').and.callThrough();
383+
384+
const ngModuleRef = await platformBrowserDynamic().bootstrapModule(TestModule);
385+
ngModuleRef.destroy();
386+
387+
expect(window.addEventListener).toHaveBeenCalledTimes(2);
388+
389+
expect(window.addEventListener)
390+
.toHaveBeenCalledWith('popstate', jasmine.any(Function), jasmine.any(Boolean));
391+
expect(window.addEventListener)
392+
.toHaveBeenCalledWith('hashchange', jasmine.any(Function), jasmine.any(Boolean));
393+
394+
expect(window.removeEventListener).toHaveBeenCalledWith('popstate', jasmine.any(Function));
395+
expect(window.removeEventListener).toHaveBeenCalledWith('hashchange', jasmine.any(Function));
396+
});
375397
});

0 commit comments

Comments
 (0)