-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[Feature Proposal]Property Wrapper for RemoteConfig #9918
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
[Feature Proposal]Property Wrapper for RemoteConfig #9918
Conversation
7336abb
to
9170a4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feature proposal implementation! We'll have to get this through internal API review, but at a glance this looks good to me.
FirebaseRemoteConfigSwift/Sources/PropertyWrapper/PropertyWrapper.swift
Outdated
Show resolved
Hide resolved
Thanks @fumito-ito! Please resolve the CI issues. |
0ac250c
to
92df894
Compare
CI back to green 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working through the CI issues!
FirebaseRemoteConfigSwift/Sources/PropertyWrapper/PropertyWrapper.swift
Outdated
Show resolved
Hide resolved
5da3fe7
to
aadf50a
Compare
var recipeValue: Recipe | ||
} | ||
|
||
// Contrast this test with the subsequent one to see the value of the Codable API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the "subsequent" test to show the contrast or do you want to compare to the tests in Value.swift?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, This is the code comment I copied and forgot to delete. I will erase it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(done)
- fix format and license - fix availability - fix pod settings - add some documents to public interface
a491ac1
to
5808cdd
Compare
Thanks! We need to do an internal API review process so it may take us a few weeks to a month to merge, but I expect it to happen. |
@fumito-ito Thank you for this initiative! I'm looking into the API review right now. Do you have a sample test app or some sample code that I can see how to use this new API to reflect config changes on SwiftUI? |
Hi! @chliangGoogle thank you for your feedback. The simplest sample is like #9828. @RemoteConfig(forKey: "is_new_feature_available")
var isNewFeatureAvailable: Bool
Toggle(isOn: isNewFeatureAvailable) {
Text("wonderful new feature")
} If you need more sample app, I can upload it into the repository but I cannot find where to put sample apps for remote config. Could you tell me place for sample app ? |
@fumito-ito The sample code is exactly what I need. No need to make a sample app for this. Thanks! |
of object: Any?, | ||
change: [NSKeyValueChangeKey: Any]?, | ||
context: UnsafeMutableRawPointer?) { | ||
guard change != nil, object != nil, keyPath == observingKeyPath else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fumito-ito I believe this should be triggered when developer calls fetchAndActivate API because last fetch time changed (for a fresh install), is that correct? However, it's not called for me when I test it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chliangGoogle thanks for your review. I found observing lastFetcheTime
is not working.
There are following options for this PR. How do you feel about these options ?
- convert to simple property wrapper, not dynamic property
- It's not fit for my needs (Remote Config working with SwiftUI)
- propagate last fetch time changes from
RCNUserDefaultsManager
- It may be possible, but I need time for this change (I'm not familiar to Objective-C)
Hey @fumito-ito I'm working on Google Open Source Peer Bonus nominations and would like to recognize your contribution to make Firebase Remote Config more SwiftUI friendly. I couldn't find an email address for you, so if you're interested, please email it to chliang at google.com and I'll make the nomination. |
self.observation?.dispose() | ||
} | ||
|
||
class RemoteConfigObsever: NSObject { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small typo.
Going to close since this PR was incorporated into #10155. Thanks @fumito-ito for getting this going! |
Why
#9828
What
This PR provides a Property Wrapper with DynamicProperty for RemoteConfig. With this feature, users can bind Remote Config values to variables, and at the same time, reflect them in the View of the SwiftUI in real time.
Known Issues
lastFetchTime
is monitored to detect that the RemoteConfig value has changed. This is not very accurate. For example, if thedefaultValue
is changed, the change will not be detected.Decodable
properties are covered. Therefore, users cannot bind the PropertyWrapper to variables of type Any, such as those provided byjsonValue
.