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, getAll): init with empty config #5859

Merged
merged 2 commits into from
Nov 16, 2021

Conversation

mikehardy
Copy link
Collaborator

@mikehardy mikehardy commented Nov 16, 2021

Description

previously if you incorrectly called getAll before setting defaults or fetching any
remote config, we would crash.

Now we initialize values as an empty object, so we have a graceful failure

confirmed via code inspection that this matches expecations from firebase-js-sdk
https://github.com/firebase/firebase-js-sdk/blob/acc58102d4429ce0593ec22192e76018e9d16ed7/packages/remote-config/test/remote_config.test.ts#L333-L335

Related issues

Fixes #5854, Fixes #5855

Release Summary

rebase merge the conventional commits

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

Test Plan

I refactored the remote config tests to put the non-native ones in a jest config and verified those worked
I added a new jest test that probed this, and failed as expected
I repaired this in a way that seems compatible with firebase-js-sdk and the test passed


Think react-native-firebase is great? Please consider supporting the project with any of the below:

previously if you incorrectly called getAll before setting defaults or fetching any
remote config, we would crash.

Now we initialize values as an empty object, so we have a graceful failure

confirmed via code inspection that this matches expecations from firebase-js-sdk
https://github.com/firebase/firebase-js-sdk/blob/acc58102d4429ce0593ec22192e76018e9d16ed7/packages/remote-config/test/remote_config.test.ts#L333-L335

Fixes #5854
@vercel
Copy link

vercel bot commented Nov 16, 2021

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

react-native-firebase – ./

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/4VX26s6cha423coSgXnPwa61HG9z
✅ Preview: https://react-native-firebase-git-mikehardy-5854-confi-cc05be-invertase.vercel.app

react-native-firebase-next – ./website_modular

🔍 Inspect: https://vercel.com/invertase/react-native-firebase-next/9xPsJeFehwGe45PX1NzvWnuNCVkV
✅ Preview: Canceled

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Nov 16, 2021
@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #5859 (ac3911c) into main (d0d9bd8) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head ac3911c differs from pull request most recent head 5ab2462. Consider uploading reports for the commit 5ab2462 to get more accurate results

@@             Coverage Diff              @@
##               main    #5859      +/-   ##
============================================
+ Coverage     52.98%   52.99%   +0.02%     
- Complexity      620      626       +6     
============================================
  Files           208      208              
  Lines         10118    10148      +30     
  Branches       1589     1606      +17     
============================================
+ Hits           5360     5377      +17     
- Misses         4503     4519      +16     
+ Partials        255      252       -3     

@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Nov 16, 2021
@mikehardy mikehardy merged commit 232d860 into main Nov 16, 2021
@mikehardy mikehardy deleted the @mikehardy/5854-config-get-all branch November 16, 2021 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant