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.

Sign up for GitHub

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

Jump to bottom

Use fallback value when source == .static #11095

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Use fallback value when source == .static #11095

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions FirebaseRemoteConfigSwift/Sources/PropertyWrapper/RemoteConfigValueObservable.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,13 @@ internal class RemoteConfigValueObservable<T: Decodable>: ObservableObject {
self.key = key
remoteConfig = RemoteConfig.remoteConfig()
self.fallbackValue = fallbackValue

// Initialize with fallback value
configValue = fallbackValue

// Check cached remote config value
do {
let configValue: RemoteConfigValue = remoteConfig[key]
if configValue.source == .remote || configValue.source == .default {
self.configValue = try remoteConfig[key].decoded()
} else {
self.configValue = fallbackValue
}
} catch {
configValue = fallbackValue
}
applyConfigValue()

NotificationCenter.default.addObserver(
self, selector: #selector(configDidActivate), name: .onRemoteConfigActivated, object: nil
)
Expand All @@ -61,13 +55,19 @@ internal class RemoteConfigValueObservable<T: Decodable>: ObservableObject {
if FirebaseApp.app()?.name != appName {
return
}
applyConfigValue()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

private func applyConfigValue() {
do {
let configValue: RemoteConfigValue = remoteConfig[key]
if configValue.source == .remote {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if configValue.source == .remote || configValue.source == .default {
self.configValue = try remoteConfig[key].decoded()
} else {
self.configValue = fallbackValue
Copy link
Member

@ncooke3 ncooke3 Apr 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
} catch {
// Suppresses a hard failure if decoding failed.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

configValue = fallbackValue
}
}
}