Repository: FormidableLabs/urql
Pull request #592 (2020-03-09 16:50:43)

Filter results that are being retried from retryExchange's output

<!--
  Thanks for opening a pull request! We appreciate your dedication and help!
  Before submitting your pull request, please make sure to read our CONTRIBUTING guide.

  The best contribution is always a PR! However, if you're starting to work on a large
  change, it's best to make sure to open an issue first.

  If this PR is already related to an issue, please reference it like so:
  Resolves #123
-->

## Summary

In preparation for getting  `retryExchange` ready to publish:

- `retryExchange` now only returns an error if the max number of retries have been exceeded
- `retryCount` have been moved to operation context (removes unnecessary typings)
- Update tests to reflect that only 1 result should be returned per operation
<!-- What's the motivation of this change? What does it solve? -->



changeset-bot[bot]: ###  💥  No Changeset

Latest commit: dc6198fa2e6d8017d0c6c5abb981f2eec681b724

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

**If these changes should be published to npm, you need to add a changeset.**

[Click here to learn what changesets are, and how to add one](https://github.com/changesets/changesets/blob/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/FormidableLabs/urql/new/retry-exchange-update?filename=.changeset/nasty-donkeys-wait.md&value=---%0A%22%40fake-scope%2Ffake-pkg%22%3A%20patch%0A---%0A%0AUpdate%20the%20package%20info%20above%20to%20the%20changes%20you%20want%2C%20and%20write%20your%20description%20here.%20You%20can%20add%20more%20packages%20on%20new%20lines.%0A)



Patch:
diff --git a/.circleci/config.yml b/.circleci/config.yml
index fe36b565..a269f862 100644
--- a/.circleci/config.yml
+++ b/.circleci/config.yml
@@ -106,7 +106,7 @@ jobs:
           name: Deploy Production Site
           command: |
             cd packages/site
-            node scripts/deploy/aws.js
+            node scripts/deploy/aws.js --dryrun
 
 workflows:
   version: 2
diff --git a/exchanges/retry/src/retryExchange.ts b/exchanges/retry/src/retryExchange.ts
index 1833a810..8fceeea4 100644
--- a/exchanges/retry/src/retryExchange.ts
+++ b/exchanges/retry/src/retryExchange.ts
@@ -4,7 +4,6 @@ import {
   pipe,
   merge,
   filter,
-  tap,
   fromValue,
   delay,
   mergeMap,
@@ -27,43 +26,32 @@ interface RetryExchangeOptions {
   retryIf?: (e: CombinedError) => boolean;
 }
 
-export interface OperationWithRetry extends Operation {
-  retryCount?: number;
-}
-
-export interface OperationResultWithRetry extends OperationResult {
-  operation: OperationWithRetry;
-}
-
 export const retryExchange = ({
   initialDelayMs,
   maxDelayMs,
   randomDelay,
   maxNumberAttempts,
-  retryIf,
+  retryIf: retryIfOption,
 }: RetryExchangeOptions): Exchange => {
   const MIN_DELAY = initialDelayMs || 1000;
   const MAX_DELAY = maxDelayMs || 15000;
-  const MAX_ATTEMPTS = maxNumberAttempts || Infinity;
+  const MAX_ATTEMPTS = maxNumberAttempts || 2;
   const RANDOM_DELAY = randomDelay || true;
 
-  const networkErrorOrRetryIf = err =>
-    (retryIf && retryIf(err)) || err.networkError;
+  const retryIf =
+    retryIfOption || ((err: CombinedError) => err && err.networkError);
 
   return ({ forward }) => ops$ => {
     const sharedOps$ = pipe(ops$, share);
     const { source: retry$, next: nextRetryOperation } = makeSubject<
-      OperationWithRetry
+      Operation
     >();
-    let maxNumberAttemptsExceeded = false;
 
     const retryWithBackoff$ = pipe(
       retry$,
-      mergeMap((op: OperationWithRetry) => {
-        const { key, context, retryCount } = op;
-        if (retryCount && retryCount > MAX_ATTEMPTS) {
-          maxNumberAttemptsExceeded = true;
-        }
+      mergeMap((op: Operation) => {
+        const { key, context } = op;
+        const retryCount = context.retryCount || 0;
         let delayAmount = context.retryDelay || MIN_DELAY;
 
         const backoffFactor = Math.random() + 1.5;
@@ -75,7 +63,7 @@ export const retryExchange = ({
 
         // We stop the retries if a teardown event for this operation comes in
         // But if this event comes through regularly we also stop the retries, since it's
-        // basically the query retrying itself, so no backoff should be added!
+        // basically the query retrying itself, no backoff should be added!
         const teardown$ = pipe(
           sharedOps$,
           filter(op => {
@@ -94,11 +82,9 @@ export const retryExchange = ({
             context: {
               ...op.context,
               retryDelay: delayAmount,
+              retryCount: retryCount + 1,
             },
-            retryCount: retryCount != null ? retryCount + 1 : 1,
           }),
-          filter(op => op.retryCount < MAX_ATTEMPTS),
-          // Here's the actual delay
           delay(delayAmount),
           // Stop retry if a teardown comes in
           takeUntil(teardown$)
@@ -109,29 +95,23 @@ export const retryExchange = ({
     const result$ = pipe(
       merge([sharedOps$, retryWithBackoff$]),
       forward,
-      share
-    ) as sourceT<OperationResultWithRetry>;
-
-    const successResult$ = pipe(
-      result$,
-      // We let through all results that don't fit the criteria
-      filter(res => !res.error || !networkErrorOrRetryIf(res.error))
-    );
-
-    const failedResult$ = pipe(
-      result$,
-      // Only retry if the error passes the conditional retryIf function (if passed)
-      // or if the error contains a networkError
-      filter(res => !!(res.error && networkErrorOrRetryIf(res.error))),
-      // Send failed responses to be retried by calling next on the retry$ subject
-      // Exclude operations that have been retried more than the specified max
-      tap(op => {
-        if (!maxNumberAttemptsExceeded) {
-          nextRetryOperation(op.operation);
+      share,
+      filter(res => {
+        const maxNumberAttemptsExceeded =
+          (res.operation.context.retryCount || 0) >= MAX_ATTEMPTS - 1;
+        // Only retry if the error passes the conditional retryIf function (if passed)
+        // or if the error contains a networkError
+        if (res.error && retryIf(res.error) && !maxNumberAttemptsExceeded) {
+          // Send failed responses to be retried by calling next on the retry$ subject
+          // Exclude operations that have been retried more than the specified max
+          nextRetryOperation(res.operation);
+          return false;
+        } else {
+          return true;
         }
       })
-    );
+    ) as sourceT<OperationResult>;
 
-    return merge([successResult$, failedResult$]);
+    return result$;
   };
 };
diff --git a/packages/site/scripts/deploy/aws.js b/packages/site/scripts/deploy/aws.js
index e6b95487..1fc395a3 100755
--- a/packages/site/scripts/deploy/aws.js
+++ b/packages/site/scripts/deploy/aws.js
@@ -1,6 +1,3 @@
-// TODO: Currently disabled to prevent oopsies
-process.exit(0);
-
 /**
  * Upload docs to appropriate s3 subdirectory.
  */


Test patch:
diff --git a/exchanges/retry/src/retryExchange.test.ts b/exchanges/retry/src/retryExchange.test.ts
index 4c3fd59b..1218885c 100644
--- a/exchanges/retry/src/retryExchange.test.ts
+++ b/exchanges/retry/src/retryExchange.test.ts
@@ -8,7 +8,7 @@ import {
   OperationResult,
   ExchangeIO,
 } from '@urql/core';
-import { retryExchange, OperationWithRetry } from './retryExchange';
+import { retryExchange } from './retryExchange';
 
 beforeEach(() => {
   jest.useFakeTimers();
@@ -57,17 +57,37 @@ beforeEach(() => {
     query: queryOne,
   });
 
-  ({ source: ops$, next } = makeSubject<OperationWithRetry>());
+  ({ source: ops$, next } = makeSubject<Operation>());
 });
 
-it('retries if it hits an error', () => {
+it(`retries if it hits an error and works for multiple concurrent operations`, () => {
+  const queryTwo = gql`
+    {
+      films {
+        id
+        name
+      }
+    }
+  `;
+  const queryTwoError = {
+    name: 'error2',
+    message: 'scary error2',
+  };
+  const opTwo = client.createRequestOperation('query', {
+    key: 2,
+    query: queryTwo,
+  });
+
   const response = jest.fn(
     (forwardOp: Operation): OperationResult => {
-      expect(forwardOp.key).toBe(op.key);
+      expect(
+        forwardOp.key === op.key || forwardOp.key === opTwo.key
+      ).toBeTruthy();
+
       return {
         operation: forwardOp,
         // @ts-ignore
-        error: queryOneError,
+        error: forwardOp.key === 2 ? queryTwoError : queryOneError,
       };
     }
   );
@@ -91,26 +111,39 @@ it('retries if it hits an error', () => {
   );
 
   next(op);
-  // Once for failed results, once for successful results
-  expect(mockRetryIf).toHaveBeenCalledTimes(2);
+
+  expect(mockRetryIf).toHaveBeenCalledTimes(1);
   expect(mockRetryIf).toHaveBeenCalledWith(queryOneError);
 
   jest.runAllTimers();
 
-  // max number of retries, plus original call
+  expect(mockRetryIf).toHaveBeenCalledTimes(mockOptions.maxNumberAttempts);
+
   expect(response).toHaveBeenCalledTimes(mockOptions.maxNumberAttempts);
-  expect(result).toHaveBeenCalledTimes(mockOptions.maxNumberAttempts);
+
+  // result should only ever be called once per operation
+  expect(result).toHaveBeenCalledTimes(1);
+
+  next(opTwo);
+
+  jest.runAllTimers();
+
+  expect(mockRetryIf).toHaveBeenCalledWith(queryTwoError);
+
+  // max number of retries for each op
+  expect(response).toHaveBeenCalledTimes(mockOptions.maxNumberAttempts * 2);
+  expect(result).toHaveBeenCalledTimes(2);
 });
 
 it('should retry x number of times and then return the successful result', () => {
   const numberRetriesBeforeSuccess = 3;
   const response = jest.fn(
-    (forwardOp: OperationWithRetry): OperationResult => {
+    (forwardOp: Operation): OperationResult => {
       expect(forwardOp.key).toBe(op.key);
       // @ts-ignore
       return {
         operation: forwardOp,
-        ...(forwardOp.retryCount! >= numberRetriesBeforeSuccess
+        ...(forwardOp.context.retryCount! >= numberRetriesBeforeSuccess
           ? { data: queryOneData }
           : { error: queryOneError }),
       };
@@ -138,12 +171,12 @@ it('should retry x number of times and then return the successful result', () =>
   next(op);
   jest.runAllTimers();
 
-  expect(mockRetryIf).toHaveBeenCalledTimes(numberRetriesBeforeSuccess * 2);
+  expect(mockRetryIf).toHaveBeenCalledTimes(numberRetriesBeforeSuccess);
   expect(mockRetryIf).toHaveBeenCalledWith(queryOneError);
 
   // one for original source, one for retry
   expect(response).toHaveBeenCalledTimes(1 + numberRetriesBeforeSuccess);
-  expect(result).toHaveBeenCalledTimes(1 + numberRetriesBeforeSuccess);
+  expect(result).toHaveBeenCalledTimes(1);
 });
 
 it(`should still retry if retryIf undefined but there is a networkError`, () => {
@@ -185,5 +218,5 @@ it(`should still retry if retryIf undefined but there is a networkError`, () =>
 
   // max number of retries, plus original call
   expect(response).toHaveBeenCalledTimes(mockOptions.maxNumberAttempts);
-  expect(result).toHaveBeenCalledTimes(mockOptions.maxNumberAttempts);
+  expect(result).toHaveBeenCalledTimes(1);
 });