-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate ignoreResults in useMutation #12296
Deprecate ignoreResults in useMutation #12296
Conversation
👷 Deploy request for apollo-client-docs pending review.Visit the deploys page to approve it
|
🦋 Changeset detectedLatest commit: 13dc2f6 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 |
|
@Cellule we want to keep the That option was added to |
Thanks for the details! It does make a lot of sense. |
Oops, I repointed this branch to our 3.13 release before I merged the latest from |
9c368a0
to
b844568
Compare
b844568
to
13dc2f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent. Thanks for doing this!
2422df2
into
apollographql:release-3.13
As for What do I mean by that? Hooks are mostly defined by the fact that they call a React hook. That means they need to do one of the following:
|
/** {@inheritDoc @apollo/client!MutationOptionsDocumentation#ignoreResults:member} */ | ||
/** | ||
* {@inheritDoc @apollo/client!MutationOptionsDocumentation#ignoreResults:member} | ||
* @deprecated This property will be removed in the next major version of Apollo Client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add a bit more information what to use instead?
"Instead, please use useApolloClient
to get your ApolloClient
instance and call client.mutate
directly."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I wonder is, if there's really a good reason to have an optimized version of mutate that doesn't hook into the lifecycle.
Maybe we'd want to offer a helper to do that.
Something like useMutate
(I'm bad with naming things...) that would very simply take a mutation document, maybe an optional client and return a useCallback
wrapper around client.mutate
That would simplify the implementation and make a clear distinction between cases where you might want to use data, loading state or some other information about the mutation through useMutation
and a more lightweight version that won't cause a rerender when the mutation is called.
I'd wait to hear community feedback before implementing such helper, but I agree maybe there should be a bit more details in the deprecation notice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really argue "this shouldn't be a hook" and "people shouldn't be afraid just touching the client" here.
One thing I wonder is, if there's really a good reason to have an optimized version of mutate that doesn't hook into the lifecycle.
I mean, you have used it in a good number of places, so there seems to be demand to at least do this from time to time? :D
I could see places where you might want to just trigger a bunch of mutations and are really not interested in the result, and it might be so many mutations that you would get a performance problem from the resulting renders. That said, I would also say that these places would not be the root problem, and instead those "expensive renders" would need investigating, - but that doesn't mean that the use case doesn't exist.
And it's certainly nothing we'd encourage, as, as I said, the root of the problem usually would be a performance problem somewhere else - and as it can be done without a hook I'd just not provide such a hook.
maybe there should be a bit more details in the deprecation notice
Could you open a follow-up PR? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, you have used it in a good number of places, so there seems to be demand to at least do this from time to time? :D
To be fair, it was used like a default way of doing things which lead to numerous bugs and confusion.
A lot of devs would have a separate state to track the loading state and try to update it manually instead of simply using const [mutate, { loading }] = useMutation()
Could you open a follow-up PR? :)
Looking into it right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #12301 we can continue the discussion there :)
Really good explanation of the difference between both use cases! |
From discussion in #12277
ignoreResults
is more likely to cause confusion than actually help since the results are always returned.The option potentially reduces the number of rerenders where the mutation is hooked, but I'm skeptical of the necessity and effectiveness of this.
If there's really a need for performance around state management in
useMutation
then there could be a separate hook more specialized for this that has a clear contract and expected behavior instead of a somewhat unused state that doesn't always gets refreshed@jerelmiller I was wondering if we want to also deprecate
ignoreResults
inuseSubscription
. I admit I didn't even know that was an option there so I didn't dig