[Rollouts] Notify RolloutsState change to Interop subscriber #12334
Conversation
ddnan
force-pushed
the
ddn-rollouts-notify
branch
from
6084dbd
to
d595008
Compare
(cherry picked from commit af3306f0518da23b0302a4917007a762bd3f43bf)
(cherry picked from commit 91d2b401f8415772ca89615b262c310faedf3160)
…ate version update sync
ddnan
force-pushed
the
ddn-rollouts-notify
branch
from
2e3d9a5
to
998854e
Compare
|
Add some screenshots from app manual testing |
ddnan
changed the title
| // Send active rollout metadata stored in persistence while app launched if there is activeConfig | ||
| NSString *FQNamespace = [self fullyQualifiedNamespace:_FIRNamespace]; | ||
| NSDictionary<NSString *, NSDictionary *> *activeConfig = self->_configContent.activeConfig; | ||
| if (activeConfig[FQNamespace].count > 0) { |
There was a problem hiding this comment.
Is it possible activeConfig[FQNamespace] == nil? Would we throw an exception getting count in that case?
There was a problem hiding this comment.
When the db is loading, we initialized all the content with namespace. It should be at least an empty dictionary. But I still add a nil check before check count.
| if (rolloutId && variantID && affectedParameterKeys) { | ||
| for (NSString *key in affectedParameterKeys) { | ||
| FIRRemoteConfigValue *value = self->_configContent.activeConfig[FQNamespace][key]; | ||
| if (value) { |
There was a problem hiding this comment.
Does this allow an empty string value? (we want to allow that since it's a valid value from the backend)
Other note here is that we want to fall back to the in-app default value or the static default if there isn't a value present in the active config (i.e. following the SDK's get(param) logic)
There was a problem hiding this comment.
Yes, empty string will return true when do if check on it and nil will return false.
Add in-app default value logic if value is nil.
There was a problem hiding this comment.
Maybe I'm missing it - can you point me to where the in-app default fallback logic is?
| @@ -45,6 +45,8 @@ | |||
| /// Notification when config is successfully activated | |||
| const NSNotificationName FIRRemoteConfigActivateNotification = | |||
| @"FIRRemoteConfigActivateNotification"; | |||
| static NSNotificationName RolloutsStateDidChangeNotificationName = | |||
| @"RolloutsStateDidChangeNotification"; | |||
There was a problem hiding this comment.
curious - should this be the same format as the other notification name? (prefixed with FIR)
There was a problem hiding this comment.
Ah yeah, since it's global(used by notification center) let's perfix with FIR. Also this is in objc (objc does not have namespace so the best practise is uppercase prefixes for every object).
| if (rolloutId && variantID && affectedParameterKeys) { | ||
| for (NSString *key in affectedParameterKeys) { | ||
| FIRRemoteConfigValue *value = self->_configContent.activeConfig[FQNamespace][key]; | ||
| if (value) { |
There was a problem hiding this comment.
Maybe I'm missing it - can you point me to where the in-app default fallback logic is?
| for (NSString *key in affectedParameterKeys) { | ||
| FIRRemoteConfigValue *value = self->_configContent.activeConfig[FQNamespace][key]; | ||
| if (!value) { | ||
| value = [self defaultValueForFullyQualifiedNamespace:FQNamespace key:key]; |
There was a problem hiding this comment.
nice, i like how you factored this out 👍
Notify RolloutsState change to Interop subscriber
Note: this change will merge to our feature branch not master
#no-changelog