Changed return type of useMutation when ignoreResult is explicitly set to true to hide unset result #12277

Cellule · 2025-01-15T17:02:16Z

We've encountered numerous bugs around this where the default (somehow) was to set ignoreResult: true when calling useMutation
Then devs would try to use data or loading from the tuple without realizing it would never be set.
I've been using this patch for a while and it does help avoid bugs

changeset-bot · 2025-01-15T17:02:20Z

🦋 Changeset detected

Latest commit: 185c889

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Patch

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

netlify · 2025-01-15T17:02:21Z

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	`185c889`

svc-apollo-docs · 2025-01-15T17:03:07Z

✅ Docs preview ready

The preview is ready to be viewed. View the preview

File Changes

0 new, 2 changed, 0 removed

* (developer-tools)/react/(latest)/development-testing/schema-driven-testing.mdx
* (developer-tools)/react/(latest)/development-testing/testing.mdx

Build ID: a6b98535a5473ca491c3f8c8

URL: https://www.apollographql.com/docs/deploy-preview/a6b98535a5473ca491c3f8c8

…t to true to hide unset result

jerelmiller · 2025-01-17T00:19:16Z

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!

jerelmiller

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!

jerelmiller · 2025-01-17T21:30:21Z

src/react/hooks/useMutation.ts

+): [
+  MutationFunction<TData, TVariables, TContext, TCache>,
+  // result is not reliable when ignoreResults is true
+  Pick<MutationResult<TData>, "reset">,


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

const [result, setResult] = React.useState<Omit<MutationResult, "reset">>({

called: false,

loading: false,

client,

});

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.

The biggest problem I've seen is that while the properties are there at runtime, their value is always unset.
Trying to use them is almost sure to be a bug.

In my opinion, I feel that at runtime the unreliable fields should not be returned, but that would be breaking change and I wasn't keen on going down that route.
Even changing the type has the potential to cause typescript errors after update, but it should in theory find potential bugs in the code.

I see that client is potentially correct IF not provided when calling the mutation

Now that I review it a bit more, I now notice that in the catch case it'll actually set the result to have an error
But if the mutation is called another time and succeeds, the error won't be cleared in the result making it even more unreliable

We've got a planning meeting tomorrow so let me take this to the team and get their thoughts on this.

jerelmiller · 2025-01-17T21:30:44Z

src/react/types/types.ts

-    // TODO This FetchResult<TData> seems strange here, as opposed to an
-    // ApolloQueryResult<TData>
-  ) => Promise<FetchResult<MaybeMasked<TData>>>,
+  mutate: MutationFunction<TData, TVariables, TContext, TCache>,


💯 I have no idea why this wasn't that way to begin with. Good find!

Hahah yeah, I went down the route of making a new type... only to realize it already existed 😅

jerelmiller · 2025-01-17T21:31:12Z

src/react/hooks/__tests__/useMutation.test.tsx

+      expectTypeOf(result).toMatchTypeOf<{ reset: () => void }>();
+      expectTypeOf(result).not.toMatchTypeOf<MutationResult<Mutation>>();
+      expectTypeOf(mutate()).toMatchTypeOf<Promise<FetchResult<Mutation>>>();
+      const {


Can you update this to check client and called as well?

Done, I rewrote it a bit, the destructuring was not needed and made the test longer than needed

jerelmiller · 2025-01-22T17:36:07Z

@Cellule we met as a team and discussed this change a bit more.

The more we talked, the more we questioned why ignoreResults made any sense at all. In summary:

This hook is not affected by cache updates.
Rerenders only take place when you call the mutate function.
You can use client.mutate directly (along with useApolloClient to get access to the client) to achieve this same behavior.

What we'd like to do instead is deprecate this option and remove it in v4. Would you be willing to update this PR to mark this field as @deprecated? I'd love to get that change in our upcoming 3.13 release.

Cellule · 2025-01-22T20:27:09Z

@Cellule we met as a team and discussed this change a bit more.

The more we talked, the more we questioned why ignoreResults made any sense at all. In summary:

This hook is not affected by cache updates.

Rerenders only take place when you call the mutate function.

You can use client.mutate directly (along with useApolloClient to get access to the client) to achieve this same behavior.

What we'd like to do instead is deprecate this option and remove it in v4. Would you be willing to update this PR to mark this field as @deprecated? I'd love to get that change in our upcoming 3.13 release.

I'm all up for that!
I've slowly started to change our default way of doing things to stop passing ignoreResults since it is more likely to cause confusions than anything.
I'll update my PR

Cellule · 2025-01-22T20:28:04Z

actually, I'll just close this PR and open a new one since there's really nothing shared between the 2

Changed return type of useMutation when ignoreResult is explicitly se…

ac38202

…t to true to hide unset result

Cellule force-pushed the useMutation-ignoreResults branch from ef8b036 to ac38202 Compare January 15, 2025 17:08

jerelmiller requested changes Jan 17, 2025

View reviewed changes

More thorough test

185c889

Cellule closed this Jan 22, 2025

Cellule mentioned this pull request Jan 22, 2025

Deprecate ignoreResults in useMutation #12296

Merged

github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed return type of useMutation when ignoreResult is explicitly set to true to hide unset result #12277

Changed return type of useMutation when ignoreResult is explicitly set to true to hide unset result #12277

Cellule commented Jan 15, 2025

changeset-bot bot commented Jan 15, 2025 •

edited

Loading

netlify bot commented Jan 15, 2025 •

edited

Loading

svc-apollo-docs commented Jan 15, 2025 •

edited

Loading

jerelmiller commented Jan 17, 2025

jerelmiller left a comment

jerelmiller Jan 17, 2025

Cellule Jan 20, 2025

jerelmiller Jan 22, 2025

jerelmiller Jan 17, 2025

Cellule Jan 20, 2025

jerelmiller Jan 17, 2025

Cellule Jan 20, 2025

jerelmiller commented Jan 22, 2025

Cellule commented Jan 22, 2025

Cellule commented Jan 22, 2025

	const [result, setResult] = React.useState<Omit<MutationResult, "reset">>({
	called: false,
	loading: false,
	client,
	});

Changed return type of useMutation when ignoreResult is explicitly set to true to hide unset result #12277

Changed return type of useMutation when ignoreResult is explicitly set to true to hide unset result #12277

Conversation

Cellule commented Jan 15, 2025

changeset-bot bot commented Jan 15, 2025 • edited Loading

🦋 Changeset detected

netlify bot commented Jan 15, 2025 • edited Loading

👷 Deploy request for apollo-client-docs pending review.

svc-apollo-docs commented Jan 15, 2025 • edited Loading

✅ Docs preview ready

jerelmiller commented Jan 17, 2025

jerelmiller left a comment

Choose a reason for hiding this comment

jerelmiller Jan 17, 2025

Choose a reason for hiding this comment

Cellule Jan 20, 2025

Choose a reason for hiding this comment

jerelmiller Jan 22, 2025

Choose a reason for hiding this comment

jerelmiller Jan 17, 2025

Choose a reason for hiding this comment

Cellule Jan 20, 2025

Choose a reason for hiding this comment

jerelmiller Jan 17, 2025

Choose a reason for hiding this comment

Cellule Jan 20, 2025

Choose a reason for hiding this comment

jerelmiller commented Jan 22, 2025

Cellule commented Jan 22, 2025

Cellule commented Jan 22, 2025

changeset-bot bot commented Jan 15, 2025 •

edited

Loading

netlify bot commented Jan 15, 2025 •

edited

Loading

svc-apollo-docs commented Jan 15, 2025 •

edited

Loading