LF-4082 RTK Query setup and fetching inventory

[kathyavini]

Description

This creates the RTK Query apiSlice.ts and associated changes. You can see that once that part was done, fetching the inventory into the new container was a piece of cake! ☺️ Right now the inventory displays as a wall of text. Please comment out the tag invalidation line to observe caching.

Reference was @antsgar's wonderful PR #3019 and the only differences were due to the migration to TypeScript.

NOTE: I did not migrate the useQueries custom wrapper, as I could not figure out how to do so without losing the type safety of the built-in RTK-Query hooks -- that is a TS conversion well beyond my skills (and might even be impossible). See code note below.

Jira link: https://lite-farm.atlassian.net/browse/LF-4082

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?

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

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 added "MISSING" for all new language tags to languages I don't speak
• I have added the GNU General Public License to all new files

[kathyavini]

This is the point at which type-checking will be enforced on the returned data based on the interfaces that were provided in apiSlice.ts-- it's fun to see by trying to access existent + non-existent properties on each of those data objects.

Although these can be successfully batched with the wrapper function useQueries linked above:

const { data, isLoading } = useQueries([
  { hook: useGetAnimalsQuery, label: 'animals' },
  { hook: useGetAnimalBatchesQuery, label: 'animalBatches' },
  { hook: useGetAnimalGroupsQuery, label: 'animalGroups' },
]);

const { animals, animalBatches, animalGroups } = data;

I never figured out how to modify the wrapper to still get that type checking.

[Duncan-Brain]

I kind of like the explicitness you have here!

[kathyavini]

I looked a bit more into this and I think the index signature is still the best way to avoid having to write out the whole interface to UserFarm (a Partial would be correct as well, but would still require the base interface which I think we want to avoid in these situations where it's not relevant and we aren't trying to type check on it).

[Duncan-Brain]

I think this interface could go in your new types file as we scope our data hierarchy. I think its good to write out the whole interface but I also am good with the idea of leaving it for now and maybe using whatever we touch as candidates for the next .ts update. Like a contagion we can create a ticket and say we need to update userFarm to .ts next.

[kathyavini]

Yeah, let's talk about that with @antsgar when she returns in terms of how we want to manage the transition over to TS. I would prefer to not write any interfaces that we aren't actively using in code (i.e. where we aren't actually type checking on the properties) because it feels like bloat. So I like the "whatever we touch" approach personally (like the full interface would be written the day we use the userFarm selector in a TS file and need the properties), but it might be easier to establish a system now rather than do piecemeal 🤔

[antsgar]

At this point in time since there's no requirement to use TS I'm honestly alright with going as far as it seems to make sense within each situation for adding types. Enforcing doing full interfaces might increase scope and take a lot of time on some scenarios, whereas on others it might be relatively easy to go all the way. I'm on the "use your best judgement" camp for now, since TS use is still entirely optional even incomplete typing helps

[Duncan-Brain]

Does the new copyright standard just use the one current year now? https://lite-farm.atlassian.net/wiki/spaces/LITEFARM/pages/1269202970/Copyright+GNU+General+Public+License

[kathyavini]

That's only if it's a new file. The copyright represents all the years the specific file was touched/modified.

[Duncan-Brain]

Well I have been doing it wrong then lol -- I remember Ivan sharing some kind of blog saying 1 year was enough. I think I have actually deleted these intervals in favour of 1 year approach.

[kathyavini]

Oh no 😂 The original block we were copy-pasting had all the years that LiteFarm was active, and I think Ivan was just asking us to get rid of all the other dates if the file itself hadn't existed back then. I think this method with the range of dates (from file creation up to last modification) is pretty standard.

[SayakaOno]

@kathyavini are the APIs being called over and over again in /inventory for you?

[SayakaOno]

I think this can be 2024.

[kathyavini]

I wanted to give credit to @antsgar for the code she wrote in 2023 (upon which this was based!) 😄 But I guess since it never got merged, this is a bit confusing.

[SayakaOno]

We need to add internal_identifier and photo_url to Animal and AnimalBatch (PR), can it be done now??

[kathyavini]

@SayakaOno oh goodness! Yes I do see that, and it was not how it used to behave 🤔

Edit: well on the off-chance that it was, and I wasn't looking at the network tab at the time, I'll remove invalidating the tags from the same component... but I could have sworn that used to work safely (with one network request only).

[Duncan-Brain]

Just wondering are we waiting until we have more endpoints to do specific animal tags?

To get both individual animal tags and we could do :

(result, error, userFarm) => {
  const tags= result.map((animal) => return {type: 'Animal', id: animal.id});
  tags.push {return type: 'FarmAnimals', id: userFarm.farm_id}
}

Just have to pass in useFarm on the query selector.

[kathyavini]

Oh shoot I meant to ask about this in meeting this morning because I don't quite follow.

The idea is that you can create a new tag and entity (the singular animal) based upon the /animals endpoint, without ever having to have an actual GET singular animal endpoint? I didn't know that was a pattern, but it sounds interesting.

What I don't follow is the role of userFarm though, since the base endpoint was already only for one farm. I would think the nested tag would want to be associated with animal.id, not farm_id.

[Duncan-Brain]

The idea is you can invalidate tags for an individual animal (update, remove). The backend endpoint/query param will still be needed.

For 'FarmAnimals' tag -- its essentially the same as ['Animals'] you have currently as its goal would be to refresh all animals. I just chose a structure which is probably not necessary.... The example I have from my class uses users.id to go with something like 'UsersMovies' tag. I was just transposing. Animal.id already identifies the individual animal there. in the line above.

[Duncan-Brain]

I mentioned on Antos PR that I think this file is going to get huge. I know the redux docs have this pattern for their examples but in the class I took they did it like this:

store 
> apis 
    > animalBatchesApi 
    > animalApi
> slices
    > whateverSlice
> thunks
    > whateverThunk

This has the benefit of setting the baseQuery to match the endpoints, we can start using the new pattern where we don't have to pass in user_id to the headers for the other routes. And of course the nice pattern of making an redux api for each new table.

[kathyavini]

I think Anto said we will start with one file but almost certainly move to some sort of chunking. But looking at this closer, I'm not sure about one file-one endpoint (is that what the diagram is indicating?) I guess I would have thought of chunking all the animal endpoints together, although now I realize our API wasn't really set up like that with a common base URL 🤔

[Duncan-Brain]

Even all the animal endpoints together could get chunky -- imagine addOne, removeOne, addMany, fetchOne, fetchMany etc etc. for each of animal, animal batches, animal groups, animal identifiers, animal sexes, breeds, types etc. etc.

With one per endpoint it would mirror our routes/models/tables with probably one clearly defined get, post, and delete and typings for most.

[kathyavini]

True, true. And yes it does feel particularly appealing to mirror the routes file exactly...!

[antsgar]

I like the idea of having one file per resource! To be clear though we'd still need the base API slice, and the code splitting would only be at the file level, essentially the slice itself still needs to be just one slice. Check the "Tip" section here https://redux-toolkit.js.org/rtk-query/api/created-api/overview for a better explanation, and here https://redux-toolkit.js.org/rtk-query/usage/code-splitting is the example of how to use the injectEndpoints method to extend the API slice with additional files

[Duncan-Brain]

Just wondering if the redux store is the best location for types. As we start converting our codebase to .ts I get the feeling we will be using types in our containers or components or even backend too.

[kathyavini]

Discussed in tech daily but want to leave a ping to discuss with @antsgar too.

Yeah I think your idea of having an animal type that we use throughout the frontend (and then extend to get the apiSlice interface) makes sense -- and I agree, must avoid interface duplication 👍 👍 👍 Would love to discuss types organization generally as even within the RTK Query docs alone there were several patterns.

[Duncan-Brain]

It seems to be working well for me ! I did not see the multiple requests at first but navigating around added it. If you want to keep the demonstration you could make the dispatch into a button 😄 .

I just added a few architecture questions mostly carried over from the template PR.

My main approval question was about if breed name, and type name should be retrieved here or if there is another PR supposed to do those RTK queries. I think we decided it can go in the "implement table" PR.

[antsgar]

Looks and works great, so excited to see this one come to life!

[antsgar]

I like the idea of having one file per resource! To be clear though we'd still need the base API slice, and the code splitting would only be at the file level, essentially the slice itself still needs to be just one slice. Check the "Tip" section here https://redux-toolkit.js.org/rtk-query/api/created-api/overview for a better explanation, and here https://redux-toolkit.js.org/rtk-query/usage/code-splitting is the example of how to use the injectEndpoints method to extend the API slice with additional files

[antsgar]

At this point in time since there's no requirement to use TS I'm honestly alright with going as far as it seems to make sense within each situation for adding types. Enforcing doing full interfaces might increase scope and take a lot of time on some scenarios, whereas on others it might be relatively easy to go all the way. I'm on the "use your best judgement" camp for now, since TS use is still entirely optional even incomplete typing helps

[SayakaOno]

@kathyavini I was looking at dispatch(api.util.invalidateTags(['Animals', 'AnimalBatches', 'AnimalGroups'])), it doesn't seem to refetch when reloading...?
This is off topic, but if a query needs refetching every time on mount, passing refetchOnMountOrArgChange(https://redux-toolkit.js.org/rtk-query/api/createApi#refetchonmountorargchange) true to the hook may be what we need to do.

[kathyavini]

@SayakaOno wait are you saying that dispatching invalidateTags in the component creates an infinite loop OR does nothing that all then??! 😓

I like the sound of refetchOnMountOrArgChange!! I thought it was something we would need especially while testing, but it sounds like a good default always to refetch on page reload.

[SayakaOno]

@kathyavini sorry, navigating to the inventory causes an infinite loop, but when reloading, I don't think network requests are sent...

Is there a way to refetch on page reload?? I feel like we would not use cache at all with refetchOnMountOrArgChange 🤔