feat(remote-config): Expose modular API that matches the Firebase web JS SDK v9 API #6868

russellwheatley · 2023-01-26T16:38:55Z

Description

Feedback welcome on the following:

setLogLevel() API on web has no counterpart on native. At the moment, it just returns debug. See here.
Web JS SDK has a setter for defaultConfig. We have a native counterpart that is async called setDefaults(). What should we do about this?
Exact same situation for settings. Native counterpart is setConfigSettings() which is async. What to do about this?

Related issues

Release Summary

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

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

👉 Star this repo on GitHub ⭐️
👉 Follow React Native Firebase and Invertase on Twitter

vercel · 2023-01-26T16:39:00Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
react-native-firebase	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 6, 2023 at 0:18AM (UTC)

1 Ignored Deployment

Name	Status	Preview	Comments	Updated
react-native-firebase-next	⬜️ Ignored (Inspect)			Feb 6, 2023 at 0:18AM (UTC)

codecov · 2023-01-27T11:24:12Z

Codecov Report

Merging #6868 (85ff236) into main (4e170ea) will decrease coverage by 0.07%.
The diff coverage is 46.52%.

@@             Coverage Diff              @@
##               main    #6868      +/-   ##
============================================
- Coverage     54.46%   54.38%   -0.07%     
- Complexity      718      720       +2     
============================================
  Files           225      226       +1     
  Lines         11102    11177      +75     
  Branches       1749     1764      +15     
============================================
+ Hits           6046     6078      +32     
- Misses         4724     4768      +44     
+ Partials        332      331       -1

mikehardy · 2023-01-27T12:03:30Z

interesting - on the 3 questions listed currently in description -

1- to be firebase-js-sdk compatible we need to define the LogLevel type as well https://firebase.google.com/docs/reference/js/remote-config.md#loglevel and support the setLogLevel API, I think 'error' is probably the better choice for default return value as it will imply on native that you are not going to get any special non-error logging, and have a note that just says "setter is ignored on native"

2- we have to have a setter to be compatible, so it should do delegate to the existing implementation and just not await on it I guess. javascript property setters can delegate to other methods right? It's just the other direction (non-property implementations delegating to property setters) that doesn't work, as you learned+mentioned the other day?

3- same here we have to have the setter to be compatible. I just read an issue logged against the TC39 committee plus some stackoverflow and there is not and probably will never by async setters which - to me - just argues for never using javascript properties but firebase-js-sdk forces us to.

So both 2 and 3 - implement the setter so we are compatible, perhaps with a note that it is async on native platforms, and people should await on the native API or await on a short sleep if avoiding a race condition is critical 🤷

russellwheatley · 2023-01-30T10:11:05Z

interesting - on the 3 questions listed currently in description -

1- to be firebase-js-sdk compatible we need to define the LogLevel type as well firebase.google.com/docs/reference/js/remote-config.md#loglevel and support the setLogLevel API, I think 'error' is probably the better choice for default return value as it will imply on native that you are not going to get any special non-error logging, and have a note that just says "setter is ignored on native"

2- we have to have a setter to be compatible, so it should do delegate to the existing implementation and just not await on it I guess. javascript property setters can delegate to other methods right? It's just the other direction (non-property implementations delegating to property setters) that doesn't work, as you learned+mentioned the other day?

3- same here we have to have the setter to be compatible. I just read an issue logged against the TC39 committee plus some stackoverflow and there is not and probably will never by async setters which - to me - just argues for never using javascript properties but firebase-js-sdk forces us to.

So both 2 and 3 - implement the setter so we are compatible, perhaps with a note that it is async on native platforms, and people should await on the native API or await on a short sleep if avoiding a race condition is critical 🤷

@mikehardy I've updated the PR to delegate the setters to the existing async API. The difference between perf and remote-config is; it didn't matter that we didn't have to await dataCollectionEnabled setter on perf because it would only take effect on the next app run, and we updated a simple boolean on the perf instance before getting to native.

The two setters for remote-config update the settings and default config on the remote-config instance after sending them to native. I guess we should update them before they get to native? 🤔

mikehardy · 2023-01-30T12:33:34Z

The two setters for remote-config update the settings and default config on the remote-config instance after sending them to native. I guess we should update them before they get to native? 🤔

Updating the local instance before going to native seems best so that at least after the property setting returns it has an expected value if re-tested, even though native will be potentially still working async. This is a platform inconsistency we can't really bridge but I think a note in the docs "due to the nature of the react-native javascript/native bridge the actual native effect is asynchronous and there may be a slight delay at the native level before this takes effect. If that's important to your app, and you want to use the firebase-js-sdk compatible property setters, insert a small sleep after calling the setters" or similar ?

russellwheatley · 2023-01-30T14:41:54Z

We have a bit of an issue with web. The current, native setConfigSettings() is designed for iOS and android which has different property names for settings and is also set in seconds 😓.

See here.
It also gets the returned values from settings with the native property names and converts them here.

I've drafted this commit to get your thoughts, @mikehardy?

Will update tests and make note about non-async stuff for the setter but thought I'd see what you think of this first?

mikehardy · 2023-02-03T17:07:45Z

@russellwheatley I think the native interfaces do not matter much, the only really important thing is that the JS (and Typescript) API is firebase-js-sdk compatible, and it appears we took some care to do that - firebase-js-sdk API is in millis and our API is in millis so we don't have a breaking change on tap here. No problem if our internal JS code is translating between millis and seconds (which it is doing now), we can just keep right on doing that.

And same with the property names - as long as we present a firebase-js-sdk compatible facade at the JS layer, native can be whatever it needs to be

Stated more practically: I think this may actually be close then - allowing for the "dangit, react-native is async..." stuff as a slight difference, which I think you've handled reasonably given constraints that javascript properties can't be async but react-native has to be over the bridge

russellwheatley · 2023-02-06T12:22:28Z

@mikehardy, I changed the API for a couple of reasons. I wanted to make them cross platform and follow DRY principles.

The way to now think about settings & setConfigSettings is the former is sync (values updated on instance before making the trip to their platform) and the latter is async (values updated on native and brought back to update instance). I've implemented them so they're cross platform. (i.e. I've checked what platform is running and updated the values accordingly). They can now be used on all 3 platforms.

This is the same for defaultConfig & setDefaults.

_updateFromConstants has been updated to accept values from all platforms to this end. See here. You might notice, I've now wrapped constants.lastFetchTime and constants.lastFetchStatus. This is because we have to call this function here & here as we need to update values on the instance before it gets to native for the sync setters. Those properties won't be available until the round trip from the platform is complete.

I have been a bit sneaky with the error messages so they correctly output the correct API call. I do this by using call method to pass another argument that isn't part of the API. Retrieve it here for correct error message.

I'm sure you know, but simple demonstration of how that works:

function func1(a) {
  console.log(arguments[0]);
  // Expected output: 1

  console.log(arguments[1]);
  // Expected output: 2

}

func1.call(this, 1, 2)

I hope you see this as an improvement? Can change back if it's a little too complicated, and you wanted to keep each API separate and dedicated to their respective platforms 😅

mikehardy

A bit fancy but not too fancy, and how else to bridge the "property setters can't be async" + "apple/android expect seconds not millis" well? I think this does a good job

mikehardy · 2023-02-07T14:19:23Z

packages/remote-config/lib/index.d.ts

+ /**
+ * The status of the latest Remote RemoteConfig fetch action.
+ */
+ type LastFetchStatusType = 'success' | 'failure' | 'no_fetch_yet' | 'throttled';


nice - a real type is an improvement

mikehardy · 2023-02-07T14:19:47Z

packages/remote-config/lib/index.d.ts


 /**
 * Provides an object which provides the properties `minimumFetchIntervalMillis` & `fetchTimeMillis` if they have been set
 * using setConfigSettings({ fetchTimeMillis: number, minimumFetchIntervalMillis: number }). A description of the properties
 * can be found above
 *
 */
- settings: { fetchTimeMillis: number; minimumFetchIntervalMillis: number };
+ settings: ConfigSettings;


😆 why wasn't it like this before !? nice improvement

mikehardy · 2023-02-07T14:22:01Z

packages/remote-config/lib/index.js

+ throw new Error("firebase.remoteConfig().defaultConfig: 'defaults' must be an object.");
+ }
+ // To make Firebase web v9 API compatible, we update the config first so it immediately
+ // updates defaults on the instance. We then pass to web platform to update. We do this because


This is just a quibble on wording, but it's a mentality - if we work with apple/android/web we are not passing to web just because it's the modular API we present, we're wrapping any of the SDKs, so mental model is "we handle our business in the wrapping layer then hand off to SDK...", not just "web"

Suggested change

// updates defaults on the instance. We then pass to web platform to update. We do this because

// updates defaults on the instance. We then pass to underlying SDK to update. We do this because

I didn't perturb the PR with these FWIW but still worth thinking about w.r.t. mental model :-)

mikehardy · 2023-02-07T14:22:22Z

packages/remote-config/lib/index.js

+
+ set settings(settings) {
+ // To make Firebase web v9 API compatible, we update the settings first so it immediately
+ // updates settings on the instance. We then pass to web platform to update. We do this because


Suggested change

// updates settings on the instance. We then pass to web platform to update. We do this because

// updates settings on the instance. We then pass to underlying SDK to update. We do this because

russellwheatley added 7 commits January 26, 2023 14:57

update types

255854f

init modular file

38a7afb

initial e2e setup

b5bf5da

Merge branch 'main' into feat/modular-remote-config

478d6b4

export modular

f6adb44

update e2e tests

65fabc8

get defaultConfig

4717704

fix broken e2e tests

54736ec

vercel bot deployed to Preview – react-native-firebase January 27, 2023 09:00 View deployment

format

c17eade

vercel bot deployed to Preview – react-native-firebase January 27, 2023 09:06 View deployment

fix setLogLevel

a3d1d4e

vercel bot deployed to Preview – react-native-firebase January 27, 2023 09:50 View deployment

add isSupported e2e test

aadaee5

vercel bot deployed to Preview – react-native-firebase January 27, 2023 09:56 View deployment

russellwheatley added 2 commits January 27, 2023 09:58

space

3750488

add test for defaultConfig

0eb1828

vercel bot deployed to Preview – react-native-firebase January 27, 2023 10:10 View deployment

fix test

555629d

vercel bot deployed to Preview – react-native-firebase January 27, 2023 10:47 View deployment

russellwheatley marked this pull request as ready for review January 27, 2023 11:15

return error. note to ignore on native

58784b8

vercel bot deployed to Preview – react-native-firebase January 30, 2023 09:32 View deployment

delegate setters to existing API

37621c4

vercel bot deployed to Preview – react-native-firebase January 30, 2023 10:10 View deployment

update e2e tests

1f47540

vercel bot deployed to Preview – react-native-firebase January 30, 2023 11:56 View deployment

russellwheatley added 2 commits January 30, 2023 12:00

make getter adjacent to setter

0d7a826

Merge branch 'main' into feat/modular-remote-config

6ffe368

vercel bot deployed to Preview – react-native-firebase January 30, 2023 12:10 View deployment

update defaultConfig and settings for web

a057034

vercel bot deployed to Preview – react-native-firebase January 30, 2023 14:39 View deployment

russellwheatley added 2 commits February 6, 2023 12:05

updated defaultConfig and settings to be sync & DRY

f719568

Merge branch 'main' into feat/modular-remote-config

85ff236

vercel bot deployed to Preview – react-native-firebase February 6, 2023 12:18 View deployment

mikehardy approved these changes Feb 7, 2023

View reviewed changes

mikehardy merged commit e1504aa into main Feb 7, 2023

mikehardy deleted the feat/modular-remote-config branch February 7, 2023 14:26

mikehardy mentioned this pull request Feb 7, 2023

☂️ Bring Modular V9 to React Native Firebase #6705

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(remote-config): Expose modular API that matches the Firebase web JS SDK v9 API #6868

feat(remote-config): Expose modular API that matches the Firebase web JS SDK v9 API #6868

russellwheatley commented Jan 26, 2023 •

edited

Loading

vercel bot commented Jan 26, 2023 •

edited

Loading

codecov bot commented Jan 27, 2023 •

edited

Loading

mikehardy commented Jan 27, 2023

russellwheatley commented Jan 30, 2023

mikehardy commented Jan 30, 2023

russellwheatley commented Jan 30, 2023

mikehardy commented Feb 3, 2023

russellwheatley commented Feb 6, 2023 •

edited

Loading

mikehardy left a comment

mikehardy Feb 7, 2023

mikehardy Feb 7, 2023

mikehardy Feb 7, 2023

mikehardy Feb 7, 2023

mikehardy Feb 7, 2023

	// updates defaults on the instance. We then pass to web platform to update. We do this because
	// updates defaults on the instance. We then pass to underlying SDK to update. We do this because

	// updates settings on the instance. We then pass to web platform to update. We do this because
	// updates settings on the instance. We then pass to underlying SDK to update. We do this because

feat(remote-config): Expose modular API that matches the Firebase web JS SDK v9 API #6868

feat(remote-config): Expose modular API that matches the Firebase web JS SDK v9 API #6868

Conversation

russellwheatley commented Jan 26, 2023 • edited Loading

Description

Feedback welcome on the following:

Related issues

Release Summary

Checklist

Test Plan

vercel bot commented Jan 26, 2023 • edited Loading

codecov bot commented Jan 27, 2023 • edited Loading

Codecov Report

mikehardy commented Jan 27, 2023

russellwheatley commented Jan 30, 2023

mikehardy commented Jan 30, 2023

russellwheatley commented Jan 30, 2023

mikehardy commented Feb 3, 2023

russellwheatley commented Feb 6, 2023 • edited Loading

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy Feb 7, 2023

Choose a reason for hiding this comment

mikehardy Feb 7, 2023

Choose a reason for hiding this comment

mikehardy Feb 7, 2023

Choose a reason for hiding this comment

mikehardy Feb 7, 2023

Choose a reason for hiding this comment

mikehardy Feb 7, 2023

Choose a reason for hiding this comment

russellwheatley commented Jan 26, 2023 •

edited

Loading

vercel bot commented Jan 26, 2023 •

edited

Loading

codecov bot commented Jan 27, 2023 •

edited

Loading

russellwheatley commented Feb 6, 2023 •

edited

Loading