BREAKING CHANGE: useMutation returns tuple with result

[danielkcz]

Closes #90

The whole thing is sort of ripoff from Apollo's Mutation component converted to the Hooks. I've omitted a couple of things from there as those can be added later without breaking anything.

• ignoreResults prop
• onComplete & onError callbacks

---

I was thinking about this a lot. A major concern is how it fits into a whole Suspense & Error Boundaries (#28) paradigm. I mean it would be kinda nice to utilize that and have the same approach. However, mutation use cases are different than queries.

Considering simplest one...

const MutantComponent = () => {
  const [mutateMe, { error, loading }] = useMutation(MUTANT)
  return (<div>
    <button disabled={loading} onClick={mutateMe}>I want to be a mutant</button>
    <ErrorDisplay error={error} />
  </div>
}

Since Suspense is not really ready, let's not worry about that (although it's a similar problem). But could we utilize Error Boundaries here? The error would be thrown within a onClick event handler and error boundary would do what? Throw away the component and replace it with some "there was error" and "retry" button? Doesn't feel like good UX. So I am kinda convinced that we need to be handling it on more granular level like I've drafted above.

@gaearon Would be lovely to get your opinion here if you can.

[danielkcz]

I just realized, since mutations are async, a thrown error cannot be caught by error boundaries.

One of the actual use cases where propagating error toward closest error boundary is displaying such error as a toast or even sending a report to some service. Such scenarios can be nicely generalized and component code gets a bit cleaner.

So I've decided to implement an experimental approach which would throw the error synchronously by default. If anything, it gives much better error stack instead of just "unhandled rejection" inside of Apollo. And it can be switched off by throwAsync option if a need for a more granular error handling arises. Let me know what you think.

I've tested it by hand and it works nicely. Even without error boundary, the error is thrown by React, it doesn't get lost anywhere. The only caveat is that the whole component tree is thrown away in that case, but error clearly states to use error boundary, so I wouldn't be too worried about it.

[danielkcz]

@trojanowski I would love to hear an opinion from you if I should continue with adding tests/docs or it's something you don't want to merge at all.

[trojanowski]

@FredyC the current form of useMutation was enough for me - I have usually used it in conjunction with Formik, where the loading and error state was stored. But I agree that there are many use cases where the implementation suggested by you makes perfect sense (maybe with the exception of the error handling). It would also make it easier to port code which used the <Mutation /> component from react-apollo. Few comments:

Since Suspense is not really ready, let's not worry about that (although it's a similar problem).

Actually, I can't see how Suspense would be helpful here.

But could we utilize Error Boundaries here?

So I've decided to implement an experimental approach which would throw the error synchronously by default. If anything, it gives much better error stack instead of just "unhandled rejection" inside of Apollo. And it can be switched off by throwAsync option if a need for a more granular error handling arises. Let me know what you think.

I don't think an error should be thrown at all by default. In my use cases, I needed access to the error as a part of the response, so I'd always had to disable this behavior. E.g., please look at the following code:

<Formik
  onSubmit={async (values, { setErrors, setSubmitting, setValues }) => {
    let response;
    try {
      response = await performMutation({ variables: { input: values }});
    } catch (error) {
      setErrors(getValidationErrors(error));
      NotificationManager.error('Cannot perform mutation');
      return;
    } finally {
      setSubmitting(false);
    }

    setValues(mapResponseToFormValues(response.data.myMutation));
    NotificationManager.success('Mutation completed');
  }
/>

Actually, I don't see a scenario where throwing them would be helpful. For cases where I think the rest of this PR would be useful (e. g. an easy possibility of re-running a failed mutation) it'd be better to not lost the state of the component which error boundaries would cause.

[danielkcz]

Actually, I can't see how Suspense would be helpful here.

Yea, without a full picture how Suspense for data fetching will look like, I cannot imagine it either :) It pretty much boils down to eg. disable submit button which is very similar operation to showing spinner on data fetching.

Actually, I don't see a scenario where throwing them would be helpful.

One of the actual use cases where propagating error toward closest error boundary is displaying such error as a toast or even sending a report to some service. Such scenarios can be nicely generalized and component code gets a bit cleaner.

Error boundary does not always mean you have to render some placeholder UI, it can be really just to collect errors and do something different with those. The challenge could be that the error boundary does not give much of context if it was thrown from mutation or a failed query which might require a different approach.

Anyway, if you look closer at Mutation component, it already does throw by default, but since it's async code, it ends up as unhandledRejection which is not really helpful. I only turned it around so it throws synchronously, but can be turned off.

I guess Formik is not ready for this hooked world with such imperative API. If something like following would work, it would certainly clean up a lot of things. It's fairly contrived, but you get the gist :)

const [performMutation, { data, loading, error }] = useMutation(...)
<Formik
  isSubmitting={loading}
  errors={getValidationErrors(error)}
  values={mapResponseToFormValues(data)}
  onSubmit={async (values) => {
    await performMutation({ variables: { input: values }});
    NotificationManager.success('Mutation completed');
  }}
/>

[lotsoftick]

@FredyC thanks for your work. When are you going to release this ? :)

[danielkcz]

@lotsoftick I am kinda waiting for more opinions. There are still no tests and docs and I don't want to put an extra effort into it before it's clear that it's wanted change.

[lotsoftick]

@FredyC i think this is crucial, and must be done.

const [mutateMe, { error, loading, data }] = useMutation(MUTANT)

[danielkcz]

Ok, I agree with that, but what about error handling part? Should it throw by default? The previous behavior means you have to explicitly do this for every mutation call.

const MutantComponent = () => {
  // using the new option here which represents previous behavior
  const [mutateMe, { error }] = useMutation(MUTANT, { throwAsync: true })
  const mutateMeSafely = (...args) => {
    mutateMe(...args).catch(handleError)
    // or use async with try/catch which would way more verbose
  }
  return (<div>
    <button disabled={loading} onClick={mutateMeSafely}>I want to be a mutant</button>
    <ErrorDisplay error={error} />
  </div>
}

Compared to new default behavior from this PR the error is thrown synchronously which means it can be caught by error boundaries and sent to reporting service in a universal way without doing it every time the mutation is called. But it's still possible to grab the error from result and render it if that's what's required.

const MutantComponent = () => {
  const [mutateMe, { error, loading }] = useMutation(MUTANT)
  return (<div>
    <button disabled={loading} onClick={mutateMe}>I want to be a mutant</button>
    <ErrorDisplay error={error} />
  </div>
}

Personally, I don't really mind which behavior goes by default as it's easy to wrap it into a custom hook. But I am trying to provide a best default experience.

[danielkcz]

The error boundaries still need a class component, but it's easy to have a single one like this sitting on top of the tree.

class ErrorBoundary extends React.Component {
  componentDidCatch(error, info) {
    // Report error, show the toast, whatever you like...
    logErrorToMyService(error, info);
  }

  render() {
    return this.props.children; 
  }
}

By directly rendering the children, no UI gets lots or remounted. It really just to catch that error in universal way. And best part is that you can make different error boundary, use it lower in the rendering tree to customize experience for a specific part of the app.

[lotsoftick]

i prefer the first option, without Error Boundary

[danielkcz]

@lotsoftick Do you care to elaborate why? I am wondering if you are seeing some benefit there that I am missing.

[lotsoftick]

mutation request may fail because of validation, let's say with error status 422 and with error messages for each field in the body. I understand that this can be done using ErrorBoundary as well, but basically my components look like this and i don't use ErrorBoundary, i just need mutateMe to return promise, and some loading indicator. I don't know would this be helpful for you or not.

const RouteComponent = ({ params, history }) => {
  const { loading, error, data } = useQuery(GET_LIST);

  return (
    <Fragment>
      {params.id &&
        <EditModal id={params.id} closeModal={() => history.goBack()} />
        /* show modal form over the table   */
      }
      {error && <div>{error.message}</div>}
      <table className={classnames({ loading })}>
        ...
        {data.map( /* render data */)}
        ...
      </table>
    </Fragment>
  )
}

const EditModal = ({ id, closeModal }) => {
  const { loading, error, data } = useQuery(GET_ITEM, { variables: { id }}); // getting item data
  const [mutateMe, { loading: updating, error: updateError }] = useMutation(MUTANT);

  const submit = async (e) => {
    await mutateMe(formData);
    closeModal();
  }

  return (
    <Modal>
      {error && <div>{error.message}</div>} {/* show error if can't fetch item data */}
      {updateError && updateError.status !== 422 && <div>{updateError.message}</div>} {/* show error if something went wrong during update. But error is not validation */}
      <ValidForm
        className={classnames({ loading: loading || updating })}
        onSubmit={submit}
        showValidationError={updateError.status === 422 ? updateError : null} {/* in case if data vas invalid show validation errors for each filed */}
      >
        {/* ... render inputs */}
      </ValidForm>
    </Modal>
  )
}

[danielkcz]

I see that I still haven't explained it well enough. You can do exactly that with this PR and default sync throw.

You will always get the error as the result from useMutation, that's for the fact and you cannot prevent that behavior.

Think of that sync throw as a bonus. A unified way of catching all mutation errors at one place (error boundary) and ideally reporting them to some service so nothing is going to escape your attention. But you can also ignore them if you feel brave enough. Without it, you will only get unhandledRejection which you would need to catch manually for every single mutation call.

In your EditModal example, without that sync throw, you would need to do this.

  const submit = async (e) => {
    try {
      await mutateMe(formData);
      closeModal();
    } catch (err) {
      // ignore error, you are already getting it from useMutation
    }
  }

Is it better like this or am I really bad at explaining it? :)

@trojanowski Also would like to hear your thoughts on my comment above responding to you.

[lotsoftick]

@FredyC thanks, it's clear. Both will work for me, so, just use second one (
sync throw) as default :)

[guilhermedecampo]

This is amazing, when are you able to merge @FredyC ? 🚀

[danielkcz]

@guilhermedecampo Would love to get it out soon as well, but I am kinda fighting with tests.

Also @trojanowski doesn't seem to be convinced about it, so I hope he won't reject it later :)

[trojanowski]

@FredyC

Error boundary does not always mean you have to render some placeholder UI, it can be really just to collect errors and do something different with those.

About collecting errors and sending them e.g. to Sentry - I don't think I'd like to send there any of errors being a result of invoking a mutation. They can belong to one of these groups

• network connection errors, e.g. if the user is online
• server-side resolver errors - they will be sent by a server Sentry integration
• predefined Apollo errors - they are like 4xx HTTP errors

The challenge could be that the error boundary does not give much of context if it was thrown from mutation or a failed query which might require a different approach.

I can see that you already tried to solve it in cf7b8b3. One suggestion - you can use an internal Symbol here as a marker.

Anyway, if you look closer at Mutation component, it already does throw by default, but since it's async code, it ends up as unhandledRejection which is not really helpful.

It just simulates the behavior of apolloClient.mutate(). It's just like:

try {
  const result = await client.mutate(...);
  doSomethingWithResult(result);
} catch(error) {
  throw error;
}

unhandledRejection can also be sent to an error collecting service, but as I said above, I don't think it's a good idea. In my opinion async errors works best if you try/catch them manually, like in the Formik example in my previous comment.

My idea is to replace throwAsync option with something like throwMode. It could have 3 possible values:

• async - the behavior in the current version of react-apollo-hooks. It works well in almost all cases for me. I think it should be the default value.
• none - don't throw errors at all, just set the value of the error state. That way we'd have something like an "internal" error boundary. We could check the presence of the error and, e.g. show "Try again" button.
• sync - the default behavior in the current version of this PR. I actually don't see too much use cases for it (which wouldn't be solved by the none throw mode).

@trojanowski doesn't seem to be convinced about it, so I hope he won't reject it later :)

I like the API. I'm just not sure about the error handling part.

@umidbekkarimov @rosskevin what's your opinion about it?

---

EDIT: I just noticed I mistaken sync, and async modes in the previous version of my comment. Maybe we need better names for them. :)

[danielkcz]

About collecting errors and sending them e.g. to Sentry - I don't think I'd like to send there any of errors being a result of invoking a mutation. They can belong to one of these groups

• server-side resolver errors - they will be sent by a server Sentry integration

Yea, I do care about those errors and I want them in client-side Sentry simply because there might some relevant app state that can help figure out an issue. Perhaps I've sent wrong variables and I would like to know why and how it happened.

I can see that you already tried to solve it in cf7b8b3. One suggestion - you can use an internal Symbol here as a marker.

I am not sure if there some real benefit to it. I got inspired by ApolloError and how is the isApolloError implemented :)

The throwMode.none feels pretty much useless because you cannot render any UI from the hook. Someone might misunderstand, use it and the errors would be suddenly lost unless they manually check the error in every mutation call. It's fairly dangerous mode to have there. I would rather not.

---

I have a slightly different idea. We can have sync mode by default, but we can export something like this from the package.

const throwModeContext = React.createContext()

class ErrorBoundary extends React.Component {
  componentDidCatch(error, info) {
    console.error(error)
  }

  render() {
    return <throwModeContext.Provider value="async">{this.props.children}</throwModeContext.Provider>; 
  }
}

This way it's possible switch the mode not globally, but for that part of tree where we actually know there is an error boundary to catch them. It's much safer. The context Provider itself could be exported as well for a customized error boundaries.

This gives a great power as you can keep part of the tree to handles sync errors and other part would be throwing async and catching with boundary. Most importantly, you don't need to think about it. Just drop boudary where you want and rest of the app will be safe.

[avocadowastaken]

@umidbekkarimov @rosskevin what's your opinion about it?

@trojanowski I'm good with this proposal as long as we keep current hook functionality (e.g: as useMutationFn).

TBH I'm not using useMutation directly in my project, I merged it with react-final-form-hooks as useMutationForm so final-form deals with all state management for me.

---

But I'm not sure about using context to manage hook options, It makes it counter intuitive. It's still better to wrap with custom hook to keep it straightforward.

[danielkcz]

But I'm not sure about using context to manage hook options, It makes it counter intuitive. It's still better to wrap with custom hook to keep it straightforward.

It's not meant like that. You would still be able to override that throwMode per useMutation call meaning you can surely wrap it into a custom hook. The idea is merely to have an easy escape route with a primitive error boundary to never lose any error and have useful information about it. Later on you will be able to easily customize it. No need to use the Context, you can provide your own means for a configuration.

The sync mode which yields unhandledRejection gives absolutely no clue of where the error happened and that's the major reason why I don't like it.

[sami616]

this looks 😍merge merge! 😁

[robertvansteen]

I’m just wondering what the appropriate way to handle these errors in the error boundaries would be. Right now I use error boundaries to crash my application by throwing errors I can’t recover from. With this change I would need to know that the error is from a mutation (for example a form) and we can safely render the children and display the error with some sort of an alert in the form.

[danielkcz]

@rovansteen Yea, I haven't mentioned it here just yet, but the last commit contains some utility for that. I am planning to add the same thing for queries, later on, to be able to define opposite logic if necessary.

import { isMutationError } from 'react-apollo-hooks'
class ErrorBoundary extends React.Component {
  state = { bailout: false }
  componentDidCatch(error, info) {
    console.error(error)
  }

  static getDerivedStateFromError(error) {
    return { bailout: !isMutationError(error) };
  }

  render() {
    if (this.state.bailout) {
      return <div>It has crashed</div>
    }
    return this.props.children
  }
}

It's also possible to use isApolloError directly from apollo-client for some common logic.

[trojanowski]

I'm good with this proposal as long as we keep current hook functionality (e.g: as useMutationFn).

@umidbekkarimov actually you could quite easily get the current functionality. It would be enough to replace the following invocation:

const mutate = useMutation(MY_MUTATION);

with that one:

const [mutate] = useMutation(MY_MUTATION);

Would it work for you? I'd prefer not to add a separate hook just for that. @leonardfactory @DAB0mB @Urigo Is that change OK for you? Would it be problematic for graphql-code-generator?

TBH I'm not using useMutation directly in my project, I merged it with react-final-form-hooks as useMutationForm so final-form deals with all state management for me.

Good to know it's not only me using mutations with a form library. :)

The throwMode.none feels pretty much useless because you cannot render any UI from the hook. Someone might misunderstand, use it and the errors would be suddenly lost unless they manually check the error in every mutation call. It's fairly dangerous mode to have there. I would rather not.

I thought it's the biggest benefit of this PR. :) You cannot render any UI from the hook, but you can use the presence of an error to decide what to render in your component. So if you wanted such behavior and didn't want to send those errors to a collecting service you would have to use the hook that way:

const [mutateBase, { error }] = useMutation(MY_MUTATION);

async function mutate(options) {
  try {
    await mutateBase(options);
  } catch (errorToIgnore) {
    // Ignore the thrown error to prevent `unhandledRejection` event.
    // We use `error` returned by `useMutation` to decide what to render.
  }
}

Anyway, I have an alternative suggestion. Let's forget about throwMode and introduce a different option - errorHandler. It would be a function run when an error happened. The default implementation would be:

const defaultErrorHandler = error => { throw error; }

So it would work the same as the current version of useMutation. But you can replace it e.g. with noop (a more explicit version of the none throw mode), or a handler sending an error to Sentry:

function useSetryFriendlyMutation(mutation, options) {
  return useMutation(mutation, {
    ...options,
    errorHandler: error => {
      Sentry.captureException(error);
    }
  );
}

That way you don't need to use isApolloError or isMutationError. But if you still prefer to use error boundaries you can easily create a custom hook for that:

function useMutationWithErrorBoundariesAnyway(mutation, options) {
  const [mutate, result] = useMutation(mutation, options);
  if (result.error) {
    throw result.error;
  }
  return [mutate, result];
}

But I'm not sure about using context to manage hook options, It makes it counter intuitive. It's still better to wrap with custom hook to keep it straightforward.

I agree. But if you still prefer such behavior you can do it easily with a custom hook and a context provider.

[danielkcz]

But if you still prefer to use error boundaries you can easily create a custom hook for that:

Error boundaries are becoming fairly common practice. I don't like that you would not have an option to use them directly and everyone would have to enable it with a custom hook. I would be really careful about building library in a way that needs custom hooks to work properly.

Let's forget about throwMode and introduce a different option - errorHandler. It would be a function run when an error happened. The default implementation would be:

I don't follow how is that good thing. In most cases handling an error does mean you want to log/report it somewhere. It would be super annoying to need to do it everywhere. I like my DRY code :) Besides, you cannot throw sync error from that error handler, so error boundary won't work imo.

I am starting to like throwMode. And I don't mind having the none option after all (it already helped with tests). It can come in handy in scenarios where you really do want to just render error in some way (eg. icon), but you don't want to report it anywhere. It just needs proper warning in the documentation. I am thinking about a separate document about error handling in general.

And I made a wrong statement above. I've meant to keep async mode as default. I am perfectly fine with that. Then the error boundary can automatically switch it to sync for a given subtree so those errors can actually bubble up.

[leonardfactory]

@trojanowski

Would it work for you? I'd prefer not to add a separate hook just for that. @leonardfactory @DAB0mB @Urigo Is that change OK for you? Would it be problematic for graphql-code-generator?

Seems like it would be supported by default. Since we just return ReactApolloHooks.useMutate(...) result entirely, it should work by default! I'll make sure however anything plays well if and when this will be released

Good to know it's not only me using mutations with a form library. :)

+1 here 😄 Furthermore, Formik is currently working on v2 version based on hooks

[danielkcz]

Alright, it's time ❗️ I am opening PR for a review as I have finally managed to figure out tests. I will work on the documentation in the meantime.

[joenoon]

Getting up to speed on this and I've picked up a couple nice patterns from the comments that beat the fully imperative routine of try/catch/handle I usually do, so thank you. I think this reduces the code needed around calling a mutation - considerably if you only need to display the loading/data/error declaratively. In my case I usually need to do something with the response imperatively, but even then this still reduces loading and error and the need to catch. I think I like it!

[gijo-varghese]

Looks like official react-apollo is getting ready for hooks! apollographql/react-apollo#2892

[danielkcz]

@gijo-varghese That won't be ready for some time yet, there was a word they want to wait for Suspense for data fetching which might be by the end of the year.

[gijo-varghese]

@FredyC can we merge this soon?

[danielkcz]

@gijo-varghese Well, it's not up to me really, @trojanowski had some objections, I've responded. If you are impatient, just grab the Yalc and you can roll without waiting. That's what I do.

[trojanowski]

@FredyC

I think SillyErrorBoundary in the current form is not very useful

That depends, in React Hooks world it's very useful as you don't have a hook variant (yet). It's a useful workaround so you don't need to copy-paste it in your code. I agree it doesn't really fit well in the package, but I have to keep in the code for the tests. So why not export when it's already there? :)

Because it would be a part of the public API and we'd have to support it. Also, I'd instead prefer to recommend a specialized library for that, like react-error-boundary. Moreover, I don't think SillyErrorBoundary is even useful for tests in this PR - you use it to check if no errors were throws while rendering the React tree. However, you don't need an error boundary for that - tests throws by default in that case.

Also, ApolloError seems enough for me (instead of ApolloOperationError) - in your onError handler you know that the error was thrown by a mutation (and you can determine by which one).

How can you know which mutation has thrown in a universal error boundary? Or if it was a mutation at all? That's kinda point of that ApolloOperationError so you can unify your error handling into a single point and do a decision what to do based on the operation failing. When this is merged, I would like to introduce the same thing into useQuery and useSubscription.

But we decided that we won't support the synchronous error throwing, so in order to use error boundaries (or sending it to the context like in your comment) you'd need a custom hook - you could convert it to a "higher order error" there. We could continue the discussion about ApolloOperationError, but I'd prefer to do that in a separate issue or PR - if we'd merge this PR in the current form, some public API functions would behave incorrectly (isQueryError, isSubscriptionError).

[danielkcz]

But we decided that we won't support the synchronous error throwing, so in order to use error boundaries (or sending it to the context like in your comment) you'd need a custom hook

It has nothing to do with the synchronous throw. It's about an error object in general which should carry that information to improve debugging. I have following in my wrapper which allows me to pass an error to closest Context (ApolloDefender) which can decide what to do based on information in that error. If it's just regular ApolloError, that information is lost. I do the same for useQuery and useSubscription.

  const { onError } = useApolloDefender()
  React.useEffect(
    () => {
      if (result.called && result.error) {
        onError(result.error)
      }
    },
    [result.called, result.error],
  )

I think I am going to hand this over to you and do changes you deem necessary. We obviously have different opinions about it and nobody else seem to care enough. I am already using my custom fork of this library so I don't really care that much to get this merged.

[charlie632]

When is this getting merged? I need error checking for a crucial part in my application flow. Is any workaround for this?

[guilhermedecampo]

@charlie632 http://bfy.tw/N54W

[sami616]

Hi folks, just checking if this is close to being merged? really looking forward to it!

[danielkcz]

Perhaps, instead of asking for a merge to be done soon you could actually contribute with an opinion on standing questions. It would ultimately help much more than poking and prodding like impatient nerds. 🙊

[trojanowski]

Next week I'm going to try to add changes I requested in #93 (comment) and #93 (comment) and merge after that.

[sami616]

@trojanowski thanks for the insightful response.

[groundmuffin]

@FredyC, I appreciate your efforts

[jeffreyffs]

Edit: I am currently working on an article showing that error boundary is fairly useless in its current form and Context has surpassed it.

@FredyC I'm super curious on this topic and was wondering how this article is coming along? Would love to see your implementation of a context based error handler :)

[trojanowski]

I added an alternative PR with changes I requested in my previous comments: #135. @FredyC please let me know what do you think about it.

[danielkcz]

@trojanowski Well, you know my opinion. I could live with the removal of SIllyErrorBoundary, but those errors wrapped are essential. I am already using those in production and it has proven very valuable in automated bug reports and figuring out what to show to a user based on that information in there. Shame nobody else sees it. So be it, I will stick to my fork.

[ctrlplusb]

I understand you are excited about this feature @gijo-varghese - but responding like this sounds quite insensitive to the fact that a lot of people donated their time and effort to make this library freely available to you. Fair enough if there is an alternative but please show a moderate amount of appreciation for what has been gifted.

[maxschmeling]

This library has been incredibly helpful and well done. To give @gijo-varghese the benefit of the doubt, I hope they just thought that it was a transition to Apollo ownership / maintenance and they were genuinely excited for the library.