Use fallback value when source == .static #11095

peterfriese · 2023-04-10T18:12:33Z

The @RemoteConfigProperty property wrapper should use the fallback value when the user selects “Use in-app default” in the Firebase console.

The @RemoteConfigProperty property wrapper should use the `fallback` value when the user selects “Use in-app default” in the Firebase console. Signed-off-by: Peter Friese <[email protected]>

google-oss-bot · 2023-04-10T18:14:37Z

	1 Warning
⚠️	Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

ncooke3

LGTM. Also, reminder to add a CHANGELOG entry if applicable. I'll defer approval to @charlotteliang!

ncooke3 · 2023-04-10T18:28:28Z

FirebaseRemoteConfigSwift/Sources/PropertyWrapper/RemoteConfigValueObservable.swift

 self.configValue = try remoteConfig[key].decoded()
+ } else {
+ self.configValue = fallbackValue


Is this the codepath for when source == .static?

This is for source == .static, yes. Would you rather I make this more explicit?

Partially it was just me being unfamiliar with the API. I think more explicit would be a little clearer (maybe a switch statement?) and would guard against any unintended behavior if we were to add a case to that enum (probably unlikely though). Up to you, really.

I think it might be a good idea to make it more explicit (and thus, easier to understand).

google-oss-bot · 2023-04-10T18:43:36Z

Coverage Report ¹

Affected Products

`FirebaseRemoteConfig-iOS-FirebaseRemoteConfig.framework`

Overall coverage changed from ? (7452958) to 70.94% (4644a8b) by ?.

13 individual files with coverage change

Filename	Base (`7452958`)	Merge (`4644a8b`)	Diff
FIRConfigValue.m	?	58.70%	?
FIRRemoteConfig.m	?	83.91%	?
FIRRemoteConfigComponent.m	?	97.06%	?
FIRRemoteConfigUpdate.m	?	100.00%	?
RCNConfigContent.m	?	81.90%	?
RCNConfigDBManager.m	?	76.34%	?
RCNConfigExperiment.m	?	90.70%	?
RCNConfigFetch.m	?	71.09%	?
RCNConfigRealtime.m	?	35.85%	?
RCNConfigSettings.m	?	62.81%	?
RCNDevice.m	?	81.29%	?
RCNPersonalization.m	?	89.74%	?
RCNUserDefaultsManager.m	?	98.43%	?

Test Logs

Merge (4644a8b): https://github.com/firebase/firebase-ios-sdk/actions/runs/4659991670

https://storage.googleapis.com/firebase-sdk-metric-reports/V5aUq08EPy.html

charlotteliang

I don't think fallback value is equivalent to "in app default" value.

The fallback value is a pure swift language default value that is not the "in app default" value that's the default config value.

charlotteliang · 2023-04-10T20:27:13Z

FirebaseRemoteConfigSwift/Sources/PropertyWrapper/RemoteConfigValueObservable.swift

 }
 } catch {
- // Suppresses a hard failure if decoding failed.


I don't think this is right, we should only get remote value since this method is only triggered when config has remote value needs to be activated.

charlotteliang · 2023-04-10T20:28:51Z

FirebaseRemoteConfigSwift/Sources/PropertyWrapper/RemoteConfigValueObservable.swift

@@ -61,13 +55,19 @@ internal class RemoteConfigValueObservable<T: Decodable>: ObservableObject {
 if FirebaseApp.app()?.name != appName {
 return
 }
+ applyConfigValue()


from the way you refactor the logic, I'm not sure it aligns with the description because for the applyConfigValue() fundamentally didn't change any logic.

peterfriese · 2023-04-10T21:18:34Z

I don't think fallback value is equivalent to "in app default" value.

If the fallbackValue is not intended to act as the in-app default value, there should be a(nother) mechanism for specifying the default value.

The way it currently works is:

In the console, you select Use in-app default value
Nothing happens

And this - at least to me - doesn't sound right either. Unless I am missing something.

The fallback value is a pure swift language default value that is not the "in app default" value that's the default config value.

Not sure I follow. To specify a language-level default value, wouldn't you rather want to assign a value to the underlying variable at initialisation time? I.e., @RemoteConfigProperty(key="foo") var foo = "Some default value"?

charlotteliang · 2023-04-10T21:23:34Z

The way it currently works is:

In the console, you select Use in-app default value
Nothing happens

Can you elaborate the "nothing happens" part?
I think the expected case here should be "use default config value if it's being set by developers". So if developer didn't set a config value, nothing should happen.

The fallback value is used when a config key is never set by developers and because sometimes SwiftUI requires developers to do a null check, so it's easier to pass in a fallback value to avoid that. Here's the API definition: https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseRemoteConfigSwift/Sources/PropertyWrapper/RemoteConfigProperty.swift#L36

peterfriese · 2023-04-10T21:48:19Z

The way it currently works is:

In the console, you select Use in-app default value Nothing happens

Can you elaborate the "nothing happens" part? I think the expected case here should be "use default config value if it's being set by developers". So if developer didn't set a config value, nothing should happen.

Sure. Take a look at the following code, which uses the RC property wrapper to connect the cardColor to an RC variable.

@RemoteConfigProperty(key: "cardColor", fallback: "#ff2d55") var cardColor

var body: some View {
    VStack {
      Text("What's your favourite number?")
        .font(.title)
        .multilineTextAlignment(.center)
      Spacer()
      Stepper(value: $viewModel.favouriteNumber, in: minValue...maxValue) {
        Text("\(viewModel.favouriteNumber)")
      }
    }
    .foregroundColor(.white)
    .background(Color(hex: cardColor))
}

When defining the RC variable and setting it to #0000FF in the console, the UI will turn blue - as expected. When toggling the switch in the console to Use in-app default, the UI will stay red (i.e. nothing happens), instead of turning to "Sizzling red" (#ff2d55), as specified in the first line of the snippet. See https://github.com/FirebaseExtended/firebase-video-samples/blob/peterfriese/apple/remote-config/fundamentals/apple/remote-config/final/Favourites/Shared/FavouriteNumberView.swift#L69 for the complete code sample.

Expected behaviour: toggling to Use in-app default value uses the value defined by the PW.

charlotteliang · 2023-04-10T23:22:26Z

Expected behaviour: toggling to Use in-app default value uses the value defined by the PW.

No that's not how we designed the API, the "in-app default" value should be defined by developers, not the fall back in PW as described in the API definition. https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseRemoteConfigSwift/Sources/PropertyWrapper/RemoteConfigProperty.swift#L36

@karenyz

karenyz · 2023-04-11T16:29:33Z

No that's not how we designed the API, the "in-app default" value should be defined by developers, not the fall back in PW as described in the API definition. https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseRemoteConfigSwift/Sources/PropertyWrapper/RemoteConfigProperty.swift#L36

This is also how I understood it. In RC the 3 value sources work as follows:

remote: a value was defined remotely (server-side) for this parameter
default: a value was set client-side for this parameter using the setDefaults method in the SDK. Toggling "use in-app default" in the Console indicates that a parameter should use this client-side value, the server "opts out" of providing a value.
static: this essentially means "parameter not found", i think the initial intention was so RC wouldn't return null values, but in reality this often causes confusion. This could occur if "use in-app default" is toggled but no default was ever set in the client, or simply this parameter key doesn't exist.

Going back to this PR, the PW should use both remote and default values as-is, and only use the fallback if there happens to be a decoding issue with the value, or there was no (user-defined) RC value found (static).

Hope this clarifies things, if not, happy to discuss more!

karenyz

this change makes sense to me, it follows the remote > default > fallback ordering we discussed today

sorry for the confusion!

karenyz · 2023-04-20T23:43:20Z

FirebaseRemoteConfigSwift/Sources/PropertyWrapper/RemoteConfigValueObservable.swift

 do {
 let configValue: RemoteConfigValue = remoteConfig[key]
- if configValue.source == .remote {


ahh i see, so previously the PW value would only change if there was a remote value, and does nothing otherwise

paulb777 · 2023-11-14T01:00:32Z

Please close, merge, or comment. We plan to close stale PRs on November 28, 2023.

Use fallback value when source == .static

671b63c

The @RemoteConfigProperty property wrapper should use the `fallback` value when the user selects “Use in-app default” in the Firebase console. Signed-off-by: Peter Friese <[email protected]>

peterfriese added the api: remoteconfig label Apr 10, 2023

peterfriese requested review from charlotteliang, ncooke3 and danasilver April 10, 2023 18:12

ncooke3 reviewed Apr 10, 2023

View reviewed changes

paulb777 requested a review from karenyz April 10, 2023 18:31

charlotteliang requested changes Apr 10, 2023

View reviewed changes

karenyz approved these changes Apr 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use fallback value when source == .static #11095

Use fallback value when source == .static #11095

peterfriese commented Apr 10, 2023

google-oss-bot commented Apr 10, 2023

ncooke3 left a comment

ncooke3 Apr 10, 2023 •

edited

peterfriese Apr 10, 2023

ncooke3 Apr 10, 2023

peterfriese Apr 10, 2023

google-oss-bot commented Apr 10, 2023

`FirebaseRemoteConfig-iOS-FirebaseRemoteConfig.framework`

charlotteliang left a comment

charlotteliang Apr 10, 2023

charlotteliang Apr 10, 2023

peterfriese commented Apr 10, 2023

charlotteliang commented Apr 10, 2023

peterfriese commented Apr 10, 2023

charlotteliang commented Apr 10, 2023

karenyz commented Apr 11, 2023

karenyz left a comment

karenyz Apr 20, 2023

paulb777 commented Nov 14, 2023

Use fallback value when source == .static #11095

Are you sure you want to change the base?

Use fallback value when source == .static #11095

Conversation

peterfriese commented Apr 10, 2023

google-oss-bot commented Apr 10, 2023

ncooke3 left a comment

Choose a reason for hiding this comment

ncooke3 Apr 10, 2023 • edited

Choose a reason for hiding this comment

peterfriese Apr 10, 2023

Choose a reason for hiding this comment

ncooke3 Apr 10, 2023

Choose a reason for hiding this comment

peterfriese Apr 10, 2023

Choose a reason for hiding this comment

google-oss-bot commented Apr 10, 2023

Coverage Report 1

Affected Products

FirebaseRemoteConfig-iOS-FirebaseRemoteConfig.framework

Test Logs

charlotteliang left a comment

Choose a reason for hiding this comment

charlotteliang Apr 10, 2023

Choose a reason for hiding this comment

charlotteliang Apr 10, 2023

Choose a reason for hiding this comment

peterfriese commented Apr 10, 2023

charlotteliang commented Apr 10, 2023

peterfriese commented Apr 10, 2023

charlotteliang commented Apr 10, 2023

karenyz commented Apr 11, 2023

karenyz left a comment

Choose a reason for hiding this comment

karenyz Apr 20, 2023

Choose a reason for hiding this comment

paulb777 commented Nov 14, 2023

ncooke3 Apr 10, 2023 •

edited

Coverage Report ¹

`FirebaseRemoteConfig-iOS-FirebaseRemoteConfig.framework`