Skip to content
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

Use of React Query for api calls #2074

Open
bharateshwq opened this issue Oct 4, 2024 · 10 comments · May be fixed by #2087
Open

Use of React Query for api calls #2074

bharateshwq opened this issue Oct 4, 2024 · 10 comments · May be fixed by #2087

Comments

@bharateshwq
Copy link
Contributor

bharateshwq commented Oct 4, 2024

I think we should consider using React Query for our data fetching instead of useEffect.
Here's their docs https://tanstack.com/query/v3
Brief about it:

  • Caching: React Query automatically caches data, so we don’t have to worry about making the same requests over and over.

  • Optimistic Updates: It lets us update the UI instantly while waiting for the data, making things feel smoother.

  • Background Updates: Data can refresh in the background, keeping everything up to date without extra work.

  • Garbage Collection: It automatically cleans up unused data, which helps with performance.

-Easier State Management: It handles loading and error states for us, so we can write less code.

@anth-volk
Copy link
Collaborator

Curious to hear how this contrasts with React 18+ use.

In my (pedantic) opinion, one of the biggest drawbacks of our front end is that we employ an event-driven model instead of an action-driven one, such that we have a seemingly endless number of effect hooks. If we could diminish them (as well as prop stuffing) by using this, I'd be all in favor.

@bharateshwq
Copy link
Contributor Author

bharateshwq commented Oct 5, 2024

Hey @anth-volk
While the use hook in React offers promise handling and integrates well with Suspense, it doesn't quite compare to the more mature React Query, which many in the community view as the essential data-fetching library for React.

From what I understand, you're referring to how one useEffect can trigger others, complicating state management. React Query addresses these challenges effectively, providing numerous options that enhance both performance and usability.
Addressing the prop stuffing issue react query provides a centralized data fetching mechanism, making the components themselves very clean and manage only UI rendering part.

Additionally React docs itself strongly suggests not using useEffect at all or using it as the last resort to any problem

We could begin with a module or a set of pages that you think are significantly impacted by the event-driven model.

@anth-volk
Copy link
Collaborator

@bharateshwq Thanks for that rundown. I'd be completely onboard, and agreed, useEffect is used far too much within this repo.

Much of the data fetching actually happens immediately on first load, and must be accessible from anywhere within the application, after which point routing is applied. I believe this initial fetch occurs within the PolicyEngine.jsx component, and the fact that it's required before even being able to route does affect load times. Would you want to work up something replacing the fetch for country metadata?

Also, how does the usage of this library compare with the use of useEffect + Redux for state management with fetched data?

@bharateshwq
Copy link
Contributor Author

Yeah i will send a pr on it,
Redux shines when it comes to state management on the client side and query is great at syncing server data with the client and React query offers far superior built in cache management with less code.

@bharateshwq
Copy link
Contributor Author

bharateshwq commented Oct 7, 2024

Hey @anth-volk ,
While reviewing the code, I noticed a parameter that is set to trigger a refetch. Is this the only purpose of that parameter, or does it serve any other function as well? If you remember what it is, it would really help save me some time. Thanks!
image

@anth-volk
Copy link
Collaborator

Precisely, this parameter triggers the creation of a new policy and returns a new policy ID for whenever a policy is renamed. This is because the app aims to display every created policy and no policy is "assigned" to any user, and thereby cannot allow an individual policy to be editable.

@bharateshwq bharateshwq linked a pull request Oct 8, 2024 that will close this issue
@bharateshwq
Copy link
Contributor Author

Hey @anth-volk ,
I have merged it with latest please check

@anth-volk
Copy link
Collaborator

@bharateshwq Thanks for letting me know, I will do. Just briefly glancing at this, it does look like a pretty significant improvement structurally. Off the top of my head, just a few questions/comments for you:

  1. Due to the US election, we've held off on merging some PRs until after a winner is declared. This would fall into that category, since we already have all of this capability, just written more poorly and redundantly.
  2. In your view, how realistic workflow-wise is it to break up the implementation of this? E.g., add all the metadata-related parts first, then policy, etc. If it doesn't seem realistic, that's okay, just curious on your view.
  3. Typically, we don't actually re-fetch policies, only when they're first created and/or certain very particular updates. I'm not sure if your code does or doesn't do that, just wanted to let you know.

@bharateshwq
Copy link
Contributor Author

I completely understand the delays with the PRs. Regarding your second point, I've opened an issue about the workflow. I recognize that implementing changes will present challenges, but I genuinely believe it will be beneficial for maintaining the code in the long run. Issues with useEffect can arise unexpectedly, so addressing this proactively is important.

Here's the workflow I propose: [link to issue]. Of course, I'm open to your thoughts on whether a different approach might be better.

As for your third point, everything is currently functioning as intended with how the deployed application interacts with the backend.

@anth-volk
Copy link
Collaborator

Thanks for this @bharateshwq. I totally understand, and I agree, useEffect is very much something we need to avoid moving forward, where possible, so I am eager to migrate to react-query via your code, I just tend to be hesitant, given our lack of front-end testing.

I'll be sure to check out the workflow when I review. I will try to get around to this today, but am unsure if I'll be able to. It may have to be either tomorrow or Wednesday, depending on workload.

And on the third point, glad to hear. I hadn't really checked deeply into the implementation, just wanted to highlight it, since I noticed the changes you had proposed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

2 participants