useMutation.mutate() promise doesnt catch errors and always fulfills with undefined data about apollo-feature-requests HOT 19 OPEN

I managed to work around the problem with my own mutation wrapper

export function useGraphqlMutation({ query, callbacks, options = {} }) {
    const [error, setError] = useState(null);
    const [loading, setLoading] = useState(false);
    const [promise, setPromise] = useState(null);
    useDebugValue(`loading: ${loading}`);


    function getResponse(res) {
        if (callbacks?.success && typeof callbacks.success === 'function') {
            callbacks.success(res);
        }
        promise.resolve(res);
        setLoading(false);
    }

    function getError(err) {
        setError(err);
        setLoading(false);
        if (callbacks?.error && typeof callbacks.error === 'function') {
            callbacks.error(err);
        }
        promise.reject(err);
    }

    const [executeQuery, { data }] = useMutation(
        query,
        {
            onCompleted: getResponse,
            onError: getError,
            ...options,
        },
    );

    function run(executionData) {
        executeQuery(executionData);
        return new Promise((resolve, reject) => {
            setPromise({ resolve, reject });
        });
    }

    return {
        run,
        data,
        error,
        loading,
    };
}

Using this custom hook adds the expected behavior to the useMutation, I think that this should be fixed, because it doesn't have any sense to respond with a promise and always fulfill without considering the result of the mutation.

from apollo-feature-requests.

Yes, I'm familiar with this approach, but in the specific case I'm working on I have several mutations running in parallel and managing that would be dirty and complicated. The promise approach fits my needs without adding extra layers to control which request has succeeded and which ones have failed.

from apollo-feature-requests.

Did the company behind apollo fail or something? This feels like a fairly critical bug. You call a mutation, it hits a network error as a part of that mutation and instead of rejecting the promise it blows up????

Feels like a pretty major issue to just sit unfixed for so long, do they even work on this repo any more?

from apollo-feature-requests.

Version: [email protected]

If you have options where errorPolicy: 'all' the promise will resolve with { errors }. However, for some reason, any onError option won't be called. I've also noticed that onCompleted is always called even if errors occur, resulting in data being undefined. This isn't very obvious from the docs https://www.apollographql.com/docs/tutorial/mutations/#persist-the-users-token-and-id
I was expecting that onCompleted would just be the result (including errors and data) not entirely sure why though.

When errorPolicy is not set or set to none

1. useMutation()[1].errors is not populated with "network errors".
2. await useMutation()[0]() promise calls result in undefined. This means when "network errors"
   or "graphql errors" occur they force the return to be undefined (they don't reject the promise).
3. onError is only called for "network errors"
4. onCompleted is only called when no "network errors" or "graphql errors" occur

When errorPolicy is set to all

1. useMutation()[1].errors is populated with "network errors".
2. await useMutation()[0]() promise calls result with errors defined. This means when "network errors"
   or "graphql errors" occur the return will include { errors } (they don't reject the promise).
3. onError is never called for "network errors" or "graphql errors"
   Instead of using onError use the errors return from either useMutation()[1].errors or if you
   need more control (await useMutation()[0]()).errors.
4. onCompleted is always called even when "network errors" or "graphql errors" occur.
   This means that onCompleted really means after we've completed the mutation do this not when
   the mutation is successful.

from apollo-feature-requests.

This seems like a pretty serious oversight. Has no one started addressing this yet?

from apollo-feature-requests.

over 2 years old, and not a single response from any maintainer.

EDIT: In my case I had a link which didn't properly handle observer errors leading to the uncaught error. After the link was patched I could catch the errors again.

from apollo-feature-requests.

Following the above suggestions by @oscmedgon and @Nate-Wilkins, I've come up with a wrapper that keeps the same shape as the original useMutation hook.

Without Typings (see below for Typescript):

import {useCallback} from "react";
import {ApolloError, useMutation} from "@apollo/client";

export const useGqlMutation = (mutation, options = {}) => {

    const { onCompleted, onError, ...otherOptions } = options || {};

    const [executeQuery, mutationResult] = useMutation(mutation, {
        ...otherOptions,
        errorPolicy: 'all' // NOTE: populate all errors
    });

    const executeQueryWrapper = useCallback(async (options) => {
        const result = await executeQuery(options as any);
        if (result?.errors?.length) {
            if (onError) {
                onError(new ApolloError({ graphQLErrors: result.errors }))
            }
            throw result.errors;
        }
        // only call `onCompleted` if there was no error
        if (onCompleted) {
            onCompleted(result?.data);
        }
        return result;
    }, [executeQuery, onCompleted, onError]);

    return [executeQueryWrapper, mutationResult];
}

import {useCallback} from "react";
import {DocumentNode} from "graphql";
import {ApolloCache, DefaultContext, OperationVariables} from "@apollo/client/core";
import {ApolloError, TypedDocumentNode, useMutation} from "@apollo/client";
import {FetchResult} from "@apollo/client/link/core/types";
import {MutationFunctionOptions, MutationHookOptions, MutationTuple} from "@apollo/client/react/types/types";

export const useGqlMutation = <TData = any, TVariables = OperationVariables, TContext = DefaultContext, TCache extends ApolloCache<any> = ApolloCache<any>>
    (mutation: DocumentNode | TypedDocumentNode<TData, TVariables>, options?: MutationHookOptions<TData, TVariables, TContext>)
    : MutationTuple<TData, TVariables, TContext, TCache> => {

    const { onCompleted, onError, ...otherOptions } = options as any || {};

    const [executeQuery, mutationResult] = useMutation<TData>(mutation, {
        ...otherOptions,
        errorPolicy: 'all' // NOTE: populate all errors
    });

    const executeQueryWrapper = useCallback(async (options?: MutationFunctionOptions<TData, TVariables, TContext, TCache>): Promise<FetchResult<TData>> => {
        const result = await executeQuery(options as any);
        if (result?.errors?.length) {
            if (onError) {
                onError(new ApolloError({ graphQLErrors: result.errors }))
            }
            throw result.errors;
        }
        // only call `onCompleted` if there was no error
        if (onCompleted) {
            onCompleted(result?.data);
        }
        return result;
    }, [executeQuery, onCompleted, onError]);

    return [executeQueryWrapper, mutationResult];
}

This will catch both onError scenarios and promise errors.

Example usage:

const MY_ACTION = gql`
  mutation MyMutationAction {
    ....
  }
`;

const TestComponent = () => {
    const [myAction, {loading}] = useGqlMutation(MY_ACTION, {
        onError: (errors) => console.log('onError', errors),
        onCompleted: (data) => console.log('onCompleted', data),
    });
    
    const handleClick = () => {
        myAction()
            .then((data) => console.log('promise resolved', data))
            .catch((errors) => console.log('promise rejected', errors));
    }
    
    return (
        <button disabled={loading} onClick={handleClick}>Test Mutation</button>
    );
}

from apollo-feature-requests.

Still getting this nasty bug in @apollo/[email protected] !!

from apollo-feature-requests.

When I migrate to Apollo 3.0 I had to use onCompleted:

MyComponent() {
    const [executeQuery, { data, error }] = useMutation(MUTATION, {
      onCompleted: () => { // I run after mutation is completed}
    });
    if (error) console.error(error)
    function onSendData() {
        executeQuery({variables: {myData: undefined}})
    }
    return(
        <h1>I'm not important<h1>
    )
}

from apollo-feature-requests.

I managed to work around the problem with my own mutation wrapper

export function useGraphqlMutation({ query, callbacks, options = {} }) {
    const [error, setError] = useState(null);
    const [loading, setLoading] = useState(false);
    const [promise, setPromise] = useState(null);
    useDebugValue(`loading: ${loading}`);


    function getResponse(res) {
        if (callbacks?.success && typeof callbacks.success === 'function') {
            callbacks.success(res);
        }
        promise.resolve(res);
        setLoading(false);
    }

    function getError(err) {
        setError(err);
        setLoading(false);
        if (callbacks?.error && typeof callbacks.error === 'function') {
            callbacks.error(err);
        }
        promise.reject(err);
    }

    const [executeQuery, { data }] = useMutation(
        query,
        {
            onCompleted: getResponse,
            onError: getError,
            ...options,
        },
    );

    function run(executionData) {
        executeQuery(executionData);
        return new Promise((resolve, reject) => {
            setPromise({ resolve, reject });
        });
    }

    return {
        run,
        data,
        error,
        loading,
    };
}

Using this custom hook adds the expected behavior to the useMutation, I think that this should be fixed, because it doesn't have any sense to respond with a promise and always fulfill without considering the result of the mutation.

Nice thanks for sharing.

from apollo-feature-requests.

+1 to the problem above. The mutation promise should either reject or resolve with an error in case of errors.

from apollo-feature-requests.

Another temporary solution might be:

    try {
      const {data} = await someMutation({
        variables: { ... }
      })

      if (data.errors?.length > 0) {
        // some errors
      } else {
        // success
      }
    } catch (_e) {
      // some error
    }

from apollo-feature-requests.

Another temporary solution might be:

    try {
      const {data} = await someMutation({
        variables: { ... }
      })

      if (data.errors?.length > 0) {
        // some errors
      } else {
        // success
      }
    } catch (_e) {
      // some error
    }

In my experience, the value returned by await someMutation(...) is itself undefined so it fails trying to dereference data.

from apollo-feature-requests.

The latest version of apollo 3.5.6, broke my brevious workarround, I found a new workaround, it's a bit simpler, but it's awful that the native promise is always resolved instead of rejecting it when there is an error.

export function useGraphqlMutation({ query, callbacks, options = {} }) {
    const [error, setError] = useState(null);
    const [loading, setLoading] = useState(false);
    useDebugValue(`loading: ${loading}`);

    function getResponse(res) {
        if (callbacks?.success && typeof callbacks.success === 'function') {
            callbacks.success(res);
        }
        setLoading(false);
    }

    function getError(err) {
        setError(err);
        setLoading(false);
        if (callbacks?.error && typeof callbacks.error === 'function') {
            callbacks.error(err);
        }
    }

    const [executeQuery, { data }] = useMutation(
        query,
        {
            onCompleted: getResponse,
            onError: getError,
            ...options,
        },
    );

    function run(executionData) {
        return new Promise((resolve, reject) => {
            executeQuery(executionData)
                .then(({ data: responseData, errors }) => {
                    if (errors) {
                        reject(errors);
                    } else {
                        resolve(responseData);
                    }
                });
        });
    }

    return {
        run,
        data,
        error,
        loading,
    };
}

from apollo-feature-requests.

useGraphqlMutation

Hi, I am starting with JS. Could you show me example with using of this code?

from apollo-feature-requests.

I'm coming across a similar issue, is this still being investigated?

from apollo-feature-requests.

I am a bit confuse from the conversation above. I am using Apollo client 3.7.0 and createHttpLink call.
In a case very similar to the original question, the executeQuery call (in my case) always rejects. And it doesn't matter what kind of error it is (it rejects also for resolver errors). When I set errorPolicy: 'all', the promise is resolved with errors field populated. For me this is not expected behavior, as the errrorPolicy should only specify how the errors are propagated into the result.

What I would expect:

• for network or validation errors, the executeQuery should reject.
• for resolver errors, the executeQuery should resolve with either errors populated and data: undefined (for none error policy], or errors: undefined and data partially populated (for ignore error policy), or both data partially populated and errors populated (for all error policy).

From my pov, the resulting code should be like this.

MyComponent() {
    const [executeQuery, { data, errors }] = useMutation(MUTATION);
    function onSendData() {
        executeQuery({variables: {myData: undefined}})
            .then(({errors, data}) => {
                // handle resolver errors
                // errors also populated as a result of call  `useMutation(MUTATION)` above
            })
            .catch(error => {
                // handle network and validation errors
                // in general cases when server returns 400 status code
            })
    }
    return(
        <h1>I'm not important<h1>
    )
}

The worst thing is that this is not specified in the documentation at all.
And from historic issues, the functionality changes back and forth every a while. If the executeQuery function will reject for every error (including resolver error), I am fine with that (ok, less fine with that, but I can still live with it) but please specify the behavior in documentation!

from apollo-feature-requests.

That's interesting. I would also agree documenting the desired behavior and then programming it might be the way to go.
Personally I don't care for the errorPolicy at all because it changes the underlying behavior of error handling which is bizarre to me. A consistent error handling workflow is more preferable than a configurable one and in the case of executeQuery I think the catch should be called whenever any error is "thrown" that way its consistent and doesn't require the client to update error handling in two spots.

from apollo-feature-requests.

@prageeth looks good. Only thing I think you missed was handling the executeQuery errors that might occur that aren't returned and instead are "thrown". Granted when errorPolicy is all that shouldn't happen but 🤷

from apollo-feature-requests.