additionalTypenames should be a property of useMutation contex?!

[ivan-kleshnin]

additionalTypenames is a great feature I always lack in Apollo Client.

Hovewer, it's current API pends a question about additionalTypenames passing location.
It's a query-level feature, which has nothing to do with variables.

I'm going to pass the same additionalTypenames in all execute calls... right?
If so, it signalizes that it should be moved to useMutation context.

Current API:

let [r2, deleteVote] = useMutation(DELETE_VOTE_MUTATION)

deleteVote(variables1, {
  additionalTypenames: ["Vote"]
})

deleteVote(variables2, {
  additionalTypenames: ["Vote"] // each `deleteVote` / `execute` will have the same additionalTypenames
})

vs

let [r2, deleteVote] = useMutation(DELETE_VOTE_MUTATION, {
  additionalTypenames: ["Vote"]
})

or rather

let [r2, deleteVote] = useMutation({
  query: DELETE_VOTE_MUTATION,
  additionalTypenames: ["Vote"]
})

to make it closer to useQuery.

I have little experience with URQL so please correct me if I'm wrong about the use patterns and API.

It's just a question/suggestion not a rock-solid opinion from my side.

[andyrichardson]

Hey @ivan-kleshnin thanks for the detailed proposal! 👋

I see where you're coming from, it would be nice to declare the additionalTypenames attribute alongside the mutation hook.

I'm curious, are there many use cases where the execute function is being called from multiple places?

For myself, I can't think of a use case where I've had more than one handler for a mutation - even if said mutation is triggered by multiple sources.

Here's an example:

// ...
const [mutationState, executeMutation] = useMutation(SomeQuery);

// Trigger mutation - for both click and "Enter" key press
const handleSubmit = useCallback(
  () => executeMutation({ ...someAppState }, { additionalTypenames: ["Vote"] }), 
  [someAppState, executeMutation]
);

const handleKeyPress = useCallback((e) => {
  if (e.key !== "Enter") {
    return;
  }

  handleSubmit();
}, [handleSubmit])

return (
  <>
    <input {...inputStateProps} onKeyDown={hanldeKeyPress} />
    <button onClick={handleSubmit}>Submit</button>
  </>
);

[ivan-kleshnin]

I'm curious, are there many use cases where the execute function is being called from multiple places?

Yes, thank you. To be honest, I also didn't have such case in my code. But my key concern is Readability rather than DRY rule.
That list of types is an important part of a query, a declaration of intents. And it's, for sure, more readable near useQuery rather than execute (which is typically folded and hidden in some other function).

[kitten]

I have a longer comment with my thoughts here: #1139 (comment)

I also want to quickly point out that generally, especially if a wrapper callback is used where it makes sense, that executeMutation will always be right next to useMutation, or at the very least, displayed in the same file, so I don't think that DRY/Readability is the main concern here. I think the main thing I was focusing on is the intent and direction of the designed API.

[ivan-kleshnin]

Thank you for the clarifications @andyrichardson, @kitten

Not a big deal, I'll probably get used to it. I have another small question if that's a good place to ask:

Why variables in useQuery are memoized (in the use-memo sense) automatically but for context you'd have to wrap it in useMemo manually:

useQuery({
  query: QUERY,
  variables: {}, // -- no need of useMemo
  context: useMemo({ // -- required to avoid an infinite loop
    additionalTypenames: ...
  }, []),
})

I personally don't see memoization here as an expensive operation in terms of memory. So I'd probably memoize context as well to avoid verbosity on the calling side.

Also, please consider that additionalTypenames flag looks like something ALL index and aggregate queries are going to have. For the first group, you shouldn't rely on non-emptiness of responses and, for the second group, it's the only chance to auto-update queries (imagine query { postsTotal: Int }, it won't auto-update without additionalTypenames)

Simultaneously, the verbosity of this flag context + additionalTypenames + useMemo implies it's something you shouldn't be using for simple cases. So we have two mutually exclusive mental signals 🤷‍♂️

[JoviDeCroock]

@ivan-kleshnin this is because we can't assume for you when the context should trigger a refetch for the stream. For instance, does a change of your fetchOptions need a refetch? We can't assume that for you hence why we decided to go with this approach.

A change in variables always means a refetch

[ivan-kleshnin]

👌 fair enough, thank you

[kitten]

It’s also worth mentioning that it’s two fold, because React has a solid primitive for memorisation (useMemo) which allows for more control, and context could in some cases be nested and contain non-serialisable values (like functions)

In Svelte on the other hand for instance it is in fact memoised