Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

useLazyQuery Uncaught (in promise)DOMException: signal is aborted without reason #10520

Closed
zouzhe66 opened this issue Feb 6, 2023 · 17 comments · Fixed by #10698
Closed

useLazyQuery Uncaught (in promise)DOMException: signal is aborted without reason #10520

zouzhe66 opened this issue Feb 6, 2023 · 17 comments · Fixed by #10698
Assignees

Comments

@zouzhe66
Copy link

zouzhe66 commented Feb 6, 2023

Uncaught (in promise)DOMException: signal is aborted without reason

these below code maybe cause the exception,can we catch the exception?

abortControllersRef.current.forEach((controller) => {
       controller.abort();
     });
  asyncUpdate(signal: AbortSignal) {
    return new Promise<QueryResult<TData, TVariables>>((resolve, reject) => {
      const watchQueryOptions = this.watchQueryOptions;

      const handleAborted = () => {
        this.asyncResolveFns.delete(resolve)
        this.optionsToIgnoreOnce.delete(watchQueryOptions);
        signal.removeEventListener('abort', handleAborted)
        reject(signal.reason);
      };

      this.asyncResolveFns.add(resolve);
      this.optionsToIgnoreOnce.add(watchQueryOptions);
      signal.addEventListener('abort', handleAborted)
      this.forceUpdate();
    });
  }
@jerelmiller
Copy link
Member

jerelmiller commented Feb 6, 2023

Hey @q234876317 👋!

The code you're looking at fixed an issue where unmounting a component with an in-flight request caused that promise to hang indefinitely (#10365). The controller.abort() call itself does not actually throw the exception, what you're seeing is the exception from the rejected promise (i.e. reject(signal.reason)), so even if we were to wrap the controller.abort() in a try/catch, it would have no effect.

I'll note that we do "catch" this error in a way that avoids uncaught promise warnings, but we float it so that errors are still propagated to the user.

One of the biggest reasons I chose not to handle the aborted error in useLazyQuery is because I wanted the user's "success handling" code to be able to use a successful result returned from the query, rather than needing to handle either the success result, or the aborted result.

To illustrate this point, let's say Apollo Client updated useLazyQuery to look like this:

// https://github.com/apollographql/apollo-client/blob/015a1ffff3febe4604956f4bb137a3111f3d8257/src/react/hooks/useLazyQuery.ts#L95-L107

const promise = internalState
  .asyncUpdate(controller.signal)
  .then(queryResult => {
    abortControllersRef.current.delete(controller);

    return Object.assign(queryResult, eagerMethods);
  });

promise.catch(() => {
  abortControllersRef.current.delete(controller);
});

return promise.catch((error) => {
  // Do nothing if we are catching an abort, otherwise re-throw to avoid swallowing errors
  if (e.name !== 'AbortError') {
    throw error;
  }
});

In this case, because catch returns a new promise that resolves with the return value from its callback, this would mean the value resolved from our catch would be undefined. This forces the developer to have to deal with this in their success handling code. For example:

const [execute] = useLazyQuery(QUERY);

try {
  const result = await execute();

  if (typeof result !== 'undefined') {
    const { data } = result;

    // do something with `data`
  }
} catch (e) {
  // deal with a non-aborted error
}

I find this adds significantly more noise to the success handling code than if we let the user decide how to handle aborted errors.

By rejecting the promise, I find this fits the mental model as well. A component that unmounts while a query is fetched truly is exceptional! We aren't expected to get a successful result because the component isn't around to do anything with it. We'd rather you deal with this in a way that makes the most sense for your app. Perhaps unmounting while fetching actually does mean something in your app that might not in others.


Instead, if you want to ignore this error, wrap the returned promise in a try/catch and check for error.name === 'AbortError';

const [execute] = useLazyQuery(QUERY);

try {
  await execute();
} catch (error) {
  if (error.name !== 'AbortError') {
    // handle the non-abort error in some way
  }
}

@zouzhe66
Copy link
Author

zouzhe66 commented Feb 8, 2023

well done, thanks a lot!

@zouzhe66 zouzhe66 closed this as completed Feb 8, 2023
@guicara
Copy link

guicara commented Feb 18, 2023

Hello,

I have also the same issue after upgrading the @apollo/client library from version 3.7.1 to 3.7.8 when using useLazyQuery (probably the same with useQuery?).

The bug was introduced with version 3.7.4.

Here is the stack trace for reference:

useLazyQuery.ts:78 Uncaught (in promise) DOMException: signal is aborted without reason
    at http://localhost:3000/node_modules/.vite/deps/@apollo_client.js?v=d5c2e0d9:8702:20
    at Set.forEach (<anonymous>)
    at http://localhost:3000/node_modules/.vite/deps/@apollo_client.js?v=d5c2e0d9:8701:35
    at safelyCallDestroy (http://localhost:3000/node_modules/.vite/deps/chunk-JZ3YVIXN.js?v=8247418e:16737:13)
    at commitHookEffectListUnmount (http://localhost:3000/node_modules/.vite/deps/chunk-JZ3YVIXN.js?v=8247418e:16864:19)
    at invokePassiveEffectUnmountInDEV (http://localhost:3000/node_modules/.vite/deps/chunk-JZ3YVIXN.js?v=8247418e:18359:19)
    at invokeEffectsInDev (http://localhost:3000/node_modules/.vite/deps/chunk-JZ3YVIXN.js?v=8247418e:19697:19)
    at commitDoubleInvokeEffectsInDEV (http://localhost:3000/node_modules/.vite/deps/chunk-JZ3YVIXN.js?v=8247418e:19678:15)
    at flushPassiveEffectsImpl (http://localhost:3000/node_modules/.vite/deps/chunk-JZ3YVIXN.js?v=8247418e:19499:13)
    at flushPassiveEffects (http://localhost:3000/node_modules/.vite/deps/chunk-JZ3YVIXN.js?v=8247418e:19443:22)

@jerelmiller you have merged PR #10427 but it doesn't seem to fix the issue (if it's the same).

@asgeo1
Copy link

asgeo1 commented Feb 27, 2023

I'm also having an issue here, and don't fully understand how this is marked resolved.

In my tests, using @apollo/client 3.7.9, and React 18.0.0, I get an Uncaught (in promise)DOMException: signal is aborted without reason error pretty much any time I use useLazyQuery in a component.

This seems to be because of React v18's reactStrictMode, which causes components in dev mode to always unmount and re-render. See: vercel/next.js#35822

So I don't see how there's anyway to use useLazyQuery and reactStrictMode: true together, without triggering this error all the time ?

A component that unmounts while a query is fetched truly is exceptional!

I'm not sure, seems to happen all the time with latest React v18 and reactStrictMode: true

Surely we don't want to wrap everything in try/catch and check for error.name === 'AbortError' every time we use useLazyQuery ? It seems odd.

I know I could probably create my own version of useLazyQuery which by default ignores these errors (like @jerelmiller suggested here #10427 (comment))

So I'll either do that, or downgraded to 3.7.3, as I don't want to disable reactStrictMode.

But just trying to understand whether this really is intended behaviour or not for React 18 and reactStrictMode: true

@70nyIT
Copy link

70nyIT commented Mar 15, 2023

agree with @asgeo1 . I have this issue too. It fires not when there's a query running, but every time I have a component with useLazyQuery implemented in it

@jerelmiller
Copy link
Member

@asgeo1 I'm curious, how are you executing the query in strict mode? What you describe seems to imply that you're executing the query in a useEffect, which I'd argue is generally a code smell. You'd likely be better off using useQuery in that case.


That being said, there are some legitimate cases for why this behavior is not ideal. I am considering a different approach that will instead let the promise resolve, but if the component is unmounted, the value will just disappear into the void (i.e. we won't try and update state). I think this would help in this situation.

@guicara
Copy link

guicara commented Mar 15, 2023

@jerelmiller in our project we use useLazyQuery most of the time. it allows us to control when to trigger the query or mutation. In the following example, we have a custom hook to fetch some data. The fetchUsers method is then called in a useEffect hook, or when a user clicks on a button (for example).

import { useLazyQuery } from '@apollo/client';
import { GET_USERS } from './query';

interface QueryData {
  users: User[];
}

interface QueryVars {
  sortBy: string;
  query: Pick<UserQueryInput, '_partition'>;
}

function useFetchUsers() {
  const [execute, { loading, error, data, refetch, called }] = useLazyQuery<QueryData, QueryVars>(
    GET_USERS,
    {
      notifyOnNetworkStatusChange: true,
    },
  );

  const fetchUsers = async (sortBy?: SortBy) => {
    const query: QueryVars = {
      query: { ... },
      sortBy,
    };

    const response = called
      ? await refetch(query)
      : await execute({
          variables: query,
        });

    return response.data?.users || [];
  };

  return { fetchUsers, loading, error, users: data?.users || [] };
}

export default useFetchUsers;
function Users() {
  const { fetchUsers, users, loading, error } = useFetchUsers();

  const [sortModel, setSortModel] = useState<GridSortModel>(defaultSortModel);

  const handleReload = () => {
    fetchUsers(sortModel[0] as SortBy);
  };

  useEffect(() => {
    if (!loading) {
      handleReload();
    }
  }, [users]);

  return (
      <Fragment>
        <button onClick={handleReload}>Reload</button>
        <UsersTable users={users} loading={loading} />
      </Fragment>
  );
}

export default Users;

@clementAC
Copy link

I have the same problem in production so strict mode may not be the culprit. I have two types of AbortError:

  • signal is aborted without reason
  • The operation was aborted

@70nyIT
Copy link

70nyIT commented Mar 23, 2023

So the reason why this is kept "Closed" is because this is not considered a Bug by the team? Just asking so that I know how to manage codes

@guicara
Copy link

guicara commented Mar 23, 2023

For me, it's a bug AND a breaking change. A minor version upgrade should not cause something as important!

@70nyIT
Copy link

70nyIT commented Mar 23, 2023

I agree with @guicara . This is why I was asking the reason for this issue being market as "Closed"

@guicara
Copy link

guicara commented Mar 23, 2023

What are the next steps @jerelmiller?

@jerelmiller
Copy link
Member

@70nyIT I've kept this closed because I'm going to track this in #10533 which is a pretty compelling case to change the behavior. I'd recommend following that issue to keep track of updates.

@guicara As mentioned in #10520 (comment), I'm planning on changing the behavior. We've got this issue queued up for us to look at, so it will get some attention in the near term.

@jerelmiller
Copy link
Member

I've changed my mind. I'm going to reopen this issue so that I can tag it in my PR. This will close again when the PR is merged.

@jerelmiller
Copy link
Member

Hey all 👋

We just released v3.7.11 that contains a change to the behavior of useLazyQuery that will now allow the promise to resolve naturally rather than rejecting with an abort error when a component unmounts. I hope this works better for you!

@70nyIT
Copy link

70nyIT commented Apr 1, 2023

Confirmed working as expected! Thanks @jerelmiller

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2023

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For general questions, we recommend using StackOverflow or our discord server.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants