useMutation.mutate() promise doesnt catch errors and always fulfills with undefined data

[oscmedgon]

Intended outcome:

I have a series of mutations that I need to execute in parallel at some point, and since useMutation.mutate() returns a promise I did a Promise.all, I got some errors due to mismatched variables, and the promise was fulfilled instead of rejected, and In the promise response, I got an array of undefined values, no matter if the request went wrong or it succeeds.

Actual outcome:

What I expected was to reject the promise when there are any errors, can be not passing some required variables, any error on the backend or the database.

How to reproduce the issue:

To reproduce the error you just need a mutation that it's going to fail, for example the model requires myData but is undefined.

MyComponent() {
    const [executeQuery, { data }] = useMutation(MUTATION);
    function onSendData() {
        executeQuery({variables: {myData: undefined}})
            // I'm being executed also when an error happen
            .then(console.log)
            // I'm never gonna be executed, no matter what happens
            .catch(console.error)
    }
    return(
        <h1>I'm not important<h1>
    )
}

Versions

System: OS: Linux 5.4 Ubuntu 20.04 LTS (Focal Fossa) Binaries: Node: 12.18.1 - /usr/local/lib/nodejs/bin/node npm: 6.14.5 - /usr/local/lib/nodejs/bin/npm Browsers: Chrome: 84.0.4147.125 Firefox: 79.0 npmPackages: @apollo/client: ^3.1.3 => 3.1.3

[tyagow]

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>
    )
}

[oscmedgon]

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.

[oscmedgon]

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.

[tyagow]

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.

[ashubham]

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

[Nate-Wilkins]

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.

[daniel-washburn-q-free]

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

[pablomarti]

Another temporary solution might be:

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

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

[ssotangkur]

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.

[oscmedgon]

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,
    };
}

[bruteforce1414]

useGraphqlMutation

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

[falconmick]

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?

[hughbric]

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

[prageeth]

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];
}

With Typings:

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>
    );
}

[PatrikValkovic]

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!

[Nate-Wilkins]

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.

[Nate-Wilkins]

@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 :shrug: