Changed return type of useMutation when ignoreResult is explicitly set to true to hide unset result #12277
Conversation
🦋 Changeset detectedLatest commit: ac38202 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
👷 Deploy request for apollo-client-docs pending review.Visit the deploys page to approve it
|
✅ Docs preview has no changesThe preview was not built because there were no changes. Build ID: b33425ab3a332267bc7cbdd2 |
…t to true to hide unset result
Cellule
force-pushed
the
useMutation-ignoreResults
branch
from
ef8b036 to
ac38202
Compare
|
Hey @Cellule 👋 These are on my TODO list to review tomorrow (Friday). Just wanted you to know I saw these come in and will get to these very soon! |
There was a problem hiding this comment.
This change is great and I do agree it should help prevent some crashes due to the types being wrong.
I had one minor change for accessing fields that are there at runtime even with ignoreResults: true. Once that is updated, I'd be happy to get this into the next release. Thanks for the contribution!
| ): [ | ||
| MutationFunction<TData, TVariables, TContext, TCache>, | ||
| // result is not reliable when ignoreResults is true | ||
| Pick<MutationResult<TData>, "reset">, |
There was a problem hiding this comment.
It looks like we still return loading, called, and client properties even when ignoreResults are set. They just happen to use their initial values:
apollo-client/src/react/hooks/useMutation.ts
Lines 88 to 92 in eeb5f3c
Can we include those properties here as well? I see no reason not to be able to access them, even if loading or called never changes.
| // TODO This FetchResult<TData> seems strange here, as opposed to an | ||
| // ApolloQueryResult<TData> | ||
| ) => Promise<FetchResult<MaybeMasked<TData>>>, | ||
| mutate: MutationFunction<TData, TVariables, TContext, TCache>, |
There was a problem hiding this comment.
💯 I have no idea why this wasn't that way to begin with. Good find!
| expectTypeOf(result).toMatchTypeOf<{ reset: () => void }>(); | ||
| expectTypeOf(result).not.toMatchTypeOf<MutationResult<Mutation>>(); | ||
| expectTypeOf(mutate()).toMatchTypeOf<Promise<FetchResult<Mutation>>>(); | ||
| const { |
There was a problem hiding this comment.
Can you update this to check client and called as well?
We've encountered numerous bugs around this where the default (somehow) was to set
ignoreResult: truewhen callinguseMutationThen devs would try to use
dataorloadingfrom the tuple without realizing it would never be set.I've been using this patch for a while and it does help avoid bugs