Repository: auth0/auth0-spa-js (ts)

Issue:
Unexpected uncaught "Error: Timeout when executing 'fetch'" promise rejection
### Description
Even when the `fetchWithTimeout` function (in `src/utils.ts`) resolves successfully (i.e. there is no timeout) it still produces an uncaught promise rejection (`Error: Timeout when executing 'fetch'`). While this has no impact on the application, it does causes Chrome devtools to pause it's debugger (when the "Pause on exceptions" option is enabled). As an application developer, this behavior is unintuitive since everything appears to work as expected, and there are no network requests timing out.

Unless this is intended behavior, I'd be happy to put up a quick PR to resolve the issue.

### Reproduction
- Run sample app with `npm start`
- Load sample app with Chrome and open devtools
- Under "Sources" tab, Ensure "Pause on Exceptions" debugger option is enabled
- Login with popup or redirect/callback
- "Is authenticated" should now be true (i.e. there was not a fetch timeout)
- After 10 seconds the debugger should pause on the uncaught promise rejection "Error: Timeout when executing 'fetch'"
[<img width="1372" alt="Screen Shot 2020-05-27 at 1 36 02 PM" src="https://user-images.githubusercontent.com/371998/83060040-73c66180-a020-11ea-9dff-1e2e0695561b.png">
](url)

### Environment
- auth0-spa-js: 1.8.2
- Chrome: 83.0.4103.61

Pull request description:
Prevent uncaught timeout promise rejection. Fixes #483
### Description

Recap of #483:
> Even when the `fetchWithTimeout` function (in src/utils.ts) resolves successfully (i.e. there is no timeout) it still produces an uncaught promise rejection (Error: Timeout when executing 'fetch'). While this has no impact on the application, it does causes Chrome devtools to pause it's debugger (when the "Pause on exceptions" option is enabled). As an application developer, this behavior is unintuitive since everything appears to work as expected, and there are no network requests timing out.

As suggested by @adamjmcgrath, this change clears the timeout in `fetchWithTimeout` when the `switchFetch` resolves before the timeout promise. 

### References

Fixes #483 

### Testing

I did include a regression test - though I'll be the first to admit that it's less than ideal as it's testing implementation, not behavior. That said, I'm unsure if it's possible to directly verify the absence of an unreferenced & uncaught promise rejection without jumping through hoops. I'm open to other approaches, or to removing the test altogether.

If nothing else, two tests in `Auth0Client.test.ts` indirectly verify this behavior by counting how often `AbortController.prototype.abort` is called.

- [x] This change adds test coverage for new/changed/fixed functionality

### Checklist

- [ ] I have added documentation for new/changed functionality in this PR or in auth0.com/docs
- [ ] All active GitHub checks for tests, formatting, and security are passing
- [ ] The correct base branch is being used, if not `master`


Patch:
diff --git a/src/utils.ts b/src/utils.ts
index 99cca62..3effe54 100644
--- a/src/utils.ts
+++ b/src/utils.ts
@@ -229,7 +229,7 @@ const switchFetch = async (url, opts, timeout, worker) => {
   }
 };
 
-const fetchWithTimeout = (
+export const fetchWithTimeout = (
   url,
   options,
   worker,
@@ -243,16 +243,19 @@ const fetchWithTimeout = (
     signal
   };
 
+  let timeoutId;
   // The promise will resolve with one of these two promises (the fetch or the timeout), whichever completes first.
   return Promise.race([
     switchFetch(url, fetchOptions, timeout, worker),
     new Promise((_, reject) => {
-      setTimeout(() => {
+      timeoutId = setTimeout(() => {
         controller.abort();
         reject(new Error("Timeout when executing 'fetch'"));
       }, timeout);
     })
-  ]);
+  ]).finally(() => {
+    clearTimeout(timeoutId);
+  });
 };
 
 const getJSON = async (url, timeout, options, worker) => {


Test patch:
diff --git a/__tests__/Auth0Client.test.ts b/__tests__/Auth0Client.test.ts
index 712a512..0e08e53 100644
--- a/__tests__/Auth0Client.test.ts
+++ b/__tests__/Auth0Client.test.ts
@@ -361,10 +361,9 @@ describe('Auth0Client', () => {
     await expect(auth0.getTokenSilently({ ignoreCache: true })).rejects.toThrow(
       `Timeout when executing 'fetch'`
     );
-    // Called once for the authorization grant (noop)
     // Called thrice for the refresh token grant in utils (noop)
     // Called thrice for the refresh token grant in token worker
-    expect(AbortController.prototype.abort).toBeCalledTimes(7);
+    expect(AbortController.prototype.abort).toBeCalledTimes(6);
     expect(mockFetch).toBeCalledTimes(3);
     Object.defineProperty(constants, 'DEFAULT_FETCH_TIMEOUT_MS', {
       get: () => originalDefaultFetchTimeoutMs
@@ -459,9 +458,8 @@ describe('Auth0Client', () => {
     await expect(auth0.getTokenSilently({ ignoreCache: true })).rejects.toThrow(
       `Timeout when executing 'fetch'`
     );
-    // Called once for the authorization grant (noop)
     // Called thrice for the refresh token grant in utils
-    expect(AbortController.prototype.abort).toBeCalledTimes(4);
+    expect(AbortController.prototype.abort).toBeCalledTimes(3);
     expect(mockFetch).toBeCalledTimes(3);
     Object.defineProperty(constants, 'DEFAULT_FETCH_TIMEOUT_MS', {
       get: () => originalDefaultFetchTimeoutMs
diff --git a/__tests__/utils.test.ts b/__tests__/utils.test.ts
index 0fedd46..a80da8d 100644
--- a/__tests__/utils.test.ts
+++ b/__tests__/utils.test.ts
@@ -6,6 +6,7 @@ import {
   createRandomString,
   encode,
   decode,
+  fetchWithTimeout,
   sha256,
   runPopup,
   runIframe,
@@ -19,9 +20,16 @@ import {
   DEFAULT_AUTHORIZE_TIMEOUT_IN_SECONDS,
   DEFAULT_SILENT_TOKEN_RETRY_COUNT
 } from '../src/constants';
+import unfetch from 'unfetch';
 
+jest.mock('unfetch');
+const mockUnfetch = <jest.Mock>unfetch;
 (<any>global).TextEncoder = TextEncoder;
 
+afterEach(() => {
+  jest.resetAllMocks();
+});
+
 describe('utils', () => {
   describe('parseQueryResult', () => {
     it('parses the query string', () => {
@@ -135,6 +143,19 @@ describe('utils', () => {
       expect(decode('dGVzdA==')).toBe('test');
     });
   });
+  describe('fetchWithTimeout', () => {
+    it('clears timeout when successful', async () => {
+      mockUnfetch.mockImplementation(() =>
+        Promise.resolve({
+          ok: true,
+          json: () => Promise.resolve()
+        })
+      );
+      jest.spyOn(window, 'clearTimeout');
+      await fetchWithTimeout('https://test.com/', {}, undefined);
+      expect(clearTimeout).toBeCalledTimes(1);
+    });
+  });
   describe('sha256', () => {
     it('generates a digest of the given data', async () => {
       (<any>global).crypto = {
@@ -229,14 +250,9 @@ describe('utils', () => {
 
   describe('oauthToken', () => {
     let oauthToken;
-    let mockUnfetch;
     let abortController;
 
     beforeEach(() => {
-      jest.resetModules();
-      jest.mock('unfetch');
-      mockUnfetch = require('unfetch');
-
       const utils = require('../src/utils');
       oauthToken = utils.oauthToken;