[react-graphql] useMutation return type should match Apollo's useMutation

alanthai commented 4 years ago

Overview

Currently we can check the loading state of a query with

const { loading } = useMyQuery(...). We don't have an equivalent for useMutation

Motivation

What inspired this enhancement? What makes you think this should be in quilt?

Just like queries, mutations might require blocking a part of the app (eg, loading states). It would make sense to have the same type of loading key to hook into

Checklist

[x] Please delete the labels section before submitting your issue
[x] I have described this issue in a way that is actionable (if possible)

michenly commented 4 years ago

There are a few things that might contribute to the confusion, and I will try to explain all those below.

@shopify/react-graphql was created before the official apollo-hoook exist, so the API are not exactly the same with what Apollo has currently. In the future, I would suggest looking at your type to see exactly what is available over checking apollo docs.

Of note, the web-foundations team are aware of the confusing this had been causing and is looking to resolve it. We will either be updating @shopify/react-graphql to be more in-sync with apollo or possible departing from apollo all together because the team had been frustrated with a lot of the apollo features.

useQuery is a hook that will trigger query right away, while useMutaiton is a hook that return a Promise for you to tigger:

This is because mutation are almost always call upon a user action and never on page load/component mount. And because of this defer call functionality, returning a loading variable when you call the mutation doesn't make much sense, since it will never get the loading value till the promise resolved.

const myMutation = useMutation(graphqlMutation);
...
function submit() {
   const {data, /*loading here wont response till promise resolve*/} = await myMutation();
}

However, you can make your own loading status using react state easily.

const [myMutationLoading, setMyMutationLoading] = React.state(false);
const myMutation = useMutation(graphqlMutation);
...
function submit() {
   setMyMutationLoading(true);
   const {data, /*loading here wont response till promise resolve*/} = await myMutation();
   setMyMutationLoading(false);
}

Tom-Bonnike commented 4 years ago

(I ended up here after reading the Web Foundation newsletter, hi! 👋 )

You’re right in saying that you can make your own loading status using state but the code above is not what the Apollo API looks like, the loading state isn’t returned by the mutate function, it can be accessed directly as the second argument of the tuple returned by useMutation:

const [mutate, { loading }] = useMutation(graphqlMutation);

function submit() {
   const response = await mutate();
   // Here you usually use the response data but not the loading state since
   // if this code gets executed, you know the mutation has finished loading.
   console.log(response.data)
}

This is great because it saves you from having to maintain your own state for the loading status. Inside the submit function, you don’t need to know it’s loading, becacuse the code after the await statement won’t be executed until it has finished loading anyway.

Just in case someone picks this up and gets confused 😅

Seems like our own useMutation hook doesn’t really do anything that Apollo wouldn’t do? Couldn’t we just directly return Apollo’s useMutation? Obviously that would be a breaking change since it would start returning a tuple [mutate, mutationResult] but I think there‘s value in syncing with the Apollo API and writing a codemod for it shouldn’t be too complicated 🤷‍♂️ const variable = useMutation() ➡️ const [variable] = useMutation()

If this makes sense, I’d happily give it a shot!

We could do the same with useLazyQuery https://github.com/Shopify/quilt/issues/1382, which returns a similar tuple. I guess we would need to re-use our own useQuery logic though, there’s a lot in there!

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

BPScott commented 1 year ago

/me puts on quilt maintainer hat

We should try to mimic Apollo's API to ease understanding and close the gap between apollo and @shopify/react-graphql (getting people to just use Apollo directly is a good goal). That goes for updating useMutation's return type to be an array where the second item contains loading et al, and adding a version of useLazyQuery.

We should change useMutation's return type to be an array tuple, matching Apollo's useMutation. The first item should be the mutation function (i.e. what currently gets returned from useMutation), and the second item should be an object containing data, loading, error, called and client keys.

Making the breaking change in @shopify/react-graphql is relativly simple. The hard part is ensuring a simple upgrade path to all the consumers that currently rely on the current API shape. We would need to provide a codemod that can be ran against existing apps that updates all usages of const blah = useMutation() to const [blah] = useMutation() where useMutation comes from @shopify/react-graphql

Shopify / quilt