-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Update useUpdate to use react-query #6891
Conversation
@@ -28,19 +28,7 @@ export const doQuery = ({ | |||
store, | |||
mutationMode, | |||
}: DoQueryParameters) => { | |||
const resourceState = store.getState().admin.resources[resource]; | |||
if (canReplyWithCache(type, payload, resourceState)) { |
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 had to disable application cache because it's incompatible with react-query. React-query gives an equivalent, albeit not controlled by the server. It's a BC break we should accept IMO, as application cache is a minor feature not widely used.
...old, | ||
...data, | ||
}), | ||
{ updatedAt } |
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'm not happy with this one but I didn't find any better solution
); | ||
}; | ||
|
||
let context: { previousGetOne?: any; previousGetList?: any } = {}; |
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's the job of the onMutate
callback to set the context normally, but I actually need to define it beforehand (in the mutate
wrapper), so I'm using this closure.
data: callTimeData = data, | ||
} = variables; | ||
|
||
// optimistic update as documented in https://react-query.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.
The react-query doc advises to do this in onMutate, but in that case it's impossible to fire the success side effects right away. I had to use a wrapper for mutateAsync
(and therefore mutate
) instead.
Besides, the wrapper was necessary for the undoable mode, which shouldn't call the mutate
callback until the action is confirmed.
And we're Ready For Review! |
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.
Awesome work
> = {} | ||
) => { | ||
if (mutationMode === 'pessimistic') { | ||
return mutation.mutateAsync(variables, callTimeOptions); |
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.
If I understand correctly, we don't merge the declaration time options with the calltime ones?
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.
this is done by the mutation callback (the first argument of useMutation)
return mutation.mutateAsync(variables, callTimeOptions); | ||
} | ||
|
||
const { onSuccess, onSettled, onError } = callTimeOptions; |
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.
We ignore the declaration time options?
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.
No, they're taken care of by the useMutation callback
}); | ||
} else { | ||
// undoable mutation: register the mutation for later | ||
undoableEventEmitter.once('end', ({ isUndo }) => { |
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.
How can we be sure this event was dispatched for this specific query? What if there are multiple queries sent in a short timeframe?
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.
We can't. That's the way it'as always worked.
But it's not a real problem as, in order to perform a new mutation, users have to click somewhere, which dismisses the MUI Notification and commits the initial update.
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.
Ok. That's only true for our implementation of the ui though
<Edit successMessage>
prop<Edit>
and<SaveButton>
It turns out react-query's handling of cache, invalidation and stale queries is completely backwards in comparison with http logic, so this was not a piece of cake. I'm not sure we end up with less code to maintain than with our current Redux implementation.