From 0c00c94f343017e07de2075ac2ab9a8ba9745d37 Mon Sep 17 00:00:00 2001
From: George Kalpakas <kalpakas.g@gmail.com>
Date: Wed, 3 Jul 2019 17:47:07 +0300
Subject: [PATCH] test(docs-infra): clean up global listeners after
 `ScrollService` tests (#31390)

The `ScrollService` sets up some global `window` listeners.

Previously, these listeners were never unregistered. This was not a
problem in the real app, because the `ScrollService` instance exists for
the lifetime of a user session. In tests, however, where the `window`
instance is among all tests, the listeners would survive the
`ScrollService` tests. This, in addition to the fact that we used a mock
`ViewportScroller` which did not return the expected type from
`getScrollPosition()`, caused errors to be thrown in unrelated tests
(i.e. whenever a scroll event was emitted on `window`). See [here][1]
for an example failure.

This commit fixes it by adding an `ngOnDestroy()` method that
unregisters the listeners and ensuring it is called after each
`ScrollService` test.

[1]: https://circleci.com/gh/angular/angular/381649

PR Close #31390
---
 aio/src/app/shared/scroll.service.spec.ts | 38 +++++++++++++++++++++++
 aio/src/app/shared/scroll.service.ts      | 20 ++++++++----
 2 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/aio/src/app/shared/scroll.service.spec.ts b/aio/src/app/shared/scroll.service.spec.ts
index ba529a593b..50e1175882 100644
--- a/aio/src/app/shared/scroll.service.spec.ts
+++ b/aio/src/app/shared/scroll.service.spec.ts
@@ -51,6 +51,8 @@ describe('ScrollService', () => {
     spyOn(window, 'scrollBy');
   });
 
+  afterEach(() => scrollService.ngOnDestroy());
+
   it('should debounce `updateScrollPositonInHistory()`', fakeAsync(() => {
     const updateScrollPositionInHistorySpy = spyOn(scrollService, 'updateScrollPositionInHistory');
 
@@ -65,6 +67,25 @@ describe('ScrollService', () => {
     expect(updateScrollPositionInHistorySpy).toHaveBeenCalledTimes(1);
   }));
 
+  it('should stop updating scroll position once destroyed', fakeAsync(() => {
+    const updateScrollPositionInHistorySpy = spyOn(scrollService, 'updateScrollPositionInHistory');
+
+    window.dispatchEvent(new Event('scroll'));
+    tick(250);
+    expect(updateScrollPositionInHistorySpy).toHaveBeenCalledTimes(1);
+
+    window.dispatchEvent(new Event('scroll'));
+    tick(250);
+    expect(updateScrollPositionInHistorySpy).toHaveBeenCalledTimes(2);
+
+    updateScrollPositionInHistorySpy.calls.reset();
+    scrollService.ngOnDestroy();
+
+    window.dispatchEvent(new Event('scroll'));
+    tick(250);
+    expect(updateScrollPositionInHistorySpy).not.toHaveBeenCalled();
+  }));
+
   it('should set `scrollRestoration` to `manual` if supported', () => {
     if (scrollService.supportManualScrollRestoration) {
       expect(window.history.scrollRestoration).toBe('manual');
@@ -112,6 +133,23 @@ describe('ScrollService', () => {
       expect(scrollService.topOffset).toBe(100 + topMargin);
       expect(document.querySelector).toHaveBeenCalled();
     });
+
+    it('should stop updating on resize once destroyed', () => {
+      let clientHeight = 50;
+      (document.querySelector as jasmine.Spy).and.callFake(() => ({clientHeight}));
+
+      expect(scrollService.topOffset).toBe(50 + topMargin);
+
+      clientHeight = 100;
+      window.dispatchEvent(new Event('resize'));
+      expect(scrollService.topOffset).toBe(100 + topMargin);
+
+      scrollService.ngOnDestroy();
+
+      clientHeight = 200;
+      window.dispatchEvent(new Event('resize'));
+      expect(scrollService.topOffset).toBe(100 + topMargin);
+    });
   });
 
   describe('#topOfPageElement', () => {
diff --git a/aio/src/app/shared/scroll.service.ts b/aio/src/app/shared/scroll.service.ts
index 4c0d864141..aa08a517ee 100644
--- a/aio/src/app/shared/scroll.service.ts
+++ b/aio/src/app/shared/scroll.service.ts
@@ -1,7 +1,7 @@
 import { DOCUMENT, Location, PlatformLocation, PopStateEvent, ViewportScroller } from '@angular/common';
-import { Injectable, Inject } from '@angular/core';
-import { fromEvent } from 'rxjs';
-import { debounceTime } from 'rxjs/operators';
+import { Injectable, Inject, OnDestroy } from '@angular/core';
+import { fromEvent, Subject } from 'rxjs';
+import { debounceTime, takeUntil } from 'rxjs/operators';
 
 type ScrollPosition = [number, number];
 interface ScrollPositionPopStateEvent extends PopStateEvent {
@@ -14,10 +14,11 @@ export const topMargin = 16;
  * A service that scrolls document elements into view
  */
 @Injectable()
-export class ScrollService {
+export class ScrollService implements OnDestroy {
 
   private _topOffset: number | null;
   private _topOfPageElement: Element;
+  private onDestroy = new Subject<void>();
 
   // The scroll position which has to be restored, after a `popstate` event.
   poppedStateScrollPosition: ScrollPosition | null = null;
@@ -49,10 +50,13 @@ export class ScrollService {
       private viewportScroller: ViewportScroller,
       private location: Location) {
     // On resize, the toolbar might change height, so "invalidate" the top offset.
-    fromEvent(window, 'resize').subscribe(() => this._topOffset = null);
+    fromEvent(window, 'resize')
+        .pipe(takeUntil(this.onDestroy))
+        .subscribe(() => this._topOffset = null);
 
     fromEvent(window, 'scroll')
-      .pipe(debounceTime(250)).subscribe(() => this.updateScrollPositionInHistory());
+        .pipe(debounceTime(250), takeUntil(this.onDestroy))
+        .subscribe(() => this.updateScrollPositionInHistory());
 
     // Change scroll restoration strategy to `manual` if it's supported
     if (this.supportManualScrollRestoration) {
@@ -75,6 +79,10 @@ export class ScrollService {
     }
   }
 
+  ngOnDestroy() {
+    this.onDestroy.next();
+  }
+
   /**
    * Scroll to the element with id extracted from the current location hash fragment.
    * Scroll to top if no hash.