LF-5100 - Switch from invalidateTags() to resetApiState() when switching farms

[Duncan-Brain]

Description

While working on the migration from sagas/selectors to rtk-query for /locations I noticed that isLoading variable off of a useGetLocationsQuery() was not acting as expected. I expected that, like logout/login switching farms would reset the state of the rtk query and the cache. But we use api.util.invalidateTags() when switching farms and this will only have the effect of automated re-fetching. Since isLoading is about the first call to an api -- it is not currently reset between switching farms. This makes sense -- it is what invalidatesTags is supposed to do.

Because the api has not been called before with the switched farm details, the isLoading state should also be reset along with the data.

Another utility resetApiState() is chosen to achieve the desired effect of resetting the api data between farms.

A second api is also made to keep the behaviour of not invalidating shared enums/taxonomy/library constants.

Another way to do this could be to add the userFarm details to the call signature of the useGetXQuery() and the Tag key but that seemed like a lot more work. It could be valuable to do that if we expect users switch farms regularly enough to keep both sets of data in cache.

Jira link: LF-5100

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected):
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Passes test case
• UI components visually reviewed on desktop view
• UI components visually reviewed on mobile view
• Other (please explain):

A simple test to monitor isLoading state between farms:

1. Add this to /Home.jsx

// Home
const { refetch: refetchSensors, isLoading: isLoadingSensors } = useGetSensorsQuery();

console.log(isLoadingSensors)

2. Login to sensor farm go to map see isLoading change slowly in console
3. Switch to non-sensor farm go to map,
4. Switch back to sensor farm and see isLoading be false immediately instead of slowly changing:

Checklist:

• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• The precommit and linting ran successfully
• I have added or updated language tags for text that's part of the UI
• I have ordered translation keys alphabetically (optional: run pnpm i18n to help with this)
• I have added the GNU General Public License to all new files

[Duncan-Brain]

Just saw that this could be using the function someone created instead of the whole api.

[kathyavini]

This looks like a good refactor to me! I think I now quite like the two APIs solution (it surprised me at first, though 😂) and I like what you did with BASE_QUERY 🙂

As usual I wish the RTK query docs weren't so frickin' minimal -- for instance, if they described what is the idiomatic use case for resetApiState(). I agree with your logic about everything being uninitialized for the new farm! But this warning in the docs is scaring me a bit 😅 -- did you see this line?

There is a long history of github issues, some resolved and some unresolved, that left me unsure whether that warning even still applies in our version of RTK Query. As far as I can tell, though, the behaviour in app does seem correct.

Just curious -- which component's isLoading state needed the reset? There is also isFetching() which would get reset by invalidateTags; I wonder if we should have been using that one more in place of isLoading more generally, since we do invalidate tags in times other than farm switch. (Edit: I see Sayaka has already been using it in a few places!)

Anyway, it looks good to me! But does it block you on your other location work if we wait until @SayakaOno also just quickly casts her eyes over this when she gets back?

[Duncan-Brain]

@kathyavini I wondered whether you would want to wait for Sayaka! Yup whatever is fine with me, you can only approve what you are comfortable with!

The isLoading is on the...not yet pushed branch for rtk locations but it is merged there already. And do I think the second solution is better to use userFarm as a tag and query variable but just a lot more effort. I added a small test in the description so it can be seen.

I did see that line! And I think what it is saying is that like a useState() not unmounted could still hold data.. makes sense as an fyi.

I don't think the github issues is anything.... here is invalidateTags long list of issues some resolved and some unresolved.

I agree finding the best use case for isFetching vs isLoading would be helpful here is one answer i saw: https://stackoverflow.com/questions/75401477/reasons-for-using-isloading-or-isfetching-in-react-query

[kathyavini]

I don't think the github issues is anything.... here is invalidateTags long list of issues some resolved and some unresolved.

I just mean someone had specifically asked if that line/warning about resetApiState should be removed from the documentation now (to no answer), but I couldn't follow the full trail of PRs and releases to know when / if the fix would have gone in. Most of the issues are on this particular thing about hooks.

Incidentally I just went back to GitHub to find that issue thread, but instead randomly stumbled upon Mark Erikson recommending against multiple API instances here and here and in this issue request

we generally recommend that there should normally only be one API slice per application

Yeah, if at all possible, we'd recommend using a single API slice instance with injectEndpoints() instead of multiple separate instances. (This also will improve overall app perf, because each added API middleware is additional function calls that have to run in response to every dispatched action even if it's not related to the API logic.)

So that's more to chew on there! Will definitely have to defer to Sayaka on her return now 😂 Do you happen to have any links or reading on the two apiSlice approach?

[Duncan-Brain]

It looks like the fix is in 2.11.2 but I really don't think it would affect us because we navigate to a new page when we use it -- their bug sandbox is here CodeSandbox.

That guy is sharing the general recommendation from the docs about multiple api's twice on the same issue# which is about a feature request we don't need 🤷 . A general recommendation is not a strong recommendation against our use case... which we seem to agree would be temporary until we decide to migrate to using Farm or UserFarm as a cache key.

This bug may already be adversely affecting behaviour wherever we use isLoading.

Since the reccommendation is about automatic invalidation (which we don't need) and performance (the example discusses 10 createApi()'s would be noticeably slower. I think we are okay to use it temporarily.

Your worried so we can definitely wait til Sayaka is here.. but I consider it a medium high bug or potential bug right now. So if we don't like this I will probably work on the cache key solution asap.