Address PR feedback, add a test helper for stubbing timeouts

Author: sam-gc

packages-exp/auth-exp/src/platform_browser/auth_window.d.ts

@@ -0,0 +1,37 @@
+/**
+ * @license
+ * Copyright 2020 Google LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { Recaptcha } from './recaptcha/recaptcha';
+
+/** 
+ * A specialized window type that melds the normal window type plus the 
+ * various bits we need. The three different blocks that are &'d together
+ * cant be defined in the same block together.
+ */
+export type AuthWindow = {
+  // Standard window types
+  [T in keyof Window]: Window[T];
+} & {
+  // Any known / named properties we want to add
+  grecaptcha?: Recaptcha;
+} & {
+  // A final catch-all for callbacks (which will have random names) that
+  // we will stick on the window.
+  [callback: string]: ((...args: unknown[]) => void);
+};
+
+export const AUTH_WINDOW = window as unknown as AuthWindow;

packages-exp/auth-exp/src/platform_browser/auth_window.ts

@@ -23,19 +23,15 @@ import * as sinonChai from 'sinon-chai';
 import { FirebaseError } from '@firebase/util';
 
 import { testAuth } from '../../../test/mock_auth';
+import { stubSingleTimeout } from '../../../test/timeout_stub';
 import { Auth } from '../../model/auth';
-import { AuthWindow } from '../auth_window';
+import { AUTH_WINDOW } from '../auth_window';
 import * as jsHelpers from '../load_js';
 import {
-  _JSLOAD_CALLBACK,
-  MOCK_RECAPTCHA_LOADER,
-  ReCaptchaLoader,
-  ReCaptchaLoaderImpl
+    _JSLOAD_CALLBACK, MOCK_RECAPTCHA_LOADER, ReCaptchaLoader, ReCaptchaLoaderImpl
 } from './recaptcha_loader';
 import { MockReCaptcha } from './recaptcha_mock';
 
-const WINDOW: AuthWindow = window;
-
 use(chaiAsPromised);
 use(sinonChai);
 
@@ -48,7 +44,7 @@ describe('platform-browser/recaptcha/recaptcha_loader', () => {
 
   afterEach(() => {
     sinon.restore();
-    delete WINDOW.grecaptcha;
+    delete AUTH_WINDOW.grecaptcha;
   });
 
   describe('MockLoader', () => {
@@ -66,13 +62,7 @@ describe('platform-browser/recaptcha/recaptcha_loader', () => {
     const networkTimeoutId = 123;
 
     beforeEach(() => {
-      sinon.stub(window, 'setTimeout').callsFake(cb => {
-        triggerNetworkTimeout = () => cb();
-        // For some bizarre reason setTimeout always get shoehorned into NodeJS.Timeout,
-        // which is flat-wrong. This is the easiest way to fix it.
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        return networkTimeoutId as any;
-      });
+      triggerNetworkTimeout = stubSingleTimeout(networkTimeoutId);
 
       sinon.stub(jsHelpers, '_loadJS').callsFake(() => {
         return new Promise((resolve, reject) => {
@@ -105,15 +95,15 @@ describe('platform-browser/recaptcha/recaptcha_loader', () => {
 
     context('on js load callback', () => {
       function spoofJsLoad(): void {
-        WINDOW[_JSLOAD_CALLBACK]();
+        AUTH_WINDOW[_JSLOAD_CALLBACK]();
       }
 
       it('clears the network timeout', () => {
-        sinon.spy(WINDOW, 'clearTimeout');
+        sinon.spy(AUTH_WINDOW, 'clearTimeout');
         // eslint-disable-next-line @typescript-eslint/no-floating-promises
         loader.load(auth);
         spoofJsLoad();
-        expect(WINDOW.clearTimeout).to.have.been.calledWith(networkTimeoutId);
+        expect(AUTH_WINDOW.clearTimeout).to.have.been.calledWith(networkTimeoutId);
       });
 
       it('rejects if the grecaptcha object is not on the window', async () => {
@@ -127,25 +117,26 @@ describe('platform-browser/recaptcha/recaptcha_loader', () => {
 
       it('overwrites the render method', async () => {
         const promise = loader.load(auth);
-        const oldRenderMethod = (): string => 'foo';
-        WINDOW.grecaptcha = { render: oldRenderMethod };
+        const mockRecaptcha = new MockReCaptcha(auth);
+        const oldRenderMethod = mockRecaptcha.render;
+        AUTH_WINDOW.grecaptcha = mockRecaptcha;
         spoofJsLoad();
         expect((await promise).render).not.to.eq(oldRenderMethod);
       });
 
       it('returns immediately if the new language code matches the old', async () => {
         const promise = loader.load(auth);
-        WINDOW.grecaptcha = { render: (): string => 'foo' };
+        AUTH_WINDOW.grecaptcha = new MockReCaptcha(auth);
         spoofJsLoad();
         await promise;
         // Notice no call to spoofJsLoad..
-        expect(await loader.load(auth)).to.eq(WINDOW.grecaptcha);
+        expect(await loader.load(auth)).to.eq(AUTH_WINDOW.grecaptcha);
       });
 
       it('returns immediately if grecaptcha is already set on window', async () => {
-        WINDOW.grecaptcha = { render: (): string => 'foo' };
+        AUTH_WINDOW.grecaptcha = new MockReCaptcha(auth);
         const loader = new ReCaptchaLoaderImpl();
-        expect(await loader.load(auth)).to.eq(WINDOW.grecaptcha);
+        expect(await loader.load(auth)).to.eq(AUTH_WINDOW.grecaptcha);
       });
     });
   });

packages-exp/auth-exp/src/platform_browser/recaptcha/recaptcha_loader.test.ts

@@ -20,7 +20,7 @@ import { querystring } from '@firebase/util';
 import { AUTH_ERROR_FACTORY, AuthErrorCode } from '../../core/errors';
 import { Delay } from '../../core/util/delay';
 import { Auth } from '../../model/auth';
-import { AuthWindow } from '../auth_window';
+import { AUTH_WINDOW } from '../auth_window';
 import * as jsHelpers from '../load_js';
 import { Recaptcha } from './recaptcha';
 import { MockReCaptcha } from './recaptcha_mock';
@@ -36,38 +36,32 @@ export interface ReCaptchaLoader {
   clearedOneInstance(): void;
 }
 
-function pullGrecaptcha(): Recaptcha | undefined {
-  return (window as AuthWindow).grecaptcha;
-}
-
-const WINDOW = window as AuthWindow;
-
 /**
  * Loader for the GReCaptcha library. There should only ever be one of this.
  */
 export class ReCaptchaLoaderImpl implements ReCaptchaLoader {
-  private hl = '';
+  private hostLanguage = '';
   private counter = 0;
-  private readonly librarySeparatelyLoaded = !!pullGrecaptcha();
+  private readonly librarySeparatelyLoaded = !!AUTH_WINDOW.grecaptcha;
 
   load(auth: Auth, hl = ''): Promise<Recaptcha> {
     if (this.shouldResolveImmediately(hl)) {
-      return Promise.resolve(pullGrecaptcha()!);
+      return Promise.resolve(AUTH_WINDOW.grecaptcha!);
     }
     return new Promise<Recaptcha>((resolve, reject) => {
-      const networkTimeout = WINDOW.setTimeout(() => {
+      const networkTimeout = AUTH_WINDOW.setTimeout(() => {
         reject(
           AUTH_ERROR_FACTORY.create(AuthErrorCode.NETWORK_REQUEST_FAILED, {
             appName: auth.name
           })
         );
       }, NETWORK_TIMEOUT_DELAY.get());
 
-      WINDOW[_JSLOAD_CALLBACK] = () => {
-        WINDOW.clearTimeout(networkTimeout);
-        delete WINDOW[_JSLOAD_CALLBACK];
+      AUTH_WINDOW[_JSLOAD_CALLBACK] = () => {
+        AUTH_WINDOW.clearTimeout(networkTimeout);
+        delete AUTH_WINDOW[_JSLOAD_CALLBACK];
 
-        const recaptcha = pullGrecaptcha();
+        const recaptcha = AUTH_WINDOW.grecaptcha;
 
         if (!recaptcha) {
           reject(
@@ -87,7 +81,7 @@ export class ReCaptchaLoaderImpl implements ReCaptchaLoader {
           return widgetId;
         };
 
-        this.hl = hl;
+        this.hostLanguage = hl;
         resolve(recaptcha);
       };
 
@@ -121,8 +115,8 @@ export class ReCaptchaLoaderImpl implements ReCaptchaLoader {
     // In cases (2) and (3), we _can't_ reload as it would break the recaptchas
     // that are already in the page
     return (
-      !!pullGrecaptcha() &&
-      (hl === this.hl || this.counter > 0 || this.librarySeparatelyLoaded)
+      !!AUTH_WINDOW.grecaptcha &&
+      (hl === this.hostLanguage || this.counter > 0 || this.librarySeparatelyLoaded)
     );
   }
 }

packages-exp/auth-exp/src/platform_browser/recaptcha/recaptcha_loader.ts

@@ -23,14 +23,10 @@ import * as sinonChai from 'sinon-chai';
 import { FirebaseError } from '@firebase/util';
 
 import { testAuth } from '../../../test/mock_auth';
+import { stubTimeouts, TimerMap } from '../../../test/timeout_stub';
 import { Auth } from '../../model/auth';
 import {
-  _EXPIRATION_TIME_MS,
-  _SOLVE_TIME_MS,
-  _WIDGET_ID_START,
-  MockReCaptcha,
-  MockWidget,
-  Widget
+    _EXPIRATION_TIME_MS, _SOLVE_TIME_MS, _WIDGET_ID_START, MockReCaptcha, MockWidget, Widget
 } from './recaptcha_mock';
 
 use(sinonChai);
@@ -150,45 +146,26 @@ describe('platform-browser/recaptcha/recaptcha_mock', () => {
 
     context('#execute', () => {
       // Stub out a bunch of stuff on setTimer
-      let tripSolveTimer: () => void;
-      let tripExpireTimer: () => void;
+      let pendingTimers: TimerMap;
       let callbacks: { [key: string]: sinon.SinonSpy };
       let widget: MockWidget;
       let timeoutStub: sinon.SinonStub;
 
-      const solveTimer = 1;
-      const expireTimer = 2;
-
       beforeEach(() => {
         callbacks = {
           'callback': sinon.spy(),
           'expired-callback': sinon.spy()
         };
-        timeoutStub = sinon
-          .stub(window, 'setTimeout')
-          .callsFake((cb, duration) => {
-            switch (duration) {
-              case _SOLVE_TIME_MS:
-                tripSolveTimer = cb;
-                // For some bizarre reason setTimeout always get shoehorned into NodeJS.Timeout,
-                // which is flat-wrong. This is the easiest way to fix it.
-                // eslint-disable-next-line @typescript-eslint/no-explicit-any
-                return solveTimer as any;
-              case _EXPIRATION_TIME_MS:
-                tripExpireTimer = cb;
-                return expireTimer;
-              default:
-                throw new Error('Requested unknown callback duration');
-            }
-          });
+        pendingTimers = stubTimeouts();
+        timeoutStub = window.setTimeout as unknown as sinon.SinonStub;
         widget = new MockWidget(container, auth.name, callbacks);
       });
 
       it('keeps re-executing with new tokens if expiring', () => {
-        tripSolveTimer();
-        tripExpireTimer();
-        tripSolveTimer();
-        tripExpireTimer();
+        pendingTimers[_SOLVE_TIME_MS]();
+        pendingTimers[_EXPIRATION_TIME_MS]();
+        pendingTimers[_SOLVE_TIME_MS]();
+        pendingTimers[_EXPIRATION_TIME_MS]();
 
         expect(callbacks['callback']).to.have.been.calledTwice;
         expect(callbacks['callback'].getCall(0).args[0]).not.to.eq(
@@ -197,21 +174,21 @@ describe('platform-browser/recaptcha/recaptcha_mock', () => {
       });
 
       it('posts callback with a random alphanumeric code', () => {
-        tripSolveTimer();
+        pendingTimers[_SOLVE_TIME_MS]();
         const arg: string = callbacks['callback'].getCall(0).args[0];
         expect(arg).to.be.a('string');
         expect(arg.length).to.equal(50);
       });
 
       it('expired callback does execute if just solve trips', () => {
-        tripSolveTimer();
+        pendingTimers[_SOLVE_TIME_MS]();
         expect(callbacks['callback']).to.have.been.called;
         expect(callbacks['expired-callback']).not.to.have.been.called;
       });
 
       it('expired callback executes if just expiration timer trips', () => {
-        tripSolveTimer();
-        tripExpireTimer();
+        pendingTimers[_SOLVE_TIME_MS]();
+        pendingTimers[_EXPIRATION_TIME_MS]();
         expect(callbacks['callback']).to.have.been.called;
         expect(callbacks['expired-callback']).to.have.been.called;
       });

packages-exp/auth-exp/src/platform_browser/recaptcha/recaptcha_mock.test.ts

@@ -27,7 +27,7 @@ import { testAuth } from '../../../test/mock_auth';
 import * as fetch from '../../../test/mock_fetch';
 import { Endpoint } from '../../api';
 import { Auth } from '../../model/auth';
-import { AuthWindow } from '../auth_window';
+import { AUTH_WINDOW } from '../auth_window';
 import { Parameters, Recaptcha } from './recaptcha';
 import { _JSLOAD_CALLBACK, MOCK_RECAPTCHA_LOADER } from './recaptcha_loader';
 import { MockReCaptcha } from './recaptcha_mock';
@@ -144,8 +144,8 @@ describe('platform_browser/recaptcha/recaptcha_verifier.ts', () => {
 
     it('calls existing global function if on the window', async () => {
       let token = '';
-      (window as AuthWindow).callbackOnWindowObject = (t: string): void => {
-        token = t;
+      AUTH_WINDOW.callbackOnWindowObject = (t: unknown): void => {
+        token = t as string;
       };
 
       parameters = {
@@ -156,7 +156,7 @@ describe('platform_browser/recaptcha/recaptcha_verifier.ts', () => {
       const expected = await verifier.verify();
       expect(token).to.eq(expected);
 
-      delete (window as AuthWindow).callbackOnWindowObject;
+      delete AUTH_WINDOW.callbackOnWindowObject;
     });
   });
 

packages-exp/auth-exp/src/platform_browser/recaptcha/recaptcha_verifier.test.ts

@@ -23,13 +23,9 @@ import { AuthErrorCode } from '../../core/errors';
 import { assert } from '../../core/util/assert';
 import { _isHttpOrHttps } from '../../core/util/location';
 import { Auth } from '../../model/auth';
-import { AuthWindow } from '../auth_window';
+import { AUTH_WINDOW } from '../auth_window';
 import { Parameters, Recaptcha } from './recaptcha';
-import {
-  MOCK_RECAPTCHA_LOADER,
-  RECAPTCHA_LOADER,
-  ReCaptchaLoader
-} from './recaptcha_loader';
+import { MOCK_RECAPTCHA_LOADER, RECAPTCHA_LOADER, ReCaptchaLoader } from './recaptcha_loader';
 
 const DEFAULT_PARAMS: Parameters = {
   theme: 'light',
@@ -169,7 +165,7 @@ export class RecaptchaVerifier implements ApplicationVerifier {
       if (typeof existing === 'function') {
         existing(token);
       } else if (typeof existing === 'string') {
-        const globalFunc = (window as AuthWindow)[existing];
+        const globalFunc = AUTH_WINDOW[existing];
         if (typeof globalFunc === 'function') {
           globalFunc(token);
         }
@@ -218,10 +214,9 @@ export class RecaptchaVerifier implements ApplicationVerifier {
 }
 
 function isWorker(): boolean {
-  const win: AuthWindow = window;
   return (
-    typeof win['WorkerGlobalScope'] !== 'undefined' &&
-    typeof win['importScripts'] === 'function'
+    typeof AUTH_WINDOW['WorkerGlobalScope'] !== 'undefined' &&
+    typeof AUTH_WINDOW['importScripts'] === 'function'
   );
 }