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

feat: centralize react-query queries #6049

Merged
merged 15 commits into from
Jan 20, 2024

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Jan 19, 2024

Description

  • Keeps things DRY 🩸🛁 and encapsulated in a single-source-of-truth for tidiness and safety
  • Ensures we get the query keys right and don't have to remember them - noticed a few instances of infinitely-cached queries that were wrong and fixed in this diff e.g this guy in develop which should be pools (plural) and could cause bugs if we were to do another midgardPoolData call somewhere else when expecting a single pool as a response:
    const pools = useQuery({
    queryKey: ['midgardPoolData'],
    // We may or may not want to revisit this, but this will prevent overfetching for now
    staleTime: Infinity,
    queryFn: async () => {
    const { data: poolData } = await axios.get<MidgardPoolResponse[]>(
    `${getConfig().REACT_APP_MIDGARD_URL}/pools`,
    )
  • Removes util pure function wrappers over queryClient.fetchQuery, since queryClient.fetchQuery() calls can now be done in one line 🎉
  • Gives sane staleTime defaults to queries - Infinity for most, but sometimes a different one when applicable (keeping the same ones as currently), so we don't end up in cases where we void the staleTime guarantees by having different consumers use different staleTimes

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

N/A

Risk

High Risk PRs Require 2 approvals

Theoretically low to none, effectively medium given the domain diffed. Common THORChain (THORNode/Midgard) fetching queries and THORChain LP queries have been moved to the new source-of-truth. The same queries configuration has been kept so this is most likely very low risk, however, the default refetchOnMount setting of react-query has been switched to false for performance reason, which may produce regressions in lending.

Testing

  • Complete a lending (either deposit or withdraw) flow as a paranoia check and ensure status detection is still happy

Engineering

  • ☝🏽
  • THORChain LP is still happy
  • Queries aren't refetched on mount anymore. That's a global setting so will apply to any react-query query, not only LP. One easy way to test this against develop or the PR under this one is by going from "Pools" to "Your Positions" and back, which previously triggered a full refetch, whereas this PR now keeps the subscribers alive, which means no refetch 🎉

Operations

  • ☝🏽

Screenshots (if applicable)

Lending paranoia test of status detection

lending.status.detection.mov

Re-mounted subscribers don't refetch anymore

(this diff DevTools on top, develop on the bottom)

perf.mov

Copy link
Contributor Author

gomesalexandre commented Jan 19, 2024

@gomesalexandre gomesalexandre mentioned this pull request Jan 19, 2024
3 tasks
@gomesalexandre gomesalexandre changed the title wip: centralize react-query queries feat: centralize react-query queries Jan 19, 2024
@gomesalexandre gomesalexandre marked this pull request as ready for review January 19, 2024 14:45
@gomesalexandre gomesalexandre requested a review from a team as a code owner January 19, 2024 14:45
Copy link
Contributor

@kaladinlight kaladinlight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beautiful! and here I was going to comment on feat_blazingly_fast_lp that we should look at drying up all of those useQuery functions 😂

Base automatically changed from feat_blazingly_fast_lp to develop January 20, 2024 13:24
@gomesalexandre gomesalexandre enabled auto-merge (squash) January 20, 2024 13:25
@gomesalexandre gomesalexandre merged commit 1097d93 into develop Jan 20, 2024
4 checks passed
@gomesalexandre gomesalexandre deleted the feat_centralize_react_queries branch January 20, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants