Abort in-flight promises returned from useLazyQuery when component unmounts

[jerelmiller]

Closes #10365

Ensure in-flight promises returned from useLazyQuery's execute function are aborted when useLazyQuery unmounts. Currently the promise is stuck in a pending state indefinitely with no way to resolve/reject it.

Checklist:

• If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
• If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests

[changeset-bot]

🦋 Changeset detected

Latest commit: 33d2ef2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[alessbell]

If this is working in both React 17 + 18, ignore this comment 😄 Just noting I was seeing test failures when I tried moving the act import to RTL from react-dom/test-utils in a few tests that run against both versions.

[jerelmiller]

Interesting. I just rebased main and this seems to work just fine, so I think we are good!

[alessbell]

✅

[christiancho]

@jerelmiller Have you tested this with using suspense boundaries? My error boundary is catching the abort action and won't render the resource's data since the suspense boundary is un-mounting the component that is trying to render the data even if the suspender is still resolving.

[jerelmiller]

Hey @christiancho 👋

See my comment in #10520 (comment) for the reason I chose to let the promise reject without catching it inside useLazyQuery. I'd expect error boundaries to be rendered if you're not catching aborted errors on your end since this PR rejects the promise returned by useLazyQuery.

[christiancho]

@jerelmiller Thanks for the insight! We'll have to work around it.

[jerelmiller]

@christiancho no problem! As mentioned in that comment, just ensure your catch does something with error.name === 'AbortError' if you want to ignore that error.

If you use a lot of useLazyQuery and find repeating the same try/catch block is tedious and you prefer ignoring aborted errors as your default behavior, consider creating your own useLazyQuery wrapper hook that handles this. Here is a rough idea what that might look like.

import { useLazyQuery as useApolloLazyQuery } from '@apollo/client';

function useLazyQuery(...args) {
  const [execute, result] = useApolloLazyQuery(...args);

  async function executeWithCaughtAbortedError(...args) {
    try {
      return await execute(...args);
    } catch (error) {
      if (error.name === 'AbortError') {
        return // do nothing if aborted  
      }

      throw error
    }
  }

  return [executeWithCaughtAbortedError, result]
}

---

That being said, I wouldn't expect you to have to write this code super often as it should only be needed in components that have the ability to unmount while the request is in-flight. That is unless you have a lot of components that have a tendency to unmount 😄.

Hope this helps!

[christiancho]

The problem with this is that when suspending, the request gets aborted immediately, so the promise never resolves successfully. Here's the wrapper we're using for suspending the promise so the boundary can catch it:

export function wrappedFetch<T, K = undefined>(
  promise: (arg0: K | undefined) => Promise<T>,
  arg1?: K,
): Resource<T> {
  let status: API_STATUS = API_STATUS.PENDING;
  let result: T | Error | null = null;
  const suspender = promise(arg1)
    .then((response: T) => {
      console.log({ response });
      status = API_STATUS.SUCCESS;
      result = response;
    })
    .catch((err: Error) => {
      if (err.message === 'signal is aborted without reason' && err.name === 'AbortError') {
        return;
      }

      status = API_STATUS.ERROR;
      result = err;
    });

  return {
    read() {
      if (status === API_STATUS.PENDING) {
        throw suspender;
      } else if (status === API_STATUS.ERROR) {
        throw result;
      } else {
        return result as T;
      }
    },
  };
}

export function useGraphQLSuspense<
  TData = unknown,
  TVariables = OperationVariables,
>(
  query: DocumentNode,
  params: LazyQueryHookOptions<TData, TVariables> | undefined = undefined,
) {
  const [promiseReturner] = useLazyQuery(query, params);
  return useMemo<Resource<unknown>>(
    () => wrappedFetch(promiseReturner),
    [promiseReturner],
  );
}

[jerelmiller]

@christiancho right... suspense.... Let me think about this some more. I hadn't anticipated this being used in conjunction with a custom suspense solution. Would you be willing to open a new issue in regards to this behavior? I'd love to track this separately.