relay-tools / relay-hooks

Use Relay as React hooks
https://relay-tools.github.io/relay-hooks/docs/relay-hooks.html
MIT License
540 stars 56 forks source link

Return the optimisticResponse in the data field in useMutation when the mutation is in flight #145

Closed shreyas44 closed 3 years ago

shreyas44 commented 3 years ago

Is this the expected behaviour considering that optimistic updates should be instant and shouldn't have the intermediate loading state?

I'm not through with relay-runtime and the internal of relay, but would it be safe to say that we could set the data here to the optimisticResponse if the optimisticResponse is set by the user? This would remove the need for the intermediary state where data is null as mentioned above and hence a better UX. https://github.com/relay-tools/relay-hooks/blob/a1d08441c9f8119204622f9f843cb45361c9eec8/src/useMutation.ts#L73-L77

morrys commented 3 years ago

Hi @shreyas44, it seems to me a good idea to provide the optimistic response in the data field when the mutation is in flight, although the optimistic responses can be configured not only by optimisticResponse but also by optimisticUpdater / configs.

@taion, what do you think about it?

taion commented 3 years ago

I feel like it would be best to match whatever the upstream semantics are, just to keep things easy to reason about for people migrating back and forth. It might be reasonable to have a separate optimisticData for cases where that's appropriate, in any event.

shreyas44 commented 3 years ago

@taion do you mean adding another field called optimisticData to the state?

taion commented 3 years ago

Yeah! That way it wouldn't be a chance to the semantics of data.

shreyas44 commented 3 years ago

Yeah! That way it wouldn't be a chance to the semantics of data.

@taion That sounds good! However, one problem is making sure the structure of optimisticResponse is the same as the one in the expected response from the mutation defined by the user. The commitMutation function does take care of this and will raise an error if the structures don't match. Hence, I don't think there is a need for us handling the mismatch if we just send the optimisticResponse back in a separate optimisticData field like you mentioned. Thoughts?

morrys commented 3 years ago

I am not convinced by the idea of adding a new field.

In relay, optimistic responses are returned in fragments as if they were real responses.

Also in this case I would set the data field with the optimisticResponse.

immagine

shreyas44 commented 3 years ago

If I'm not wrong, according to the Relay docs the data in optimisticRepsonse updates those values only in the store. The onCompleted callback is still run only when a response is returned by the server. And in the useMutation hook, the value of data is set only when the onCompleted callback is run.

A similar thing is done in ApolloClient as well. The optimistic response updates the data only in the store. The onCompleted callback is run only when the server returns a response.

Pros of having a separate field:

Cons of having a separate field:

morrys commented 3 years ago

updates those values only in the store.

this means that each subscribed fragment will be updated as if a response had arrived from the server

We stick to relays model of optimistic response

I disagree that this is the way to stick to the Relay model. Precisely for the answer above. data in useMutation is similar to data in useFragment.

Also, you can distinguish that the data is real or optimistic through the isLoading property. When isLoading is true, data is optimistic otherwhise when isloading is false the data is real.

shreyas44 commented 3 years ago

data in useMutation is similar to data in useFragment

Isn't the data field supposed to contain the response of the query, the same as the data argument in the onCompleted callback? But If the data in useMutation was meant to act similar to the data is a useFragment as you mentioned then having the data contain the optimistic response does makes more sense. Sorry for the confusion...

Also, you can distinguish that the data is real or optimistic through the isLoading property. When isLoading is true, data is optimistic otherwhise when isloading is false the data is real.

Ahh yes!

morrys commented 3 years ago

released with version 4.1.0