Repository: FormidableLabs/urql
Pull request #1880 (2021-08-22 23:00:52)

(core) - update toPromise to exclude `hasNext` results
## Summary

This change ensures that when we call `toPromise()` on a query we wont serve an incomplete result, the user will expect to receive a non-stale full-result when using `toPromise()`

## Set of changes

- Account for `hasNext` in `toPromise`


changeset-bot[bot]: ###  ⚠️  No Changeset found

Latest commit: 0e476b25acfe2b2baa4bee269ae1601d61980f09

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. **If these changes should result in a version bump, you need to add a changeset.**

<details><summary>This PR includes no changesets</summary>

  When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

</details>

[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/defer-stream-to-promise?filename=.changeset/thick-seals-wait.md&value=---%0A%22%40urql%2Fcore%22%3A%20patch%0A---%0A%0A(core)%20-%20update%20toPromise%20to%20exclude%20%60hasNext%60%20results%0A)


kitten: @wgolledge I was extremely unsure about that one. Mainly, because the fields that are deferred will initially be `undefined` rather than `null`, so clearly identifiable and distinct from stale results and partial results. We did also get feedback that at times it's hard to handle partial results. But with `useFragment` maybe beoming a thing, this may not be that severe 🤔

The other consideration here are `@live` queries for which `hasNext` may be `true` indefinitely. So both are really good points 
wgolledge: Cheers for the detailed explanation @kitten! Sounds like it's helpful to keep track of both. The other slight query I had is if there's ever a scenario when we wouldn't want to exclude the operation even if it `hasNext`, though I'm guessing not
JoviDeCroock: > Cheers for the detailed explanation @kitten! Sounds like it's helpful to keep track of both. The other slight query I had is if there's ever a scenario when we wouldn't want to exclude the operation even if it `hasNext`, though I'm guessing not

not in the case of `toPromise`, the reason for this being that if a consumer of urql does `client.query().toPromise()` they expect a full-response rather than something being streamed in, else they'd have to use `subscribe()` to get to a full result

Patch:
diff --git a/.changeset/mean-pugs-laugh.md b/.changeset/mean-pugs-laugh.md
new file mode 100644
index 00000000..28f57212
--- /dev/null
+++ b/.changeset/mean-pugs-laugh.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Fix previous results' `null` values spilling into the next result that Graphcache issues, which may prevent updates from being issued until the query is reexecuted. This was affecting any `null` links on data, and any queries that were issued before non-optimistic mutations.
diff --git a/.changeset/stale-candles-enjoy.md b/.changeset/stale-candles-enjoy.md
new file mode 100644
index 00000000..0a70237b
--- /dev/null
+++ b/.changeset/stale-candles-enjoy.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-retry': minor
+---
+
+Add a new `retryWith` option which allows operations to be updated when a request is being retried.
diff --git a/.changeset/thick-seals-wait.md b/.changeset/thick-seals-wait.md
new file mode 100644
index 00000000..59844d10
--- /dev/null
+++ b/.changeset/thick-seals-wait.md
@@ -0,0 +1,7 @@
+---
+"@urql/core": patch
+---
+
+Fix: update toPromise to exclude `hasNext` results. This change ensures that
+when we call toPromise() on a query we wont serve an incomplete result, the
+user will expect to receive a non-stale full-result when using toPromise()
diff --git a/exchanges/graphcache/src/operations/query.ts b/exchanges/graphcache/src/operations/query.ts
index a9d56aee..f6be40a7 100644
--- a/exchanges/graphcache/src/operations/query.ts
+++ b/exchanges/graphcache/src/operations/query.ts
@@ -66,7 +66,7 @@ export const query = (
   error?: CombinedError | undefined,
   key?: number
 ): QueryResult => {
-  initDataState('read', store.data, (data && key) || null);
+  initDataState('read', store.data, key);
   const result = read(store, request, data, error);
   clearDataState();
   return result;
@@ -121,21 +121,23 @@ const readRoot = (
   ctx: Context,
   entityKey: string,
   select: SelectionSet,
-  data: Data
+  input: Data
 ): Data => {
-  const typename = ctx.store.rootNames[entityKey] ? entityKey : data.__typename;
+  const typename = ctx.store.rootNames[entityKey]
+    ? entityKey
+    : input.__typename;
   if (typeof typename !== 'string') {
-    return data;
+    return input;
   }
 
   const iterate = makeSelectionIterator(entityKey, entityKey, select, ctx);
 
   let node: FieldNode | void;
   let hasChanged = false;
-  const output = makeData(data);
+  const output = makeData(input);
   while ((node = iterate())) {
     const fieldAlias = getFieldAlias(node);
-    const fieldValue = output[fieldAlias];
+    const fieldValue = input[fieldAlias];
     // Add the current alias to the walked path before processing the field's value
     ctx.__internal.path.push(fieldAlias);
     // We temporarily store the data field in here, but undefined
@@ -152,14 +154,14 @@ const readRoot = (
     }
 
     // Check for any referential changes in the field's value
-    hasChanged = hasChanged || dataFieldValue !== output[fieldAlias];
+    hasChanged = hasChanged || dataFieldValue !== fieldValue;
     if (dataFieldValue !== undefined) output[fieldAlias] = dataFieldValue!;
 
     // After processing the field, remove the current alias from the path again
     ctx.__internal.path.pop();
   }
 
-  return hasChanged ? output : data;
+  return hasChanged ? output : input;
 };
 
 const readRootField = (
@@ -258,7 +260,7 @@ const readSelection = (
   ctx: Context,
   key: string,
   select: SelectionSet,
-  data: Data,
+  input: Data,
   result?: Data
 ): Data | undefined => {
   const { store } = ctx;
@@ -303,9 +305,9 @@ const readSelection = (
 
   let hasFields = false;
   let hasPartials = false;
-  let hasChanged = typename !== data.__typename;
+  let hasChanged = typename !== input.__typename;
   let node: FieldNode | void;
-  const output = makeData(data);
+  const output = makeData(input);
   while ((node = iterate()) !== undefined) {
     // Derive the needed data from our node.
     const fieldName = getName(node);
@@ -340,7 +342,7 @@ const readSelection = (
     ) {
       // We have to update the information in context to reflect the info
       // that the resolver will receive
-      updateContext(ctx, data, typename, entityKey, key, fieldName);
+      updateContext(ctx, output, typename, entityKey, key, fieldName);
 
       // We have a resolver for this field.
       // Prepare the actual fieldValue, so that the resolver can use it
@@ -364,9 +366,11 @@ const readSelection = (
           fieldName,
           key,
           getSelectionSet(node),
-          output[fieldAlias] as Data,
+          (output[fieldAlias] !== undefined
+            ? output[fieldAlias]
+            : input[fieldAlias]) as Data,
           dataFieldValue,
-          ownsData(output)
+          ownsData(input)
         );
       }
 
@@ -390,9 +394,11 @@ const readSelection = (
         fieldName,
         key,
         getSelectionSet(node),
-        output[fieldAlias] as Data,
+        (output[fieldAlias] !== undefined
+          ? output[fieldAlias]
+          : input[fieldAlias]) as Data,
         resultValue,
-        ownsData(output)
+        ownsData(input)
       );
     } else {
       // Otherwise we attempt to get the missing field from the cache
@@ -405,8 +411,10 @@ const readSelection = (
           typename,
           fieldName,
           getSelectionSet(node),
-          output[fieldAlias] as Data,
-          ownsData(output)
+          (output[fieldAlias] !== undefined
+            ? output[fieldAlias]
+            : input[fieldAlias]) as Data,
+          ownsData(input)
         );
       } else if (typeof fieldValue === 'object' && fieldValue !== null) {
         // The entity on the field was invalid but can still be recovered
@@ -440,7 +448,7 @@ const readSelection = (
     // After processing the field, remove the current alias from the path again
     ctx.__internal.path.pop();
     // Check for any referential changes in the field's value
-    hasChanged = hasChanged || dataFieldValue !== output[fieldAlias];
+    hasChanged = hasChanged || dataFieldValue !== input[fieldAlias];
     if (dataFieldValue !== undefined) output[fieldAlias] = dataFieldValue;
   }
 
@@ -449,7 +457,7 @@ const readSelection = (
     ? undefined
     : hasChanged
     ? output
-    : data;
+    : input;
 };
 
 const resolveResolverResult = (
@@ -564,7 +572,7 @@ const resolveLink = (
     }
 
     return hasChanged ? newLink : (prevData as Data[]);
-  } else if (link === null || (prevData === null && ownsData)) {
+  } else if (link === null || (prevData === null && skipNull)) {
     return null;
   }
 
diff --git a/exchanges/graphcache/src/store/data.ts b/exchanges/graphcache/src/store/data.ts
index 174decb0..a9dc9d8e 100644
--- a/exchanges/graphcache/src/store/data.ts
+++ b/exchanges/graphcache/src/store/data.ts
@@ -92,7 +92,7 @@ export const ownsData = (data?: Data): boolean =>
 export const initDataState = (
   operationType: OperationType,
   data: InMemoryData,
-  layerKey: number | null,
+  layerKey?: number | null,
   isOptimistic?: boolean
 ) => {
   currentOwnership = new Set();
diff --git a/exchanges/retry/src/retryExchange.ts b/exchanges/retry/src/retryExchange.ts
index a0e60d0b..1fcc2823 100644
--- a/exchanges/retry/src/retryExchange.ts
+++ b/exchanges/retry/src/retryExchange.ts
@@ -25,6 +25,11 @@ export interface RetryExchangeOptions {
   maxNumberAttempts?: number;
   /** Conditionally determine whether an error should be retried */
   retryIf?: (error: CombinedError, operation: Operation) => boolean;
+  /** Conditionally update operations as they're retried (retryIf can be replaced with this) */
+  retryWith?: (
+    error: CombinedError,
+    operation: Operation
+  ) => Operation | null | undefined;
 }
 
 export const retryExchange = ({
@@ -32,16 +37,14 @@ export const retryExchange = ({
   maxDelayMs,
   randomDelay,
   maxNumberAttempts,
-  retryIf: retryIfOption,
+  retryIf,
+  retryWith,
 }: RetryExchangeOptions): Exchange => {
   const MIN_DELAY = initialDelayMs || 1000;
   const MAX_DELAY = maxDelayMs || 15000;
   const MAX_ATTEMPTS = maxNumberAttempts || 2;
   const RANDOM_DELAY = randomDelay || true;
 
-  const retryIf =
-    retryIfOption || ((err: CombinedError) => err && err.networkError);
-
   return ({ forward, dispatchDebug }) => ops$ => {
     const sharedOps$ = pipe(ops$, share);
     const {
@@ -107,7 +110,12 @@ export const retryExchange = ({
       filter(res => {
         // 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, res.operation)) {
+        if (
+          !res.error ||
+          (retryIf
+            ? !retryIf(res.error, res.operation)
+            : !retryWith && !res.error.networkError)
+        ) {
           return true;
         }
 
@@ -115,9 +123,14 @@ export const retryExchange = ({
           (res.operation.context.retryCount || 0) >= MAX_ATTEMPTS - 1;
 
         if (!maxNumberAttemptsExceeded) {
+          const operation = retryWith
+            ? retryWith(res.error, res.operation)
+            : res.operation;
+          if (!operation) return true;
+
           // 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);
+          nextRetryOperation(operation);
           return false;
         }
 
diff --git a/packages/core/src/utils/streamUtils.ts b/packages/core/src/utils/streamUtils.ts
index f7c7ce8c..b2fabd81 100644
--- a/packages/core/src/utils/streamUtils.ts
+++ b/packages/core/src/utils/streamUtils.ts
@@ -7,7 +7,7 @@ export function withPromise<T extends OperationResult>(
   (source$ as PromisifiedSource<T>).toPromise = () => {
     return pipe(
       source$,
-      filter(result => !result.stale),
+      filter(result => !result.stale && !result.hasNext),
       take(1),
       toPromise
     );


Test patch:
diff --git a/exchanges/retry/src/retryExchange.test.ts b/exchanges/retry/src/retryExchange.test.ts
index 7cf3d224..9b9248df 100644
--- a/exchanges/retry/src/retryExchange.test.ts
+++ b/exchanges/retry/src/retryExchange.test.ts
@@ -3,6 +3,7 @@ import { pipe, map, makeSubject, publish, tap } from 'wonka';
 import {
   gql,
   createClient,
+  makeOperation,
   Operation,
   OperationResult,
   ExchangeIO,
@@ -227,3 +228,105 @@ it(`should still retry if retryIf undefined but there is a networkError`, () =>
   expect(response).toHaveBeenCalledTimes(mockOptions.maxNumberAttempts);
   expect(result).toHaveBeenCalledTimes(1);
 });
+
+it('should allow retryWhen to return falsy value and act as replacement of retryIf', () => {
+  const errorWithNetworkError = {
+    ...queryOneError,
+    networkError: 'scary network error',
+  };
+  const response = jest.fn(
+    (forwardOp: Operation): OperationResult => {
+      expect(forwardOp.key).toBe(op.key);
+      return {
+        operation: forwardOp,
+        // @ts-ignore
+        error: errorWithNetworkError,
+      };
+    }
+  );
+
+  const result = jest.fn();
+  const forward: ExchangeIO = ops$ => {
+    return pipe(ops$, map(response));
+  };
+
+  const retryWith = jest.fn(() => null);
+
+  pipe(
+    retryExchange({
+      ...mockOptions,
+      retryIf: undefined,
+      retryWith,
+    })({
+      forward,
+      client,
+      dispatchDebug,
+    })(ops$),
+    tap(result),
+    publish
+  );
+
+  next(op);
+
+  jest.runAllTimers();
+
+  // max number of retries, plus original call
+  expect(retryWith).toHaveBeenCalledTimes(1);
+  expect(response).toHaveBeenCalledTimes(1);
+  expect(result).toHaveBeenCalledTimes(1);
+});
+
+it('should allow retryWhen to return new operations when retrying', () => {
+  const errorWithNetworkError = {
+    ...queryOneError,
+    networkError: 'scary network error',
+  };
+  const response = jest.fn(
+    (forwardOp: Operation): OperationResult => {
+      expect(forwardOp.key).toBe(op.key);
+      return {
+        operation: forwardOp,
+        // @ts-ignore
+        error: errorWithNetworkError,
+      };
+    }
+  );
+
+  const result = jest.fn();
+  const forward: ExchangeIO = ops$ => {
+    return pipe(ops$, map(response));
+  };
+
+  const retryWith = jest.fn((_error, operation) => {
+    return makeOperation(operation.kind, operation, {
+      ...operation.context,
+      counter: (operation.context?.counter || 0) + 1,
+    });
+  });
+
+  pipe(
+    retryExchange({
+      ...mockOptions,
+      retryIf: undefined,
+      retryWith,
+    })({
+      forward,
+      client,
+      dispatchDebug,
+    })(ops$),
+    tap(result),
+    publish
+  );
+
+  next(op);
+
+  jest.runAllTimers();
+
+  // max number of retries, plus original call
+  expect(retryWith).toHaveBeenCalledTimes(mockOptions.maxNumberAttempts - 1);
+  expect(response).toHaveBeenCalledTimes(mockOptions.maxNumberAttempts);
+  expect(result).toHaveBeenCalledTimes(1);
+
+  expect(response.mock.calls[1][0]).toHaveProperty('context.counter', 1);
+  expect(response.mock.calls[2][0]).toHaveProperty('context.counter', 2);
+});