Skip to content

Remove deprecated token callback #1074

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

Merged
merged 4 commits into from
Apr 12, 2018
Merged

Remove deprecated token callback #1074

merged 4 commits into from
Apr 12, 2018

Conversation

charlotteliang
Copy link
Contributor

The deprecated token callback has been replaced by messaging:didReceiveRegistrationToken:.

Copy link
Contributor

@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

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

Had a question and a nit. Other than that LGTM

@@ -572,18 +572,16 @@ - (void)setDelegate:(id<FIRMessagingDelegate>)delegate {
[self validateDelegateConformsToTokenAvailabilityMethods];
}

// Check if the delegate conforms to either |didReceiveRegistrationToken:| or
// |didRefreshRegistrationToken:|, and display a warning to the developer if not.
// Check if the delegate conforms to either |didReceiveRegistrationToken:|,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove either

Optionally, the comma at the end of the line can be removed too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

__deprecated_msg("Please use messaging:didReceiveRegistrationToken:, which is called for both \
current and refreshed tokens.");
NS_SWIFT_NAME(messaging(_:didReceiveRegistrationToken:))
__IOS_AVAILABLE(10.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a lower iOS version? If this method isn't available, then will iOS 9 and lower systems have no way of receiving token updates?

Copy link
Member

Choose a reason for hiding this comment

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

+1, why is this iOS 10+?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, this should be universal. For some reason, the protocol is commented to support iOS 10+ but this method somehow doesn't, so I should remove the support commend in protocol to avoid confusion.

FIRMessagingLoggerWarn(kFIRMessagingMessageCodeTokenDelegateMethodsNotImplemented,
@"The object %@ does not respond to "
@"-messaging:didReceiveRegistrationToken:, nor "
@"-messaging:didRefreshRegistrationToken:. Please implement "
@"-messaging:didReceiveRegistrationToken:. Please implement "
Copy link
Member

Choose a reason for hiding this comment

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

Nit: While we're making this change, we should change to the Swift signature to match (almost) all of our other logs and documentation: messaging(_:didReceiveRegistrationToken:).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I will notify the doc team on the change during launch.

I believe all the public doc is using the recommended API (objc & swift), also I've checked all source code the deprecated methods have been removed. Let me know if there's anything you think might be missing.

__deprecated_msg("Please use messaging:didReceiveRegistrationToken:, which is called for both \
current and refreshed tokens.");
NS_SWIFT_NAME(messaging(_:didReceiveRegistrationToken:))
__IOS_AVAILABLE(10.0);
Copy link
Member

Choose a reason for hiding this comment

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

+1, why is this iOS 10+?

@charlotteliang
Copy link
Contributor Author

Done. Removed the unnecessary iOS 10 only label

Copy link
Contributor

@morganchen12 morganchen12 left a comment

Choose a reason for hiding this comment

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

LGTM

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

@charlotteliang charlotteliang merged commit 1397e4a into master Apr 12, 2018
@charlotteliang charlotteliang deleted the fcm-breaking-change branch April 12, 2018 21:24
minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018
@firebase firebase locked and limited conversation to collaborators Nov 4, 2019
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.

5 participants