Add support for other Firebase products to integrate with Remote Config. #6692
Conversation
|
|
||
| /// Adds a listener that will be called whenever one of the get methods is called. | ||
| /// @param listener Function that takes in the parameter key and the config. | ||
| - (void)addListener:(void (^_Nonnull)(NSString *_Nonnull, NSDictionary *_Nonnull))listener; |
There was a problem hiding this comment.
should remove this from the header file since we're just calling it internally within the RC SDK and not changing the public API
| @@ -284,6 +306,14 @@ - (void)loadConfigFromMainTable { | |||
| self->_defaultConfig = [defaultConfig mutableCopy]; | |||
| dispatch_semaphore_signal(self->_configLoadFromDBSemaphore); | |||
| }]; | |||
|
|
|||
| [_DBManager loadPersonalizationWithCompletionHandler:^(BOOL success, NSDictionary *fetchedConfig, | |||
There was a problem hiding this comment.
rename to fetchedPersonalization and activePersonalization?
|
|
||
| if (handler) { | ||
| dispatch_async(dispatch_get_main_queue(), ^{ | ||
| handler(YES, activePersonalization, fetchedPersonalization, nil); |
There was a problem hiding this comment.
I think the handler definition expects fetched and then active
There was a problem hiding this comment.
LOL, too much copying and pasting.
| @@ -331,6 +358,10 @@ - (FIRRemoteConfigValue *)configValueForKey:(NSString *)key { | |||
| @"Key %@ should come from source:%zd instead coming from source: %zd.", key, | |||
| (long)FIRRemoteConfigSourceRemote, (long)value.source); | |||
| } | |||
| if ([self->_configContent.activePersonalization count] > 0) { | |||
| [self callListeners:value.stringValue | |||
| metadata:self->_configContent.activePersonalization[key]]; | |||
There was a problem hiding this comment.
thinking: we mentioned that the listener approach would allow us to add other side-effects in the future, but given the current implementation all the listeners would be called specifically with personalization metadata, which limits the functionality we can add
| @@ -41,6 +42,10 @@ | |||
| /// Timeout value for waiting on a fetch response. | |||
| static NSString *const kRemoteConfigFetchTimeoutKey = @"_rcn_fetch_timeout"; | |||
|
|
|||
| /// Listener for the get methods. | |||
| typedef void (^FIRRemoteConfigListener)(NSString *_Nonnull, NSDictionary *_Nonnull) | |||
| NS_SWIFT_NAME(FIRRemoteConfigListener); | |||
There was a problem hiding this comment.
Remove NS_SWIFT_NAME. APIs exposed to Swift should not use typedefs and this looks like its only for internal APIs anyway
| @@ -100,6 +103,10 @@ typedef void (^RCNDBLoadCompletion)(BOOL success, | |||
| - (void)updateMetadataWithOption:(RCNUpdateOption)option | |||
| values:(NSArray *)values | |||
| completionHandler:(RCNDBCompletion)handler; | |||
|
|
|||
| /// Insert or update the data in Personalizatoin config. | |||
FirebaseRemoteConfig/CHANGELOG.md
Outdated
| @@ -7,6 +7,7 @@ | |||
| - [fixed] Fixed database creation on tvOS. (#6612) | |||
| - [changed] Updated public API documentation to no longer reference removed APIs. (#6641) | |||
| - [fixed] Updated `activateWithCompletion:` to use completion handler for experiment updates. (#3687) | |||
| - [changed] Log metadata for Remote Config parameter get calls. (#6692) | |||
There was a problem hiding this comment.
Can you make this more descriptive? As written this might raise concerns for developers. What exactly is the impact of the change for users of the RemoteConfig libraries?
There was a problem hiding this comment.
I think the RC team wants to keep this intentionally vague, since most developers won't be able to use this.
There was a problem hiding this comment.
In that case, something like "Added preliminary infrastructure to support future features" or nothing at all might be better. If I read the code right, additional logging is only happening when Personalization is being used. Saying unspecified logging is happening can be concerning to developers.
Stores Personalization metadata in database along with Remote Config parameters. Adds a RCNPersonalization class that logs Personalization events, and listeners that call that when get*() is called.