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.

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

@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

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

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

Choose a reason for hiding this comment

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

LGTM

@dmandar dmandar merged commit 6f5c203 into master Oct 9, 2019
@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 February 2, 2020 01:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants