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.

Sign up for GitHub

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

Jump to bottom

remote config overhaul #5297

Merged
merged 24 commits into from
Dec 21, 2023
Merged

remote config overhaul #5297

merged 24 commits into from
Dec 21, 2023

Conversation

benisgold
Copy link
Member

@benisgold benisgold commented Dec 20, 2023 •
edited

Fixes APP-1040, APP-1041

What changed (plus any additional context for devs)

The general problem:
Remote config values are accessed from the config object in the app. The problem with this is that if the config object updates, it will not trigger rerenders in the components that statically access its attributes. This can cause race condition issues, since we do not await fetching remote config on app startup.

How the points feature flag problem occurred:

  1. swipe navigator that includes tab bar is rendered
  2. remote config is fetched
  3. since config is statically accessed, swipe navigator never rerenders with updated remote config value

Solution:
This PR migrates the remote config implementation over to a react query hook. This allows us to provide remote config with a hook, which causes dependent components to immediately rerender if remote config is updated. Now what happens:

  1. swipe navigator that includes tab bar is rendered
  2. remote config is fetched
  3. since swipe navigator uses remote config query, it is rerendered using the appropriate remote config values

FOLLOW-UP:

  • look into moving away from react query and using built-in firebase functionality

Screen recordings / screenshots

https://www.loom.com/share/734dcab645144757814dc8b2e2334739

What to test

TF 1.9.12 (28)
points feature should be hidden. try restarting the app many times to refresh remote config

@walmat
Copy link
Contributor

walmat commented Dec 20, 2023

Should we move all consumption of the remote config to use the hook?

@linear Linear
Copy link

linear bot commented Dec 20, 2023

APP-1040 Change remote config fetching to be done on an custom interval structure

APP-1041 Write useRemoteConfig hook to consume remote config data

@benisgold benisgold marked this pull request as ready for review December 20, 2023 20:48
@benisgold benisgold changed the title hook remote config hook Dec 20, 2023
@benisgold benisgold added the release for release blockers and release candidate branches label Dec 20, 2023
@benisgold benisgold removed the release for release blockers and release candidate branches label Dec 20, 2023
@benisgold benisgold marked this pull request as draft December 20, 2023 23:39
@benisgold benisgold marked this pull request as ready for review December 21, 2023 16:35
@benisgold benisgold added the release for release blockers and release candidate branches label Dec 21, 2023
walmat
walmat reviewed Dec 21, 2023
View reviewed changes
Copy link
Contributor

@walmat walmat left a comment

Choose a reason for hiding this comment

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

testing the changes now

src/navigation/SwipeNavigator.js Outdated Show resolved Hide resolved
src/navigation/SwipeNavigator.js Outdated Show resolved Hide resolved
benisgold
benisgold commented Dec 21, 2023
View reviewed changes
index.js
@@ -25,6 +25,4 @@ to start before all of the imports.
*/
require('react-native-gesture-handler');
require('./shim');
require('./src/model/config');
Copy link
Member Author

Choose a reason for hiding this comment

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

is this ok @walmat

ibrahimtaveras00
ibrahimtaveras00 approved these changes Dec 21, 2023
View reviewed changes
Copy link
Contributor

@ibrahimtaveras00 ibrahimtaveras00 left a comment

Choose a reason for hiding this comment

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

Tested turning remote config on and off for all respective parameters and works as expected; spot checked Android as well to make sure everything still shows!

QA Passed 👍🏽

@benisgold benisgold changed the title remote config hook remote config overhaul Dec 21, 2023
@jinchung jinchung merged commit 295b959 into develop Dec 21, 2023
5 of 6 checks passed
@jinchung jinchung deleted the @benisgold/fix-remote-config branch December 21, 2023 21:17
ibrahimtaveras00 pushed a commit that referenced this pull request Dec 21, 2023
@benisgold @walmat @ibrahimtaveras00
remote config overhaul (#5297)
9fa6692 
* hook

* refetching

* finish query hook

* migrate points to new remote config query

* error handling

* fetch on app init

* cleanup

* onError

* rm testing error

* change IS_DEV to exp feature flag hook

* debugging

* tf

* rm testing stuff and fix tabs

* staleTime/cacheTime

* add IS_TEST to points

* rm unnecessary error + added finally

* rm config

* syntax

* full migration

* remove index stuff

* rm react query cache + fix firebase cache time

---------

Co-authored-by: Matthew Wall <matthew.wallt@gmail.com>
@sentry-io Sentry.io
Copy link

sentry-io bot commented Dec 22, 2023

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Error: Failed to fetch remote config _createSuperInternal(src/logger/index) View Issue

Did you find this useful? React with a 👍 or 👎

BrodyHughes added a commit that referenced this pull request Dec 27, 2023
@BrodyHughes
Merge branch 'develop' of https://github.com/rainbow-me/rainbow into …
e499f04 
…brody/QR-code-scanner-fixes

* 'develop' of https://github.com/rainbow-me/rainbow:
  bump version to 1.9.13 (#5303)
  3s race (#5302)
  remote config overhaul (#5297)
  fix lint (#5296)
  remove the need for switching to hide splash screen (#5298)
BrodyHughes added a commit that referenced this pull request Jan 2, 2024
@BrodyHughes
Merge branch 'develop' of https://github.com/rainbow-me/rainbow into …
379a8bc 
…brody/QR-code-scanner-fixes

* 'develop' of https://github.com/rainbow-me/rainbow:
  chore: i18n updates (#5292)
  bump version to 1.9.13 (#5303)
  3s race (#5302)
  remote config overhaul (#5297)
  fix lint (#5296)
  remove the need for switching to hide splash screen (#5298)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibrahimtaveras00 ibrahimtaveras00 ibrahimtaveras00 approved these changes

@skylarbarrera skylarbarrera Awaiting requested review from skylarbarrera

@jinchung jinchung Awaiting requested review from jinchung jinchung is a code owner

@walmat walmat Awaiting requested review from walmat walmat is a code owner

@nickbytes nickbytes Awaiting requested review from nickbytes nickbytes is a code owner

Assignees
No one assigned
Labels
release for release blockers and release candidate branches
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@benisgold @walmat @ibrahimtaveras00 @jinchung