8000 The instanceID sync API is deprecated. Implement the async instanceID API by dmandar · Pull Request #3993 · firebase/firebase-ios-sdk · GitHub
[go: up one dir, main page]

Skip to content

Conversation

dmandar
Copy link
Contributor
@dmandar dmandar commented Oct 3, 2019

No description provided.

@dmandar dmandar requested a review from paulb777 October 3, 2019 21:50
@dmandar dmandar requested a review from maksymmalyhin October 3, 2019 21:50
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.

4 participants
0