React-apollo: Reconsider useMutation hook API

[gajus]

The current API looks like this:

const [
  scrapeVenuesResult,
  {
    error,
    loading,
  },
] = useMutation(SCRAPE_VENUES_MUTATION);

This looks neat on its own, but becomes a problem when there are many mutations since all of the variables now need to be aliased and end up looking more like:

const [
  scrapeVenues,
  {
    error: scrapeVenuesMutationError,
    loading: scrapeVenuesMutationLoading,
  },
] = useMutation(SCRAPE_VENUES_MUTATION);

A neater API would return a single object describing the entire state of the mutation as a property the mutation function, e.g.

const scrapeVenues = useMutation(SCRAPE_VENUES_MUTATION);

scrapeVenues();
scrapeVenues.error;
scrapeVenues.loading;
scrapeVenues.data;

This would automatically provide a namespace for all properties of a mutation.

[gajus]

For the impatient ones, here is an ad-hoc decorator:

// @flow

import {useMutation as useApolloMutation} from '@apollo/react-hooks';

const useMutation = (query) => {
  const mutationTokens = useApolloMutation(query);

  const mutation = mutationTokens[0];

  mutation.data = mutationTokens[1].data;
  mutation.error = mutationTokens[1].error;
  mutation.loading = mutationTokens[1].loading;

  return mutation;
};

[zsolt-dev]

this is actually much nicer than the tuple. Please make this default

[hwillson]

Thanks for the suggestion. While there are pros/cons to both approaches, this isn't something we're interested in changing at this time. As you've shown, it's quite straightforward to create your own custom wrapper Hook to get things setup the way you prefer.

[gajus]

This project isn't released yet officially, and it is already not community driven. Shame.

Put it to a vote. Keep it open to get actual user feedback.

[hwillson]

@gajus - this topic has been discussed at some length in other areas, and we've adopted the preferred API that has already been voted for by an overwhelming majority by the react-apollo-hooks adopters (https://github.com/trojanowski/react-apollo-hooks/pull/93). React Apollo is community driven, and we welcome all constructive feedback, but at the end of the day the maintainers have to make decisions or nothing will ever ship. In this case we're going with the tuple approach because react-apollo-hooks has already shown that this is what people prefer (so far anyways), and also because this approach is more in-line with the React Hooks API.

Again, if you don't like this approach it's amazingly easy to create your own wrapper Hook to do what you want.

If an overwhelming majority feel otherwise, I'm sure we'll hear about it (and reconsider). Keeping this issue open isn't going to change anything though, since we use GH issues to track bugs not feature requests. Please consider opening an FR over in https://github.com/apollographql/apollo-feature-requests. Thanks!

[zsolt-dev]

@hwillson - it seems like @gajus solution was not proposed at the time react-apollo-hooks was considering the tuple solution (trojanowski/react-apollo-hooks#93)

Both projects are in beta and it makes sense to use the best possible solution now instead of everyone writing their own wrappers.

[hwillson]

I'm all for coming up with the best possible solution, but from a library maintainers point of view, the solution proposed here worries me. If we limit the public API to only return an object from useMutation in the manner shown, then we'll have to assign everything returned by useMutation to the mutate function itself:

const scrapeVenues = useMutation(SCRAPE_VENUES_MUTATION);
scrapeVenues();
scrapeVenues.error;
scrapeVenues.loading;
scrapeVenues.result;

The proposed solution is mixing mutation result "state" (loading, error, data) into the mutate function object, which in theory really should be immutable. This seems like an unnecessary coupling that could lead to problems later on. Also, if we limit the public API to only return an object from useMutation, then we'll have to assign everything returned by useMutation to the mutate function itself. This could greatly limit our ability to add new features without a major version bump. Keeping the result as a tuple means we're free to return new things as we build out new features, that might not necessarily be related to the mutate function.

cc @benjamn @jbaxleyiii @peggyrayzis for your thoughts.

[gajus]

then we'll have to assign everything returned by useMutation to the mutate function itself:

That is correct.

The proposed solution is mixing mutation result "state" (loading, error, data) into the mutate function object, which in theory really should be immutable.

"in theory should be immutable" What theory?

Function is just an object with a callable interface.

For what it is worth, this design is used by another popular library – Ajv:

Validation errors will be available in the errors property of Ajv instance (null if there were no errors).
Please note: every time this method is called the errors are overwritten so you need to copy them to another variable if you want to use them later.

This seems like an unnecessary coupling that could lead to problems later on.

If you do not elaborate on "could lead to problems later on", then this statement is just spreading FUD.

Also, if we limit the public API to only return an object from useMutation, then we'll have to assign everything returned by useMutation to the mutate function itself.

Correct.

This could greatly limit our ability to add new features without a major version bump. Keeping the result as a tuple means we're free to return new things as we build out new features, that might not necessarily be related to the mutate function.

Illogical. If you add new properties, you assign them to the function, just like you would add them to the tuple. If you remove properties, then it will be a major bump either way.

[hwillson]

@gajus I'm just not seeing the benefits this provides over the current tuple based approach. Looking at this another way, why can't you just use:

const [scrapeVenues, scrapeVenuesResult] = useMutation(SCRAPE_VENUES_MUTATION)
scrapeVenues();
scrapeVenuesResult.error;
scrapeVenuesResult.loading;
scrapeVenuesResult.data;

Isn't that cleaner, showing a clear separation between the mutate function and its results?

[gajus]

The tuple approach downside is that it introduces an unnecessary second variable without a reason.

Your primary argument against is that "function" should be immutable because it feels cleaner that way. This line of thinking appears to stem from a desire to adhere to pure function requirements. However, mutation function is impure and its side effects are described using error, loading and data properties (among others). Therefore, the separation just for the purpose of appearance obscures existence of the side-effects.

Put it another way: There is not a scenario where someone would need to use scrapeVenues without access to either error, loading or data properties. Also, there is not a scenario where anyone would need access to error, loading or data without scrapeVenues. They all represent an atomic unit – mutation, which can take different states.

[hwillson]

The tuple approach downside is that it introduces an unnecessary second variable without a reason.

I could make the exact same argument regarding adding error, loading, and data to the mutate function object. Why am I adding 3 new properties to the mutate function object when I could just add a second variable to the tuple to hold the result? Honestly, introducing a second variable (that can be completely ignored if you don't need it) is okay in my books.

Your primary argument against is that "function" should be immutable because it feels cleaner that way.

That's one of my arguments, because it really is cleaner that way - especially to newcomers. It's easier to grok that we have a mutate function that can be called to fire a mutation, and we have a result object that holds the results of the mutation. It's also much easier to type things this way.

Put it another way: There is not a scenario where someone would need to use scrapeVenues without access to either error, loading or data properties.

There are actually many situations where people want to trigger the mutate function without caring about the useMutation result object (they can be watching for state changes in other ways).

@gajus we can go back and forth on this indefinitely. Given that it's amazingly easy to achieve what you want to do with the tuple approach (https://github.com/apollographql/react-apollo/issues/3303#issuecomment-517928181), I'm just not convinced there is any reason to change this. I've cc'd other core team members for their input, and I welcome others to chime in as well. Thanks!

[jdmg94]

How about returning the function as part of the same object as error, loading, and data

const scrapeVenues = useMutation(SCRAPE_VENUES_MUTATION);

scrapeVenues.execute(); // also part of the namespace
scrapeVenues.error;
scrapeVenues.loading;
scrapeVenues.data;

This would achieve the separation without mutations

[hwillson]

@jdmg94 but then we're forcing people to have their mutate function named execute. With the tuple approach it can be called anything they want.

[jdmg94]

@hwillson they can still provide it with its own name which makes semantic sense, it encourages naming that reveals intent.

[hwillson]

But I just don't see how:

const scrapeVenues = useMutation(SCRAPE_VENUES_MUTATION);
scrapeVenues.execute();
scrapeVenues.error;
scrapeVenues.loading;
scrapeVenues.data;

is more clear than:

const [scrapeVenues, scrapeVenuesResult] = useMutation(SCRAPE_VENUES_MUTATION);
scrapeVenues();
scrapeVenuesResult.error;
scrapeVenuesResult.loading;
scrapeVenuesResult.data;

If we assume both of these are just as clear as each other, then we need to look for other pros/cons. In my mind the ability to separate the mutate function from the result set, which we're doing in the second example, is cleaner and more flexible.

Let's look at this with another example. First way:

const saveDolphin = useMutation(SAVE_DOLPHIN);
saveDolphin.execute(); // Wait, what??? NOOOOOOO!
saveDolphin.error;
saveDolphin.loading;
saveDolphin.data;

Second way:

const [saveDolphin, saveDolphinResult] = useMutation(SAVE_DOLPHIN);
saveDolphin(); // Ahhh, nice and safe.
saveDolphinResult.error;
saveDolphinResult.loading;
saveDolphinResult.data;

😹

[jdmg94]

@hwillson I see your point

[gajus]

Let's not get distracted. The proposal is for:

const saveDolphin = useMutation(SAVE_DOLPHIN);
saveDolphin(); // Ahhh, nice and safe.
saveDolphin.error; // Ahhh, because it describes the current state of `saveDolphin`.
saveDolphin.loading;
saveDolphin.data;

[dai-shi]

@gajus Here's my two cents.
I originally thought keeping with react-apollo-hooks would be good for transition.
But, this would be a last moment to make a new proposal. So, I want to take it seriously.

I remember it's noted that in many cases people would use the mutation function without the status object. If this is the case, I prefer

the proposal:

const saveDolphin = useMutation(SAVE_DOLPHIN); // looks much cleaner

to the current api:

const [saveDolphin] = useMutation(SAVE_DOLPHIN); // destructuring seems an extra notation

I think array destructuring should be used if it's used as a tuple in most of the use cases.

However, I have a big concern with the proposal. It's about referential equality. (related comment)

Consider the example:

const ParentComponent = () => {
  const saveDolphin = useMutation(SAVE_DOLPHIN);
  // ...
  return (
    <div>
      {/* ... */}
      <ChildComponent mutationState={saveDolphin} />
    </div>
  );
};
const ChildComponent = React.memo(({ mutationState }) => {
  const { loading } = mutationState;
  return (
    <div>
      {loading ? 'loading' : 'not loading'}
    </div>
  );
});

To make this work, we need to create new mutation function every time mutation result is changed. Something like: useCallback(() => mutate(), [error, loading, data])

This is not very common because usually it's better to keep function referential equality. (and that's what useCallback is for.)
But, if this is acceptable, the proposal can be an option.
Otherwise, the current tuple approach would be better.

P.S. Personally, I don't 100% agree with [mutation, result] pattern because it's too different from [state, update] pattern. So, execute() seems a still option to me, but if this is just about a preference, I agree with keeping react-apollo-hooks api.

[jinshin1013]

const [scrapeVenues, scrapeVenuesResult] = useMutation(SCRAPE_VENUES_MUTATION)
scrapeVenues();
scrapeVenuesResult.error;
scrapeVenuesResult.loading;
scrapeVenuesResult.data;

Above is much easier to reason about than below:

const scrapeVenues = useMutation(SCRAPE_VENUES_MUTATION);

scrapeVenues();
scrapeVenues.error;
scrapeVenues.loading;
scrapeVenues.data;

It clearly separates out the mutation/execution to its side effect loading, data and error . Also maybe it's because I'm not a so-called Software Engineer but it feels rather weird and unnatural to do mutation() and with the same variable do mutation.loading. It almost feels dirty and hacky.

[gajus]

Part of the problem is that you are calling the second variable *Result (scrapeVenuesResult). It is not a result and the fact that you think of it as a result is the _primary_ reason why I object to this design because it misrepresents and obscures the role of the value.

The scrapeVenuesResult object describes these properties:

data?: TData;
error?: ApolloError;
loading: boolean;
called: boolean;
client?: ApolloClient<object>

Only data and error could be considered a "result" of an operation. All of the other properties describe the state of the mutation. Therefore, calling it scrapeVenuesState would be far more appropriate.

However, then you realise that scrapeVenues has a state. If scrapeVenues has a state then it is not a pure function. Indeed, scrapeVenues is a _callable object with a state_. That state is currently described separately from the callable interface for no reason whatsoever and it requires user to pick an arbitrary name to describe scrapeVenuesState, scrapeVenuesResult, scrapeVenuesContext or whatever else arbitrary name user will chose that describes the intended use case of the mutation at the time. Indeed, I will further argue that the lack of clarity to the user as to what actually the second variable is the the primary reason why every single article and codebase that I have seen opts-in to use destructuring assignment instead, i.e.

const [
  scrapeVenues,
  {
    error: scrapeVenuesMutationError,
    loading: scrapeVenuesMutationLoading,
  },
] = useMutation(SCRAPE_VENUES_MUTATION);

Stop thinking of scrapeVenues as a pure function (which it isn't) and think of it as a callable object with a state and it will stop feeling awkward to have properties attached to it.

[gajus]

Perhaps @gaearon you have an opinion on the subject?

If I am wrong, I would certainly not want to mislead the entire community to adopt an erroneous pattern. However, I feel like that the current use of destructuring assignment with hooks promotes unnecessary proliferation of arbitrary named variables and does no good to explain that hooks such as useMutation describe a continuous state similar to that observable in reactive programming than paradigms associated with pure functions and atomic actions.

[jinshin1013]

I get what you are trying to describe here and no one is wrong here as both views have its valid and legitimate grounds to support. Correct me if I'm wrong but if the performance isn't the problem and essentially what we are arguing here is purely how the API looks, as @hwillson mentioned above the community at react-apollo-hooks had discussed and decided to use the current implementation. The project started almost 9 months ago and the community went through several iterations and got to where we are now. You are not misleading any community here, you are simply recommending what you think it might work better for you and maybe for some people. For me and probably most people out there are using this just fine and as you wrote it above you can very easily create a wrapper to make it work the way you want. Just putting my opinion out there, I'm not smart enough to even start talking about all the nitty gritty stuff but just saying