Skip to content

Migrate FetchAndActivate from deprecated implementation #5617

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 15, 2020
Merged

Migrate FetchAndActivate from deprecated implementation #5617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac7e93a
Migrate FetchAndActivate from deprecated implementation
paulb777 May 15, 2020
641d18b
style
paulb777 May 15, 2020
a332411
explicit nil and CHANGELOG.md
paulb777 May 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions FirebaseRemoteConfig/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
# Unreleased
- [changed] Updated `fetchAndActivateWithCompletionHandler:` implementation to activate asynchronously. (#5617)

# v4.4.11
- [fixed] Fixed a bug where settings updates weren't applied before fetches. (#4740)
- [changed] Updated public API documentation for 4.4.10 change from FirebaseInstanceID to
FirebaseInstallations. (#5561)

# v4.4.10
- [changed] Internal code changes - migrate to using the FIS SDK. (#5096)
- [changed] Include both CFBundleString and CFBundleShortVersionString in the outgoing fetch requests.

# v4.4.9
- [changed] Internal code changes. (#4934)

# v4.4.8
- [fixed] Fixed a bug (#4677, #4734) where Remote Config does not work after a restore of a previous backup of the device. (#4896).

# v4.4.7
- [fixed] Fixed a crash that could occur when attempting a remote config fetch before a valid Instance ID was available. (#4622)
- [fixed] Fixed an issue where config fetch would sometimes fail with a duplicate fetch error when no other fetches were in progress. (#3802)
Expand Down
31 changes: 16 additions & 15 deletions FirebaseRemoteConfig/Sources/FIRRemoteConfig.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,27 +236,28 @@ - (void)fetchAndActivateWithCompletionHandler:
(FIRRemoteConfigFetchAndActivateCompletion)completionHandler {
__weak FIRRemoteConfig *weakSelf = self;
FIRRemoteConfigFetchCompletion fetchCompletion =
^(FIRRemoteConfigFetchStatus fetchStatus, NSError *error) {
^(FIRRemoteConfigFetchStatus fetchStatus, NSError *fetchError) {
Copy link
Contributor

@maksymmalyhin maksymmalyhin May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is a great candidate to be refactored with Promises. We can consider it for future PRs.

Copy link
Member Author

@paulb777 paulb777 May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! This and several other RC methods.

FIRRemoteConfig *strongSelf = weakSelf;
if (!strongSelf) {
return;
}
// Fetch completed. We are being called on the main queue.
// If fetch is successful, try to activate the fetched config
bool didActivate = false;
if (fetchStatus == FIRRemoteConfigFetchStatusSuccess && !error) {
didActivate = [strongSelf activateFetched];
}
if (completionHandler) {
FIRRemoteConfigFetchAndActivateStatus status = FIRRemoteConfigFetchAndActivateStatusError;
if (fetchStatus == FIRRemoteConfigFetchStatusSuccess) {
status = didActivate ? FIRRemoteConfigFetchAndActivateStatusSuccessFetchedFromRemote
: FIRRemoteConfigFetchAndActivateStatusSuccessUsingPreFetchedData;
} else {
status = FIRRemoteConfigFetchAndActivateStatusError;
}
// Pass along the fetch error e.g. throttled.
completionHandler(status, error);
if (fetchStatus == FIRRemoteConfigFetchStatusSuccess && !fetchError) {
[strongSelf activateWithCompletionHandler:^(NSError *_Nullable activateError) {
Copy link
Contributor

@charlotteliang charlotteliang May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! it's odd that it was using sync method even tho this async method exists.

Copy link
Contributor

@maksymmalyhin maksymmalyhin May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

if (completionHandler) {
FIRRemoteConfigFetchAndActivateStatus status =
activateError ? FIRRemoteConfigFetchAndActivateStatusSuccessUsingPreFetchedData
: FIRRemoteConfigFetchAndActivateStatusSuccessFetchedFromRemote;
completionHandler(status, nil);
}
}];
} else if (completionHandler) {
FIRRemoteConfigFetchAndActivateStatus status =
fetchStatus == FIRRemoteConfigFetchStatusSuccess
Copy link
Contributor

@charlotteliang charlotteliang May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible that the fetchStatus is successful and fetchError also exists?

Copy link
Member Author

@paulb777 paulb777 May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

? FIRRemoteConfigFetchAndActivateStatusSuccessUsingPreFetchedData
: FIRRemoteConfigFetchAndActivateStatusError;
completionHandler(status, fetchError);
}
};
[self fetchWithCompletionHandler:fetchCompletion];
Expand Down