Rejecting responses with HTTP failure codes has unfortunate consequences

[torkelrogstad]

Thanks for making restful-react! It's a great library and tool (I'm using it to generate code from an OpenAPI spec).

I really like how useGet works:

1. The promise never rejects, so you can await on it safely without it throwing an error
2. It's easy to check if data or error is defined, and then act on it. (If I was to change anything, there are some cool things you can do with conditional types, so that the compiler can automatically infer that data or error is defined, but never both)

However, useMutate has some drawbacks:

1. The promise can throw, meaning that await is not safe without appending an empty .then clause:

   await useMutate("path", props).catch(() => { /* pass */})
   

   Furthermore, even though the error field from useMutate is of type GetDataError<TError>, the compiler cannot infer that in the promise rejection clause, because failed promises can contain anything.
2. There's no data field, to accompany error. That means that I'll have to use two mechanisms: await on mutate to read the result of my operation, and check error for any failures.

I'd think it would make a lot of sense for useMutate to be more similar to useGet:

1. The promise returned by mutate should never reject, and also resolve to void
2. A new field data: TData | null should be added, to mirror the existing error.

Do you have any thoughts on this? I'd be happy to contribute some code to implement this functionality.

[torkelrogstad]

If I submitted a PR for this, would it be considered for merging?

[fabien0102]

Hi @torkelrogstad,

Thanks for the feedback! 😃

For info, we have 24 occurences of .catch in our codebase, so even if we break the API this should not take so long to migrate. So yes, we would considered to merge if the partern is better (and I also think we can improve this)

Just some considerations to have in mind, we have a lot of this kind of logic:

const MyComp = () => {
  const { mutate: createResource } = useMutate();

  return (
    <Button
      onClick={() =>
        createResource()
          .then(() => dispatch("success"))
          .catch(() => dispatch("error"))
      }
    />
  );
};

Basically, redirect on success / display a nice message / refetch a list. I wondering how this will look like with a promesse that doesn't throw.

For the error & data part in useGet, both can be defined if I remember well in case of a refetch, so you can display an error and keep your stale data 😉

[torkelrogstad]

What do you think of this?

const MyComp = () => {
  const { mutate: createResource, data, error } = useMutate();

  return (
    <Button
      onClick={async () => {
        await createResource()
        if (error) {
          return dispatch("error", error) // error has type GetDataError<TError>
        }
        dispatch("success", data))
      }}
    />
  );
};

It very closely mimics useGet, which I think is good for two reasons:

1. useGet has a very pleasant API
2. Consistency is always good:-)

[fabien0102]

@torkelrogstad Looks nice, always worried about how React behave with this error state in middle of a callback, but if this is working this looks good.

@TejasQ any thoughts about this?

[amacleay-cohere]

I think typically one would want to handle the dispatch in a useEffect, so rewriting the above you might have:

const MyComp = () => {
  const { mutate: createResource, data, error } = useMutate();
  useEffect(() => {
    if (error) {
      dispatch("error", error)
    }
  }, [error]);
  useEffect(() => {
    if (data) {
      dispatch("data", data)
    }
  }, [data]);
  return (
    <Button
      onClick={() => createResource()}}
    />
  );
};

which I think mirrors the typical pattern for useGet-handling as well.

[torkelrogstad]

@amacleay-cohere you're absolutely right, this seems like the idiomatic hooks way of handling this. I'm a bit swamped with work right now, but hopefully I can sit down next week with an attempt at this.

[TejasQ]

Thank you so much, @torkelrogstad!