Adjust useMutation result tuple to be more inline with React Core
#3199
Conversation
This commit re-arranges the `useMutation` result tuple such that the mutation result is in the first position, with the mutation function in the second position. The intent of this changes is to bring the `useMutation` Hook more in-line with how the React Core handles its Hook results (e.g. `useState`, `useReducer`). This is a breaking change, but since we're in beta hopefully that's okay. To help lessen the blow a bit this commit includes changes to display a `console.warn` message describing the API changes. This message will be removd before the final version of React Apollo 3 is published. Fixes #3189.
|
Just want to point out that docs (README) are kinda mixed, it shows correct signature coming from this change, but the example below has the "old way". That got me confused and had to look into a source to verify it. |
|
Great point @FredyC - thanks! We're going to be overhauling all of the |
|
Either way, I am a bit surprised by this change, to be honest. The point is, that user might want to execute a mutation and get the result from returned Promise more often than reading it out from scoped result. That means we will have to be skipping tuple variable, eg. It's not a big deal breaker, but on the point of aligning with React Core, there is never use case like that (eg. calling setState without reading value) so the reasoning might be a little bit off in my opinion. |
|
I'm also not fan of this. Having mutation result as promise data should be enough in all cases - if you need, you can just assign it to the state yourself or even create custom hook. It also means component will re-render after mutation which is not needed in many cases. |
|
@pie6k Could you elaborate on your comment a bit further? All this is doing is switching the position of the mutation function and the mutation result object in the returned |
|
@hwillson He probably means if there wouldn't be any internal state for mutation, it wouldn't need to rerender and you would get a result from the Promise... But then there wouldn't much of a difference from calling No, I think that the return result has its uses, especially the Thanks for reconsidering switching arguments over, it indeed makes more sense in most cases. |
|
This is an important one to get right. To rephrase what FredyC said, I think the analogy to "how the React Core handles its Hook result" is misguided. Keeping data types in the same order might be the principle to follow if all else were equal. But the principle of 'optional values should come last' is a stronger convention (like in function arguments), and it's more strongly grounded in practice as well (it avoids |
|
@reverie Hold your horses, there are already considering switching tuple values around ... trojanowski/react-apollo-hooks#187 (comment) |
|
@FredyC When people are considering it is the right time to comment, right? |
|
I wouldn't be making such a fuss around it really. If anyone is bothered by |
|
Seems like you are making a fuss about it :) |
|
Hi all - we're going to switch it back; we just haven't had a chance yet (we just finished a company get together week, so time has been a bit tight). Everything will be back to normal this week, and this (along with other outstanding changes) will be wrapped up. Thanks! |
While the changes requested in #3189 and implemented in #3199 seemed like a good idea, we no longer think it makes sense for React Apollo to align its hooks API with React's core hooks API. In `useMutation`'s case, the mutation function will almost always be called, whereas the results aren't necessarily used. For this reason it makes sense to keep the mutation function in the first position of the result tuple.
This commit re-arranges the
useMutationresult tuple such that the mutation result is in the first position, with the mutation function in the second position. The intent of this change is to bring theuseMutationHook more in-line with how the React Core handles its Hook results (e.g.useState,useReducer).This is a breaking change, but since we're in beta hopefully that's okay. To help lessen the blow a bit this commit includes changes to display a
console.warnmessage describing the API changes. This message will be removed before the final version of React Apollo 3 is published.Fixes #3189.