search

grassrootsgrocery / admin-portal

GNU Affero General Public License v3.0
11 stars 6 forks source link

70 invalidate queries #146

Closed jasoncavanaugh closed 1 year ago

jasoncavanaugh commented 1 year ago

Description of this change

70 Refactor to use invalidateQueries instead of refetch when mutations succeed.

Screenshots (for UI changes - otherwise delete this section)

Before this PR:

\<screenshot(s)>

After this PR:

\<screenshot(s)>

Checklist

railway-app[bot] commented 1 year ago

This PR is being deployed to Railway 🚅

admin-portal: ◻️ REMOVED

jasoncavanaugh commented 1 year ago

So I'm honestly not sure if this is better. It seems to me that the code is a bit less intuitive to a newcomer than it was before (e.g. refetch is a lot easier to understand at a glance compared to invalidateQueries). Additionally, I had to do some refactoring of the query keys to make it a bit more systematic, since otherwise we would have raw strings lying around that are related to each other. This is probably a good change that we should keep in general. But it also meant that I had to separate out data fetching logic in a few places where I otherwise wouldn't have (in general, I prefer keeping everything inline until it needs to be used somewhere else). Another thing that I'm not sure how I feel about is the fact that, while we no longer need to pass around a refetch function as a prop to different components, we still sometimes need to pass around other things instead. For example, if a we had a fetching function that depended on the eventId, in order to invalidate that queries cache via the query key, we would need access to the eventId, which means that it has to be passed around as a prop. So it's not always the case that we are saving ourselves the trouble of passing around props. So I guess I'm still a bit unsure of this change is actually a win for code readability and maintainability. But it seems that people tend to opt for query invalidation with this library instead of refetching, as indicated by the post that I linked in the issue. Would love to hear any thoughts from @mattsahn @6mp @zuechai

mattsahn commented 1 year ago

Thanks, @jasoncavanaugh. I'm going to try to get some opinion on this one from another frontend/React SME on my team, as well.

prndP commented 1 year ago

@jasoncavanaugh Hi Jason, @mattsahn asked me to take a look here.

Under perfect circumstances, we should always be computing the "fetch" via a computed key derived from existing state (if we KNOW some API depend on ABC inputs, then ABC goes into the computation of that key which would automatically be classified as a new fetch). If we do NOT know, then that means there is some hidden implied state that triggers it: either "TIME" or a "USER ACTION", in which case it's perfectly fine to call invalidate.

Looking at this file, (https://github.com/grassrootsgrocery/admin-portal/blob/032e9c98f64158dfc855b0a799113c55ababcc09/client/src/pages/DriverLocationInfo/AssignLocationDropdown.tsx) this is clearly falling in the latter use case. Maybe in a perfect world, the local state exists as an atom with side effects which synchronize with API, but given what you have, it follows that the "read" endpoint for "drivers" must be invalidated and the invalidation happens due to a user initiated action which makes an "UPDATE" call. Any writable operation in a set of CRUD endpoints will naturally invalidate reads.

Good. This actually makes total sense from a reasoning perspective (invalidate being much better than refetch, because we are explicitly stating our intention to invalidate BECAUSE of a write). But, for some reason it "feels bad". I think your instincts are good because your intuition is telling you something is not developer friendly with the current implementation. I believe the root cause is actually not with the dependency itself, but where and when invalidation call is happening.

The reason in my opinion of why it feels icky is because here the "data invalidation" concerns are actually being mixed with the "dropdown" concern. We KNOW the CRUD endpoints are coupled together, but it doesn't actually show that in the code. Why does setting the onSuccess portion of a dropdown menu invalidate my API call? Whether it's refetch or invalidate, neither makes a lot of sense. But if we see invalidateQuery immediately after another successful API call, the reason becomes much more obvious: the very act of calling the "write" portion of a CRUD api invalidates the "read". This has nothing to do with dropdowns, and 100% to do with synchronizing API states.

Even if you change nothing else, if you move the invalidation piece out of the Dropdown callback up to the useMutation hook, it will feel much more natural. We are declaring "update locations" to be an operation which both: writes to the server state, and, upon success, invalidates the existing read state, encapsulating it into one function. This also means it becomes far more portable -> at some point the entire useMutation code might be extracted into a generic "useUpdateDriverLocation" custom hook which always manages this relationship

jasoncavanaugh commented 1 year ago

Thank you for the insightful comment, I think that makes a lot of sense. I will look more into the file you linked to and also try to find similar parts of the code where I can apply a similar reasoning.

jasoncavanaugh commented 1 year ago

Bumping this for review @6mp @mattsahn @zuechai

mattsahn commented 1 year ago

Bumping this for review @6mp @mattsahn @zuechai

@prndP, please have another look, too.

mattsahn commented 1 year ago

Bumping this for review @6mp @mattsahn @zuechai

@prndP, please have another look, too.

Sorry, @jasoncavanaugh, we owe you a review. This one is over my abilities to give a meaningful review so looking for @zuechai and @6mp to chime in. I know @prndP is on vacation for a couple more weeks.

zuechai commented 1 year ago

I can get on this tonight.

On Thu, Sep 28, 2023 at 3:27 PM Matt Sahn @.***> wrote:

Bumping this for review @6mp https://github.com/6mp @mattsahn https://github.com/mattsahn @zuechai https://github.com/zuechai

@prndP https://github.com/prndP, please have another look, too.

Sorry, @jasoncavanaugh https://github.com/jasoncavanaugh, we owe you a review. This one is over my abilities to give a meaningful review so looking for @zuechai https://github.com/zuechai and @6mp https://github.com/6mp to chime in. I know @prndP https://github.com/prndP is on vacation for a couple more weeks.

— Reply to this email directly, view it on GitHub https://github.com/grassrootsgrocery/admin-portal/pull/146#issuecomment-1739887075, or unsubscribe https://github.com/notifications/unsubscribe-auth/ARWFW6JNKZMMQ3MXK6VPYJ3X4XFTDANCNFSM6AAAAAA44BUWVM . You are receiving this because you were mentioned.Message ID: @.***>

zuechai commented 1 year ago

This is awesome. I can see your concern @jasoncavanaugh about readability, but I think after a new contributor reads the React Query docs it'll feel approachable. I haven't used React Query much, but seeing invalidation in practice like this really displays it's strength and practicality. Sorry, it took long to get this. I needed to read through docs for both React Query and Node Query first to feel confident before giving an opinon. Overall, this is awesome and a huge improvement I think!

@prndP Your explanation is really helpful. I really like the idea of encapsulating the example you mentioned into its own hook.