Migrate FetchAndActivate from deprecated implementation #5617
Conversation
| // Pass along the fetch error e.g. throttled. | ||
| completionHandler(status, error); | ||
| if (fetchStatus == FIRRemoteConfigFetchStatusSuccess && !fetchError) { | ||
| [strongSelf activateWithCompletionHandler:^(NSError *_Nullable activateError) { |
| FIRRemoteConfigFetchAndActivateStatus status = | ||
| activateError ? FIRRemoteConfigFetchAndActivateStatusSuccessUsingPreFetchedData | ||
| : FIRRemoteConfigFetchAndActivateStatusSuccessFetchedFromRemote; | ||
| completionHandler(status, fetchError); |
There was a problem hiding this comment.
Should we use activateError instead of fetchError here?
There was a problem hiding this comment.
Maybe, but fetchError matches the previous behavior and changing it would be a breaking change.
There was a problem hiding this comment.
If I read the code correctly, fetchError is nil here, so can we just pass nil?
There was a problem hiding this comment.
Yep - good catch. Thanks!
| @@ -236,27 +236,28 @@ - (void)fetchAndActivateWithCompletionHandler: | |||
| (FIRRemoteConfigFetchAndActivateCompletion)completionHandler { | |||
| __weak FIRRemoteConfig *weakSelf = self; | |||
| FIRRemoteConfigFetchCompletion fetchCompletion = | |||
| ^(FIRRemoteConfigFetchStatus fetchStatus, NSError *error) { | |||
| ^(FIRRemoteConfigFetchStatus fetchStatus, NSError *fetchError) { | |||
There was a problem hiding this comment.
This method is a great candidate to be refactored with Promises. We can consider it for future PRs.
There was a problem hiding this comment.
Agreed! This and several other RC methods.
| }]; | ||
| } else if (completionHandler) { | ||
| FIRRemoteConfigFetchAndActivateStatus status = | ||
| fetchStatus == FIRRemoteConfigFetchStatusSuccess |
There was a problem hiding this comment.
is it possible that the fetchStatus is successful and fetchError also exists?
There was a problem hiding this comment.
Yes - if the config didn't change. See #3586. I'm planning to deprecate that API and add a new API that returns a bool like Android and JS instead of an error - after I get some integration test infra set up.
| // Pass along the fetch error e.g. throttled. | ||
| completionHandler(status, error); | ||
| if (fetchStatus == FIRRemoteConfigFetchStatusSuccess && !fetchError) { | ||
| [strongSelf activateWithCompletionHandler:^(NSError *_Nullable activateError) { |
There was a problem hiding this comment.
nice! it's odd that it was using sync method even tho this async method exists.
Migrate
fetchAndActivateWithCompletionHandler:implementation from using the deprecatedactivateFetchedAPI to the asyncactivateWithCompletionHandler:Working on integration tests, I was getting a deadlock in
activateFetchedso thought it would be good to move this forward.