-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
(docs): add typescript example for optimistic updates #1366
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/tannerlinsley/react-query/arbmppxht |
return previousValue | ||
}, | ||
// On failure, roll back to the previous value | ||
onError: (err, variables, previousValue) => { | ||
queryClient.setQueryData('todos', previousValue) | ||
}, |
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.
@boschni I couldn't get type inference to work properly here. It seems that TContext
is not inferred by the return value of onMutate
- it's always unknown
for me. I know that we can add generics to the useMutation
call, but it would have to be all 4 of them 😅 .
It works here coincidentally because unknown
is valid for setQueryData
if no generic is specified. What I really wanted to do is return a rollback
function:
return () => queryClient.setQueryData('todos', previousValue)
},
// On failure, roll back to the previous value
onError: (err, variables, rollback) => {
rollback()
},
but this is then not possible ... :(
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.
Yeah it seems like TypeScript is not able to infer the context correctly anymore when the context is actually used in any of the onError/onSuccess/onSettled functions. It does infer it correctly when only specifying onMutate. I'll need to figure out why this happens and how this can be fixed
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.
It only infers correctly if you do one of the following things:
- explicitly type the variables argument in
onMutate
. - omit the variables argument in
onMutate
. - explicitly type the context argument in
onError/onSuccess/onSettled
.
This is probably related to microsoft/TypeScript#26242. Not sure what we can do to fix it.
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.
so interestingly, if I define the type of variables
only on onMutate
, but not on the mutationFn
, the variables are still correctly inferred everywhere, including mutationFn
.
rollback then has the correct type, but it will be potentially undefined:
do you know where the union with undefined is coming from? Because if we can solve that, I think thats a "good enough" approach because we only have to define variables once (onMutate
). If not, I would just stick with the current approach because it works and people can always annotate their types manually. what do you think?
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.
In case of onError
the context could be undefined
if the onMutate
function errored. At onSuccess
it should always be available though (will fix the types for that). Think defining the variables on onMutate
is a good compromise.
Apart from that, maybe better to use actual data as context because a rollback function cannot be serialized and persisted: https://react-query-beta.tanstack.com/guides/optimistic-updates
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.
Cool, I’ll update to that syntax later today, thanks
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.
done: 855b202
There is no need to add generics to userQuery now - it will be inferred correctly by the options
Deployment failed with the following error:
|
@tannerlinsley do you want me to open a new PR to the new master branch, or was there a different reason for closing it 🤔 ? |
Oh dang. Didn’t realize it removed the branch lol. Just reopen to master :) so sorry. |
Hehe sure, no worries 👍 |
per popular demand, I added a typescript example for v3
maybe it's not so good to have the same example twice? But how to get types right for optimistic updates has been asked a lot lately... I can try to come up with something else if you want, or maybe we can replace the javascript one with this one or so...