Repository: auth0/auth0-spa-js
Pull request #689 (2021-01-06 11:17:06)

[SDK-2238] Only use timeout promise when using fetchWithTimeout without a worker
fetchWithTimeout is, as the name implies, configured to timeout after a certain `timeout`.

```
return Promise.race([
    switchFetch(url, audience, scope, fetchOptions, timeout, worker),
    new Promise((_, reject) => {
      timeoutId = setTimeout(() => {
        controller.abort();
        reject(new Error("Timeout when executing 'fetch'"));
      }, timeout);
    })
  ]).finally(() => {
    clearTimeout(timeoutId);
  });
```

In the above snippet, there are two promises:
- **switchFetch**, which is configured internally to timeout after `timeout` seconds. Important here is that this timeout is only for inside the worker. So when there is no worker, the timeout is ignored
- a **timeout promise** to ensure the promise.race is rejected when the timeout is hit.

When we are not using a worker, the second promise is what is being used to reject on a timeout.
When a worker is used, both fetchTimeout and the timeout promise will reject. However, it will not always be the same promise that rejects first, occasionally the switchFetch rejects before the timeout promise.

In case of a worker, the fact that we have two promises to handle the sme timeout can be a bit weird. So this PR resolves this to only use the timeout promise in case of not using a worker.


Patch:
diff --git a/src/http.ts b/src/http.ts
index 8497487..3455476 100644
--- a/src/http.ts
+++ b/src/http.ts
@@ -11,60 +11,27 @@ import { GenericError } from './errors';
 
 export const createAbortController = () => new AbortController();
 
-const switchFetch = async (
-  fetchUrl: string,
-  audience: string,
-  scope: string,
-  fetchOptions: FetchOptions,
-  timeout: number,
-  worker?: Worker
-): Promise<any> => {
-  if (worker) {
-    // AbortSignal is not serializable, need to implement in the Web Worker
-    delete fetchOptions.signal;
-
-    return sendMessage(
-      {
-        auth: {
-          audience,
-          scope
-        },
-        timeout,
-        fetchUrl,
-        fetchOptions
-      },
-      worker
-    );
-  } else {
-    const response = await fetch(fetchUrl, fetchOptions);
-    return {
-      ok: response.ok,
-      json: await response.json()
-    };
-  }
+const dofetch = async (fetchUrl: string, fetchOptions: FetchOptions) => {
+  const response = await fetch(fetchUrl, fetchOptions);
+  return {
+    ok: response.ok,
+    json: await response.json()
+  };
 };
 
-export const fetchWithTimeout = (
-  url: string,
-  audience: string,
-  scope: string,
-  options: FetchOptions,
-  worker?: Worker,
-  timeout = DEFAULT_FETCH_TIMEOUT_MS
-): Promise<any> => {
+const fetchWithoutWorker = async (
+  fetchUrl: string,
+  fetchOptions: FetchOptions,
+  timeout: number
+) => {
   const controller = createAbortController();
-  const signal = controller.signal;
-
-  const fetchOptions = {
-    ...options,
-    signal
-  };
+  fetchOptions.signal = controller.signal;
 
   let timeoutId: NodeJS.Timeout;
 
   // The promise will resolve with one of these two promises (the fetch or the timeout), whichever completes first.
   return Promise.race([
-    switchFetch(url, audience, scope, fetchOptions, timeout, worker),
+    dofetch(fetchUrl, fetchOptions),
     new Promise((_, reject) => {
       timeoutId = setTimeout(() => {
         controller.abort();
@@ -76,6 +43,50 @@ export const fetchWithTimeout = (
   });
 };
 
+const fetchWithWorker = async (
+  fetchUrl: string,
+  audience: string,
+  scope: string,
+  fetchOptions: FetchOptions,
+  timeout: number,
+  worker?: Worker
+) => {
+  return sendMessage(
+    {
+      auth: {
+        audience,
+        scope
+      },
+      timeout,
+      fetchUrl,
+      fetchOptions
+    },
+    worker
+  );
+};
+
+export const switchFetch = async (
+  fetchUrl: string,
+  audience: string,
+  scope: string,
+  fetchOptions: FetchOptions,
+  worker?: Worker,
+  timeout = DEFAULT_FETCH_TIMEOUT_MS
+): Promise<any> => {
+  if (worker) {
+    return fetchWithWorker(
+      fetchUrl,
+      audience,
+      scope,
+      fetchOptions,
+      timeout,
+      worker
+    );
+  } else {
+    return fetchWithoutWorker(fetchUrl, fetchOptions, timeout);
+  }
+};
+
 export async function getJSON<T>(
   url: string,
   timeout: number,
@@ -89,7 +100,7 @@ export async function getJSON<T>(
 
   for (let i = 0; i < DEFAULT_SILENT_TOKEN_RETRY_COUNT; i++) {
     try {
-      response = await fetchWithTimeout(
+      response = await switchFetch(
         url,
         audience,
         scope,
diff --git a/src/worker/token.worker.ts b/src/worker/token.worker.ts
index ed64fc7..9f5be37 100644
--- a/src/worker/token.worker.ts
+++ b/src/worker/token.worker.ts
@@ -70,7 +70,7 @@ const messageHandler = async ({
     }
 
     if (!response) {
-      // If the request times out, abort it and let `fetchWithTimeout` raise the error.
+      // If the request times out, abort it and let `switchFetch` raise the error.
       if (abortController) abortController.abort();
 
       port.postMessage({


Test patch:
diff --git a/__tests__/Auth0Client/getTokenSilently.test.ts b/__tests__/Auth0Client/getTokenSilently.test.ts
index c44b9ee..fca5e9f 100644
--- a/__tests__/Auth0Client/getTokenSilently.test.ts
+++ b/__tests__/Auth0Client/getTokenSilently.test.ts
@@ -792,9 +792,8 @@ describe('Auth0Client', () => {
         auth0.getTokenSilently({ ignoreCache: true })
       ).rejects.toThrow(`Timeout when executing 'fetch'`);
 
-      // 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(6);
+      expect(AbortController.prototype.abort).toBeCalledTimes(3);
       expect(mockFetch).toBeCalledTimes(3);
 
       Object.defineProperty(constants, 'DEFAULT_FETCH_TIMEOUT_MS', {
@@ -894,7 +893,7 @@ describe('Auth0Client', () => {
       await expect(
         auth0.getTokenSilently({ ignoreCache: true })
       ).rejects.toThrow(`Timeout when executing 'fetch'`);
-      // Called thrice for the refresh token grant in utils
+      // Called thrice for the refresh token grant in http.switchFetch
       expect(AbortController.prototype.abort).toBeCalledTimes(3);
       expect(mockFetch).toBeCalledTimes(3);
       Object.defineProperty(constants, 'DEFAULT_FETCH_TIMEOUT_MS', {
diff --git a/__tests__/http.test.ts b/__tests__/http.test.ts
index 9bf7281..5d3c913 100644
--- a/__tests__/http.test.ts
+++ b/__tests__/http.test.ts
@@ -1,12 +1,12 @@
 import fetch from 'unfetch';
-import { fetchWithTimeout } from '../src/http';
+import { switchFetch } from '../src/http';
 
 jest.mock('../src/worker/token.worker');
 jest.mock('unfetch');
 
 const mockUnfetch = <jest.Mock>fetch;
 
-describe('fetchWithTimeout', () => {
+describe('switchFetch', () => {
   it('clears timeout when successful', async () => {
     mockUnfetch.mockImplementation(() =>
       Promise.resolve({
@@ -15,7 +15,7 @@ describe('fetchWithTimeout', () => {
       })
     );
     jest.spyOn(window, 'clearTimeout');
-    await fetchWithTimeout('https://test.com/', null, null, {}, undefined);
+    await switchFetch('https://test.com/', null, null, {}, undefined);
     expect(clearTimeout).toBeCalledTimes(1);
   });
 });