Skip to content
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

The instanceID sync API is deprecated. Implement the async instanceID API #3993

Merged
merged 2 commits into from Oct 9, 2019

Conversation

@dmandar
Copy link
Contributor

@dmandar dmandar commented Oct 3, 2019

No description provided.

@dmandar dmandar requested a review from paulb777 Oct 3, 2019
@dmandar dmandar requested a review from maksymmalyhin Oct 3, 2019
Copy link
Member

@paulb777 paulb777 left a comment

LGTM, deferring to Maksym for approval.

FIRLogInfo(kFIRLoggerRemoteConfig, @"I-RCN000022", @"Success to get iid : %@.",
instanceIDStrongSelf->_settings.configInstanceID);
} else {
FIRLogWarning(kFIRLoggerRemoteConfig, @"I-RCN000055", @"Error getting iid : %@.",
Copy link
Member

@paulb777 paulb777 Oct 3, 2019

Choose a reason for hiding this comment

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

Is it possible for !error && !identity? If so, this will crash.

Copy link
Contributor Author

@dmandar dmandar Oct 9, 2019

Choose a reason for hiding this comment

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

yes it is..not sure where we would crash though..

Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

@dmandar Thank you for updating it!

Mostly LGTM, just one question in the comments.

__weak RCNConfigFetch *instanceIDSelf = strongSelf;
[instanceID getIDWithHandler:^(NSString *_Nullable identity, NSError *_Nullable error) {
RCNConfigFetch *instanceIDStrongSelf = instanceIDSelf;
instanceIDStrongSelf->_settings.configInstanceID = identity;
Copy link
Contributor

@maksymmalyhin maksymmalyhin Oct 4, 2019

Choose a reason for hiding this comment

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

Should _settings be modified from the _lockQueue? [instanceID getIDWithHandler:] is called on the main queue. I wonder if the code will be simpler if we call [instanceID getIDWithHandler:] first (before line 209) and move the code with dispatch_async(instanceIDHandlerSelf->_lockQueue inside the getIDWithHandler: handler?

Copy link
Contributor Author

@dmandar dmandar Oct 9, 2019

Choose a reason for hiding this comment

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

Good pt. Done.

Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

LGTM

@dmandar dmandar merged commit 6f5c203 into master Oct 9, 2019
2 checks passed
@paulb777 paulb777 added this to the 6.11.0 milestone Oct 16, 2019
@firebase firebase locked and limited conversation to collaborators Nov 9, 2019
@paulb777 paulb777 deleted the md-asynciidapi branch Feb 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants