firebase-ios-sdk icon indicating copy to clipboard operation
firebase-ios-sdk copied to clipboard

Use fallback value when source == .static

Open peterfriese opened this issue 1 year ago • 8 comments

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

peterfriese avatar Apr 10 '23 18:04 peterfriese

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

Generated by :no_entry_sign: Danger

google-oss-bot avatar Apr 10 '23 18:04 google-oss-bot

Coverage Report 1

Affected Products

  • FirebaseRemoteConfig-iOS-FirebaseRemoteConfig.framework

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

    13 individual files with coverage change

    FilenameBase (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

google-oss-bot avatar Apr 10 '23 18:04 google-oss-bot

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:

  1. In the console, you select Use in-app default value
  2. 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"?

peterfriese avatar Apr 10 '23 21:04 peterfriese

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

charlotteliang avatar Apr 10 '23 21:04 charlotteliang

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.

peterfriese avatar Apr 10 '23 21:04 peterfriese

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

charlotteliang avatar Apr 10 '23 23:04 charlotteliang

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 avatar Apr 11 '23 16:04 karenyz

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

paulb777 avatar Nov 14 '23 01:11 paulb777