Lf 4671 migrate weather board saga to rtk query #3699

gursimran-singh · 2025-02-26T17:54:57Z

Description

This PR migrates the functionality to RTK query by extending the main API and removes the dependency on saga and slice.

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

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?

Login with the account and on the home page correct weather is shown.

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

Duncan-Brain

Thanks so much @gursimran-singh I think it looks great to me!

If you are interested in continuing on WeatherBoard we would love to obscure the api call a little more and move it to the backend https://lite-farm.atlassian.net/browse/LF-4281

kathyavini

This is so cool Gursimran!!

Thank you for delving into so much uncharted territory and getting us started with the separate files for new endpoints (actually I think we should have started doing this earlier 😅), migrating off of slice + saga (🎉!!) and using RTK Query to interface with an external API. It's great! ☺️

Everything functions well, and my only comments are about how to keep the weatherApi.ts a little bit more parallel to the existing RTK Query endpoints in terms of types and structure. I think a consistent structure will help us a lot with keeping things readable at a glance as you pick up more and more endpoints.

Awesome job -- so exciting to see the great migration underway 😁

kathyavini · 2025-02-28T19:27:51Z

packages/webapp/src/containers/WeatherBoard/weatherApi.ts

@@ -0,0 +1,67 @@
+/*


I'm pretty sure we'd like to keep all the endpoint definitions within store/api/ instead of keeping them near their calling component. But I'll double-check with @antsgar when she returns!

Currently, as per my understanding, we are using feature-based structure where container/feature has all the logic/functionality contained in them, hence creating separate of concerns. Having all the endpoint within store/api folder reflects service-based approach. Are we planning to have mix of both or migrating to service-based structure? let me know and i will change accordingly.

I personally like the separation of concerns Gursimran uses here... but maybe it is something we should have a quick chat about. I thought we just hadn't got around to it yet haha

It's honestly already a mix of things right now. Some things are more feature based where you have the container file, a hook and a saga in the same place. But then there's a specific hooks folder and a specific stories folder. Even the things currently organized by feature aren't currently very easy to find because there's tons of containers and they're not grouped by modules or areas of the app. I personally feel it's a lot more intuitive and harder to go wrong with a structure where we have specific folders for components, hooks, slices, rather than the feature based approach (although I'm not sure I understand why that would be a service-based approach?). So my vote is for keeping these in a store/api folder in an attempt to move towards that direction

Sorry I misread as wanting to keep the endpoints in the same file store/api/index not the same folder. I like store/api folder better too. Just appreciating moving endpoints into a specified file aha

From a broader perspective, I understand that in feature based approach, both presentation and business logic reside within the same folder/feature, resembling a form of vertical scaling. On contrast, a service based approach follows horizontal scaling, where the business logic is separated from the presentation/ui logic and placed in the folders categorized by services, such as APIs. I am not advocating one approach over the other, as both of the design patterns have their pros and cons, and more importantly is depends upon where the team interest is leveraging towards.

I know mix of these exists and was just confirming as perviously all the business logic(saga, slice, state management) was contained in the same folder and now we are moving these to separate one. It’s good to have these discussion/talks early on, as they help establish a clear direction for the rest of the migration process. Now I got the clarity and will implement this.

kathyavini · 2025-02-28T19:38:34Z

packages/webapp/src/containers/WeatherBoard/weatherApi.ts

+export const weatherApi = api.injectEndpoints({
+  endpoints: (build) => ({
+    getWeather: build.query({
+      queryFn: async (arg) => {


I know it's only called only in a JS file, but since all our API endpoints will be written in TS, it would be great to keep type safety. In this case, the return type can be inferred, but the parameters cannot so at least those could be typed explicitly:

interface WeatherQueryArgs { lat: number; lon: number; measurement: string; }

BUT to keep consistency with all our other RTK query endpoints and for the clearest documentation, it would actually be ideal if we could keep this format and type both return and params:

getWeather: build.query<WeatherData, WeatherQueryArgs>({

Sure, i will implement this.

kathyavini · 2025-02-28T19:56:30Z

packages/webapp/src/containers/WeatherBoard/weatherApi.ts

+        }
+
+        try {
+          const response = await axios.get(openWeatherUrl.toString());


RTK Query manages its own fetching so it would be cleaner and more like the other endpoints to use query instead of queryFn and you wouldn't need axios or the error handling (try/catch) as that is all built-in (unless there is a particular reason it's necessary to use axios for this endpoint)

query: ({ lat, lon, measurement }) => { const apikey = import.meta.env.VITE_WEATHER_API_KEY; const params = new URLSearchParams({ lat: String(lat), // ... }); return `https://api.openweathermap.org/data/2.5/weather?${params.toString()}`; },

We haven't used it yet in app, but I think the best RTK Query pattern for the returned data would then be transformResponse:

transformResponse: (response: any /* optional to type their API response */) => { const weatherPayload = { humidity: `${response.main?.humidity}%`, // ... }; return weatherPayload;

The only reason was query generates CORS error and { axios } from '../saga' had already figured out. Nevertheless, as we are moving API call to open weather to the backend, this whole thing would be changed. https://lite-farm.atlassian.net/browse/LF-4281

@gursimran-singh you are getting a CORS error (from openweathermap?) by using query on this endpoint? That's really weird... I tried it with the code above and it worked fine.

True about the backend migration for this endpoint. I was thinking more about the pattern going forward with the RTK migration in general since this is the first of many.

I agree that we should as far as possible use RTK Query's built in functionality for fetching rather than Axios. I'd imagine that long term once we have migrated all of the frontend to RTK Query we'd be able to get rid of Axios as a dependency altogether

This is the CORS error i get after using the following code:

getWeather: build.query({ query: () => 'https://api.openweathermap.org/data/2.5/weather?units=metric&lang=en&lat=52.08089409999999&lon=-106.6206944&appid=332adfb0a45a2d6ac05f65e3106a3890&cnt=1', providesTags: ['Weather'], }),

As we won't be calling the API directly from the frontend and moving the logic to backend, will see if this error occurs there and get back you if needed.

@gursimran-singh isn't the API key param missing in that request?

@antsgar Actually appid param uses the api key in the requestappid=332adfb0a45a2d6ac05f65e3106a3890. This is how it has been formulated.

antsgar

@gursimran-singh thank you so much for working on this, and for proactively reaching out to get involved in the migration! Made a few comments but this is great work 🙌

antsgar · 2025-03-05T00:56:40Z

packages/webapp/src/containers/WeatherBoard/index.jsx

+    if (currentTime - data?.lastUpdated > two_hours) {
+      refetch();
+    }


I haven't tried this before so not sure, but it seems it's possible to control the frequency the query is refetched through RTK Query by setting the keepUnusedDataFor parameter -- which can not only be set api-wide but also for a specific endpoint https://redux-toolkit.js.org/rtk-query/api/createApi#keepunuseddatafor-1

I will check this out!!

antsgar · 2025-03-05T01:04:41Z

packages/webapp/src/containers/WeatherBoard/weatherApi.ts

@@ -0,0 +1,67 @@
+/*


It's honestly already a mix of things right now. Some things are more feature based where you have the container file, a hook and a saga in the same place. But then there's a specific hooks folder and a specific stories folder. Even the things currently organized by feature aren't currently very easy to find because there's tons of containers and they're not grouped by modules or areas of the app. I personally feel it's a lot more intuitive and harder to go wrong with a structure where we have specific folders for components, hooks, slices, rather than the feature based approach (although I'm not sure I understand why that would be a service-based approach?). So my vote is for keeping these in a store/api folder in an attempt to move towards that direction

antsgar · 2025-03-05T01:11:44Z

packages/webapp/src/containers/WeatherBoard/weatherApi.ts

+        }
+
+        try {
+          const response = await axios.get(openWeatherUrl.toString());


I agree that we should as far as possible use RTK Query's built in functionality for fetching rather than Axios. I'd imagine that long term once we have migrated all of the frontend to RTK Query we'd be able to get rid of Axios as a dependency altogether

…ther

…RTK-Query

gursimran-singh added 2 commits February 20, 2025 11:05

LF-4671 Converted weather saga to RTK query

9ea5b21

LF-4671 Cleanup of slice and saga in favour of weather RTk query

40477a4

gursimran-singh requested review from a team as code owners February 26, 2025 17:54

gursimran-singh requested review from Duncan-Brain and kathyavini and removed request for a team February 26, 2025 17:54

Duncan-Brain previously approved these changes Feb 28, 2025

View reviewed changes

kathyavini reviewed Feb 28, 2025

View reviewed changes

kathyavini requested a review from antsgar March 4, 2025 17:42

antsgar reviewed Mar 5, 2025

View reviewed changes

gursimran-singh added 2 commits March 17, 2025 01:03

LF-4671 moved weatherApi to /store/api folder to grpup all API's toge…

2a06ed1

…ther

Merge branch 'integration' into LF-4671-Migrate-WeatherBoard-saga-to-…

dc476c3

…RTK-Query

gursimran-singh dismissed Duncan-Brain’s stale review via dc476c3 March 17, 2025 07:07

antsgar approved these changes Mar 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lf 4671 migrate weather board saga to rtk query #3699

Lf 4671 migrate weather board saga to rtk query #3699

gursimran-singh commented Feb 26, 2025

Duncan-Brain left a comment

kathyavini left a comment •

edited

Loading

kathyavini Feb 28, 2025 •

edited

Loading

gursimran-singh Mar 4, 2025

Duncan-Brain Mar 4, 2025

antsgar Mar 5, 2025

Duncan-Brain Mar 5, 2025 •

edited

Loading

gursimran-singh Mar 10, 2025

kathyavini Feb 28, 2025

gursimran-singh Mar 4, 2025

kathyavini Feb 28, 2025 •

edited

Loading

gursimran-singh Mar 4, 2025

kathyavini Mar 4, 2025

antsgar Mar 5, 2025

gursimran-singh Mar 10, 2025

antsgar Mar 13, 2025

gursimran-singh Mar 14, 2025 •

edited

Loading

antsgar left a comment

antsgar Mar 5, 2025

gursimran-singh Mar 10, 2025

antsgar Mar 5, 2025

antsgar Mar 5, 2025

Lf 4671 migrate weather board saga to rtk query #3699

Are you sure you want to change the base?

Lf 4671 migrate weather board saga to rtk query #3699

Conversation

gursimran-singh commented Feb 26, 2025

Duncan-Brain left a comment

Choose a reason for hiding this comment

kathyavini left a comment • edited Loading

Choose a reason for hiding this comment

kathyavini Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Duncan-Brain Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kathyavini Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gursimran-singh Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

antsgar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kathyavini left a comment •

edited

Loading

kathyavini Feb 28, 2025 •

edited

Loading

Duncan-Brain Mar 5, 2025 •

edited

Loading

kathyavini Feb 28, 2025 •

edited

Loading

gursimran-singh Mar 14, 2025 •

edited

Loading