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

🏛 Update to React Query 4 #4060

Merged
merged 3 commits into from
Aug 30, 2022
Merged

🏛 Update to React Query 4 #4060

merged 3 commits into from
Aug 30, 2022

Conversation

jxom
Copy link
Contributor

@jxom jxom commented Aug 23, 2022

Fixes RNBW-4355

What changed (plus any additional context for devs)

This PR updates React Query to v4, following the migration guide, in preparation for future Async State related work in Code Foundations.

  • Updated all import references to @tanstack/react-query.
  • Ensured all query functions do not return undefined (and null instead).
    • This is in line with React Query 4 making undefined an illegal cache value.
  • Ensured all queries use an array as the cache key.
  • Removed the notifyOnChangeProps: 'tracked' global config as tracked behavior is now enabled by default.
  • Contentious: Created a patch to put the isIdle state back in. This was removed in React Query 4.
    • I believe more often than not, we will reference the idle state when the "fetch status" is "idle" and the "status" is "loading" (no data in cache). I feel like manually representing the idle state through fetchStatus === 'idle' && status === 'loading' could lead to issues as it's not completely obvious that this is the idle state/developers could forget to add one of the values & if it were mitigated and composed to a function, we would lose the benefit of tracked return values.
    • For the edge case we may want to represent the idle state of something like fetchStatus === 'idle', we can just directly encapsulate this into a variable in the component/hook itself (e.g. isNetworkIdle = fetchStatus === 'idle').
    • I decided to make this a patch as it will not be trivial to create our own useQuery hook WITH tracked return values. I think maintaining a small patch like this is a lot easier than creating our own hook with tracked return value capabilities.

Screen recordings / screenshots

No visual changes.

@linear
Copy link

linear bot commented Aug 23, 2022

RNBW-4355 Async State: Migrate to React Query 4

  • We currently use React Query in the app for some features such as Profiles, DPI, Wallet Balances, etc.
  • We should migrate to React Query v4 (we are currently on v3).

@@ -446,6 +446,7 @@
"test-runner": "jest"
},
"react-native": {
"@tanstack/react-query": "@tanstack/react-query/build/esm/index",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to reference the ESM build, and not the UMD build.

Copy link
Contributor

Choose a reason for hiding this comment

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

Waiting on this one? I'll subscribe to updates 🤓

Copy link
Member

@brunobar79 brunobar79 left a comment

Choose a reason for hiding this comment

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

🔥 🔥 🔥

Comment on lines +10 to +11
+ isLoading: status === 'loading' && fetchStatus === 'fetching',
+ isIdle: status === 'loading' && fetchStatus === 'idle',
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a couple silly questions here, I might be confused 😛

The change to isLoading makes sense to me, since status === loading only on initial load and a cold cache, right?

But "idle" to me means "not fetching atm". With status === loading here, won't this only be idle on initial load as well? Should it be status === 'success' && fetchStatus === 'idle'?

Seems like a weird choice by our boy Tanner, but like I said, might just not grok this bit yet 😅

Copy link
Contributor Author

@jxom jxom Aug 23, 2022

Choose a reason for hiding this comment

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

But "idle" to me means "not fetching atm". With status === loading here, won't this only be idle on initial load as well? Should it be status === 'success' && fetchStatus === 'idle'?

The terminology of status is a bit confusing, but it essentially means "cache status". A status of loading means we do not have any data in the cache. We will consider the state being idle when we do not have any data in the cache (status === 'loading'), and we are not fetching (fetchStatus === 'idle').

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok my mistake, I misunderstood 👍 dig the patch too!

Copy link
Contributor

@estrattonbailey estrattonbailey left a comment

Choose a reason for hiding this comment

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

So good 🚢

@jxom jxom added the dev QA PR that can be safely QA'd by devs for merge (feature-specific limited impact). QA during regression label Aug 23, 2022
Copy link
Contributor

@estebanmino estebanmino left a comment

Choose a reason for hiding this comment

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

went over profiles flows where we use react query heavily, looking nice
🚢

Copy link
Contributor

@skylarbarrera skylarbarrera left a comment

Choose a reason for hiding this comment

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

🏛️

@jxom jxom requested a review from markdalgleish as a code owner August 29, 2022 21:00
@jxom jxom removed the request for review from derHowie August 29, 2022 22:37
Copy link
Contributor

@markdalgleish markdalgleish left a comment

Choose a reason for hiding this comment

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

Approving the design system docs Prettier formatting diff 😄

@jxom jxom merged commit 95c6a9b into develop Aug 30, 2022
@jxom jxom deleted the @jxom/react-query-4 branch August 30, 2022 23:44
estrattonbailey added a commit that referenced this pull request Sep 6, 2022
…w-4378-clean-up-global-and-env-vars-in-app-move

* 'develop' of github.com:rainbow-me/rainbow: (40 commits)
  fix: quiet logs when linting in CI (#4070)
  analytics: add tracking for screen dimensions + fix nav events on android (#4088)
  fix native input cursor color (#4112)
  Profiles: use `name` text record instead of `me.rainbow.displayName` (#4046)
  chore: upgrade eslint-config-rainbow (#4118)
  backup PIN  (#4012)
  Remove long press on address in switching wallets (#3990)
  Version bump iOS v.1.7.4 & Android 144 (#4117)
  Fix slow image loading (#4052)
  Add new text size scale, deprecate old sizes and Heading (#4113)
  Disable campaings in e2e and fix testID of PromoSheet (#4107)
  🏛 chore: prettier everything (#4100)
  fix settings StatusIcon shadows on android (#4104)
  fix clear async storage crash (#4105)
  fix RNBW-4416 (#4103)
  🏛 Update to React Query 4 (#4060)
  swaps: init cross chain feature flag (#4091)
  enable arbiitrum swap details test (#4092)
  remove unsafe access to networkInfo (#4096)
  network check (#4094)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev QA PR that can be safely QA'd by devs for merge (feature-specific limited impact). QA during regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants