-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[Rollouts] Set active rollout metadata after activating config #12316
Conversation
@@ -329,6 +329,8 @@ - (void)activateWithCompletion:(FIRRemoteConfigActivateChangeCompletion)completi | |||
// New config has been activated at this point | |||
FIRLogDebug(kFIRLoggerRemoteConfig, @"I-RCN000069", @"Config activated."); | |||
[strongSelf->_configContent activatePersonalization]; | |||
[strongSelf->_settings updateLastActiveTemplateVersion]; |
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.
Thinking: We don't need to wait for this to complete before calling activateRolloutMetadata
since UserDefaults is updated synchronously in the process (and persisted to disk async) so rollouts will get the latest value in the process https://developer.apple.com/documentation/foundation/userdefaults#overview
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.
Make sense to me. Dispatch the activeTemplateVersion updating.
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.
Ah, I was saying I think that the previous code (synchronously) is fine, since UserDefaults is updated in the process synchronously. Can revisit this when we use the value in a subsequent PR, though
Set active rollout metadata after activate config.
Add last active template version to userDefault.
Looks like the previous PR is getting bigger, split this part into a PR for better review. Notifying interop logic will be in a following PR.
Note: this change will merge to our feature branch not master
#no-changelog