[Bug?]: useMutation not rejected on error about redwood HOT 12 CLOSED

Oh, thanks for the extra explanation @xmaxcooking ---now I understand the core problem:

As things stand right now if you want to create a toast with a feedback that the "request is running" AND eventually if the "request failed or succeeded" then you'd have to create multiple toast instances.

and make it easier/possible to use

Let me dig into this a bit and potentially ask the Apollo team if they have thoughts/experience using toast in this way.

from redwood.

@xmaxcooking Sounds like you've got a handler on this now? Would you recommend a documentation to https://redwoodjs.com/docs/toast-notifications ?

If so, would you be able to PR that with and example an explanation?

Or, do you think we need any framework changes to make it work better?

Thanks both to you and @jerelmiller for helping me thought understanding this.

from redwood.

Hi @xmaxcooking and thanks much for the detailed summary and links - super helpful

I was wondering if you think this is simply an out-of-date documentation example?

I ask because if you look at how toast is used here:

https://github.com/redwoodjs/redwood-tutorial/blob/main/web/src/components/Post/NewPost/NewPost.js

Might this be how you'd use it?

If so, then I think we should revisit these docs.

from redwood.

Hey @dthyresson! Thanks for taking a look at this issue.

No, in this case it isn't an outdated documentation. It's true that the useMutation provides events to create a toast.success or a toast.error and these events work perfectly. No issue there.

but giving the user feedback with these is only working if you skip the loading state feedback.

As things stand right now if you want to create a toast with a feedback that the "request is running" AND eventually if the "request failed or succeeded" then you'd have to create multiple toast instances.

The react-hot-toast promise is designed to merge these 2 toasts into a single toast instance but .. and that's the issue .. they designed the toast.promise function probably with axios.post() in mind and that function rejects the promise of the invoked mutation if the api throws an error.

the useMutation from redwood or apollo (I'm unsure about this) never rejects the promise because the errors are available either way.

I was able to workaround this by creating a wrapper around the useMutation hook and let it behave like I'd expect it to

like this:

import {
  ApolloCache,
  DefaultContext,
  DocumentNode,
  MutationFunctionOptions,
} from '@apollo/client'

import { useMutation as useRwMutation } from '@redwoodjs/web'

const useMutation = <TData = unknown, TVariables = GraphQLOperationVariables>(
  mutation: DocumentNode,
  options?: GraphQLMutationHookOptions<TData, TVariables>
): MutationOperationResult<TData, TVariables> => {
  const [invoke, result] = useRwMutation(mutation, options)

  const customInvoke = (
    options: MutationFunctionOptions<
      TData,
      TVariables,
      DefaultContext,
      ApolloCache<unknown>
    >
  ) => {
    return new Promise<TData | undefined>((resolve, reject) => {
      invoke(options)
        .then((res) => {
          if (res.errors) {
            reject(res.errors)
          } else {
            resolve(res.data)
          }
        })
        .catch((err) => {
          reject(err)
        })
    })
  }

  return [customInvoke, result]
}

export default useMutation

but it felt like the original function should have rejected this anyway.. so I created an issue here.

from redwood.

Exactly. Glad that I was able to explain it more clearly 🙂 the 2nd link was missleading.

from redwood.

Just a FYI @xmaxcooking I reached out to Apollo for a little guidance and will report back when I learn more.

from redwood.

Hey @xmaxcooking 👋

I'm Jerel from the Apollo Client team! Any chance you're using an errorPolicy other than none in your mutation? If its set to all or ignore, then errors aren't thrown and are instead returned in the mutation result itself via the error property, that way you're able to handle partial data from the response.

The only other thing that would cause that promise not to resolve is if you passed an onError callback to your useMutation hook. I don't see that in your sample code, so I'm willing to bet this isn't the reason.

We do have some tests to verify that the promise is rejected when using an errorPolicy of none. Check out this one: https://github.com/apollographql/apollo-client/blob/f92db4e809e113000b30fc081b7cf1648efe3446/src/react/hooks/__tests__/useMutation.test.tsx#L337-L375

You can also see that setting a different error policy will resolve the promise and set the error property instead: https://github.com/apollographql/apollo-client/blob/f92db4e809e113000b30fc081b7cf1648efe3446/src/react/hooks/__tests__/useMutation.test.tsx#L412-L448

Any more information would be super helpful!

from redwood.

hello @jerelmiller thank you so much! .. No, I don't have an errorpolicy configured.

In my case I was kind of expecting that the onError callback should have no influence to the way the promise resolves.

I'm using it like this:

  const [createAddress, { loading, error }] = useMutation(
    CREATE_ADDRESS_MUTATION,
    {
      onCompleted: () => {
        navigate(routes.addresses())
      },
      onError: (error) => {
        logger.error('Error creating address:', error)
      },
    }
  )

  const onSave = async (input: AddressInput) => {
    toast.promise(createAddress({ variables: { input } }), {
      loading: t('addresses.page.new.create.loading'),
      success: t('addresses.page.new.create.success'),
      error: t('addresses.page.new.create.error'),
    })
  }

and you're absolutely right. if I remove the onError callback then the promise gets rejected as expected.

  const [createAddress, { loading, error }] = useMutation(
    CREATE_ADDRESS_MUTATION,
    {
      onCompleted: () => {
        navigate(routes.addresses())
      }, 
    }
  )

  const onSave = async (input: AddressInput) => {
    toast.promise(createAddress({ variables: { input } }), {
      loading: t('addresses.page.new.create.loading'),
      success: t('addresses.page.new.create.success'),
      error: t('addresses.page.new.create.error'),
    })
  }

to have both (the logged error + the toast.promise) I'd then have to use it like:

const [createAddress, { loading, error }] = useMutation(
  CREATE_ADDRESS_MUTATION
)

const onSave = async (input: AddressInput) => {
  toast
    .promise(createAddress({ variables: { input } }), {
      loading: t('addresses.page.new.create.loading'),
      success: t('addresses.page.new.create.success'),
      error: t('addresses.page.new.create.error'),
    })
    .then(() => {
      navigate(routes.addresses())
    })
    .catch((error) => {
      logger.error('Error creating address:', error)
    })
}

I'm guessing there is no errorpolicy to have them both, so the 1st version (with the cleanest code) would work?

from redwood.

@jerelmiller might we be setting a policy here

from redwood.

ah. you're right. assuming that's how it's supposed to be.

from redwood.

@xmaxcooking Ahhh ok that dang onError callback haha. To be honest, I'm not sure why the useMutation API was originally designed that way. I would love to see Apollo Client core change this functionality in a future major version since it never made much sense to me to resolve instead of reject the promise when onError is used. Its bitten quite a lot of our users and we have had a decent number of issues over it. You're right in that we don't have an errorPolicy that covers this specific onError behavior. You can see in the useMutation code that we don't throw when onError is given, so there is no way to bypass this currently. Up to you which method works for your use case the most, just as long as you aren't relying on the rejection behavior and passing onError at the same time 🙂

@dthyresson this doesn't look connected to the client to me. If I'm reading that error boundary right, onError doesn't get passed to useMutation in any way, so I don't think this is affecting the behavior of useMutation at all. Am I understanding your question correctly?

from redwood.

@dthyresson Yes, everything is clear now.

You can expect a PR from me to the docs but I'm going to take a look at the docs in general first. The core issue was that the behaviour of useMutation was unexpected, if you provide an onError callback. I wonder if this missing information could fit anywhere else (near a useMutation explanation or related).

As for the framework changes.. you could of course implement a wrapper like I did around it but then you'd potentially have issues with people wondering why the useMutation exported from redwood behaves differently than the apollo documentation says.

It's a tradeoff redwood could take if we expect that apollo client is never going to change this, but even then I wouldn't really recommend it.

from redwood.