Fix bug where onCompleted and onError are stale for useMutation. onCompleted and onError could not be changed while a mutation is executing.

[charle692]

fixes #9714
fixes #9493
Related to #9111

Since 3.5.0 there's been a bug with the useMutation hook from this PR that has been preventing us from upgrading to the latest version.

Currently, onCompleted and onError are stale and cannot be updated while a mutation is executing. See here for example on how to reproduce this issue.

The reason for this is that useMutation invokes onCompleted and onError via baseOptions rather than directly from the ref.current. Due to this, it doesn't have the latest copy of the callbacks once the operation is complete.

Checklist:

• If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests

[apollo-cla]

@charle692: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[charle692]

These two specs I added specifically fail without this change, with the change pass. So hopefully this should prevent the issue from being re-introduced.

[brainkim]

I took a look at this. I am confirming that the tests fail when using baseOptions (see attached image). @charle692 Thank you so much for tracking down the stale callback issue, writing tests! I’m really sorry that I introduced this bug and I really appreciate you fixing it.

[brainkim]

@charle692 If you want to rebase this branch against the latest main and delete the .dccache file, I am happy to merge. If you don’t know how to do this I am happy to help or do this for you. Thank you again!

[charle692]

@charle692 If you want to rebase this branch against the latest main and delete the .dccache file, I am happy to merge. If you don’t know how to do this I am happy to help or do this for you. Thank you again!

Sorry about that, I have a vscode extension that keeps adding that in. I think that you removed it already but I'll double check and remove. At minimum I'll rebase with main to get this up to date.

[charle692]

I took a look at this. I am confirming that the tests fail when using baseOptions (see attached image). @charle692 Thank you so much for tracking down the stale callback issue, writing tests! I’m really sorry that I introduced this bug and I really appreciate you fixing it.

@brainkim no worries at all :) just trying to help out anyway I can! Thanks for taking a look at this so quickly, both of you.

[brainkim]

@benjamn I reverted the changes you made attempting to use the merged options. Firstly, it would be incorrect insofar as the useMutation() hook accepts onError() and onCompleted() callbacks both as an option to the hook call and as an option to the execute function call, for some reason, so if both were defined and you used the merged options you would get potentially surprising behavior where only one gets called. Secondly, the contention right now is that there’s a race condition, so I do think the usage of a React ref is appropriate. If you have concerns, please feel free to modify the code in a separate PR.

@charle692 Thank you again and congratulations on the PR. I really do appreciate the time and effort you put into fixing this.

Unrelatedly, there are batch link tests which are failing 😭

[benjamn]

@charle692 @brainkim These changes are available now if you run npm i @apollo/client@next (to get version 3.6.5, just published to npm). Please let us know if we missed something!

[charle692]

@benjamn I reverted the changes you made attempting to use the merged options. Firstly, it would be incorrect insofar as the useMutation() hook accepts onError() and onCompleted() callbacks both as an option to the hook call and as an option to the execute function call, for some reason, so if both were defined and you used the merged options you would get potentially surprising behavior where only one gets called. Secondly, the contention right now is that there’s a race condition, so I do think the usage of a React ref is appropriate. If you have concerns, please feel free to modify the code in a separate PR.

@charle692 Thank you again and congratulations on the PR. I really do appreciate the time and effort you put into fixing this.

No worries, after looking at the specs that were failing I can agree with what you are saying. That if we use clientOptions, and onCompleted or onError callbacks are specified by both executeOptions and the original hook only one of them would get called.

Anyways, thanks to both of you for taking a look at this and getting it merged, really appreciate it :).