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

fix(remote-config,ios): fixed fetch throttling issue #4664

Merged
merged 1 commit into from
Dec 9, 2020

Conversation

Danchoys
Copy link
Contributor

@Danchoys Danchoys commented Dec 9, 2020

Description

As per documentation, apps must be very gentle when it comes to fetching from the Firebase Remote Config storage. By default, it happens not more often than once in 12 hours. For that the library provides a selector: fetchWithCompletionHandler: that doesn't have the time interval parameter and uses the value from settings and the second one fetchWithExpirationDuration:completionHandler: that allows to override this configuration. In JS it is exposed as a single method of signature fetch(expirationDurationSeconds?: number): Promise<void>;. When expirationDurationSeconds is undefined, -1 is passed to the native module and then treated as follows:

  if (expirationDuration == @(-1)) {
    [[FIRRemoteConfig remoteConfigWithApp:firebaseApp] fetchWithCompletionHandler:completionHandler];
  } else {
    [[FIRRemoteConfig remoteConfigWithApp:firebaseApp] fetchWithExpirationDuration:expirationDuration.doubleValue completionHandler:completionHandler];
  }

The problem here is that an NSNumber instance is compared by reference with another NSNumber instance (yet holding the same value) which always fails, leading to the else path always executed. And internally in the FirebaseRemoteConfig SDK -1 is going to be treated same as 0 in such a way that no limitation on fetching will be applied, resulting in almost immediate throttling unleashed by the server.

This PR mitigates this issue and allows to fetch the remote config without the risk of throttling.

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

🔥

@vercel
Copy link

vercel bot commented Dec 9, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/kql2vnhtc
✅ Preview: https://react-native-firebase-git-master.invertase.vercel.app

@codecov
Copy link

codecov bot commented Dec 9, 2020

Codecov Report

Merging #4664 (1e32c82) into master (a96a401) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4664      +/-   ##
==========================================
+ Coverage   88.93%   88.99%   +0.06%     
==========================================
  Files         109      109              
  Lines        3712     3712              
  Branches      347      347              
==========================================
+ Hits         3301     3303       +2     
+ Misses        369      367       -2     
  Partials       42       42              

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Nice catch! I think you win my available bounty of internet points for the day 🥇 - I would never have seen this.

I'm hoping to get a release done today, but the patch-package Github check will have a patch set available as an attached artifact in the meantime

@mikehardy mikehardy merged commit 5a68a8a into invertase:master Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants