-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Merge IID removal code to master #7814
Conversation
) Co-authored-by: Maksym Malyhin <[email protected]>
Coverage ReportAffected SDKs
Test Logs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few nits from a cursory review since this has already been reviewed in the branch
FirebaseMessaging.podspec
Outdated
@@ -42,7 +42,7 @@ device, and it is completely free. | |||
'Firebase/InstanceID/Public/*.h', | |||
'FirebaseInstallations/Source/Library/Private/*.h', | |||
] | |||
s.requires_arc = base_dir + 'Sources/*.m' | |||
s.requires_arc = base_dir + 'Sources/**/*.m' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that protobuf's non-arc files are gone, this line could be deleted since requires_arc is the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
s.dependency 'FirebaseInstanceID', '~> 7.0' | ||
s.dependency 'FirebaseInstallations', '~> 7.0' | ||
s.dependency 'FirebaseCore', '~> 7.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: alphabetize dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
FirebaseMessaging/CHANGELOG.md
Outdated
@@ -1,3 +1,6 @@ | |||
#unreleased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be 7.11.0
FOUNDATION_EXPORT const int kFIRMessagingSendTtlDefault; // 24 hours | ||
|
||
/** | ||
* Value included in a structured response or GCM message from IID, indicating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it still called IID when it's integrated in FCM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description doesn't make sense. I updated. But yeah FM code-wise should not depend on IID. But might still handle some signals sending from IID in case users still use IID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the podspec as well.
FOUNDATION_EXPORT const int kFIRMessagingSendTtlDefault; // 24 hours | ||
|
||
/** | ||
* Value included in a structured response or GCM message from IID, indicating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description doesn't make sense. I updated. But yeah FM code-wise should not depend on IID. But might still handle some signals sending from IID in case users still use IID.
s.dependency 'FirebaseInstanceID', '~> 7.0' | ||
s.dependency 'FirebaseInstallations', '~> 7.0' | ||
s.dependency 'FirebaseCore', '~> 7.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked through the changes, but didn't dive too deep assuming that the changes were previously reviewed. LGTM. Please make sure not to merge it before M93 branch cut off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7.10 release branch has been made.
The chen/fm-master branch contains all the changes required to remove IID from Messaging.
The code change is targeting in M94 that will not include the part we remove the IID dependency in podspec. We will remove the dependency at the I/O breaking change.