Skip to content

update messaging API based on the Firebase 7 #1056

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 5, 2020
Merged

update messaging API based on the Firebase 7 #1056

merged 1 commit into from
Oct 5, 2020

Conversation

charlotteliang
Copy link
Contributor

No description provided.

@charlotteliang charlotteliang mentioned this pull request Oct 2, 2020
Merged
morganchen12
morganchen12 reviewed Oct 2, 2020
View reviewed changes
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.

Changes LGTM. Paul brought up in a separate PR that there's an issue with merging these before Firebase 7 lands (doubly so in this case, since it updates a snippet on devsite). We need to resolve that issue before this PR can actually be merged.

@charlotteliang
Copy link
Contributor Author

yeah, i'm holding on this to see how they resolve the other PR.

@charlotteliang
Copy link
Contributor Author

@morganchen12 @paulb777 Somehow this passes the check. Do you think this is still an issue?

@morganchen12
Copy link
Contributor

It's likely because the delegate is ObjC under the hood and ObjC protocols don't enforce types for method parameters, only selector matching. Seems strange that Swift wouldn't raise at least a warning though--maybe because String? is a strict superset of String?

@charlotteliang
Copy link
Contributor Author

Should this still be a blocker for submission? The other PR failed the CI, but this one not.

@paulb777
Copy link
Member

paulb777 commented Oct 5, 2020

I'm fine to merge if CI stays green and there is an eventual plan to update the quickstart.

paulb777
paulb777 requested changes Oct 5, 2020
View reviewed changes
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.

Oh wait, the CI wasn't triggered by the swift cchange

paulb777
paulb777 approved these changes Oct 5, 2020
View reviewed changes
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.

Right, it's only the firebase-ios-sdk change that is blocked. Sorry about my spinning.

@charlotteliang charlotteliang merged commit 5a4de54 into master Oct 5, 2020
@charlotteliang charlotteliang deleted the fm-firebase-7 branch October 5, 2020 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulb777 paulb777 paulb777 approved these changes

@ryanwilson ryanwilson Awaiting requested review from ryanwilson

@morganchen12 morganchen12 Awaiting requested review from morganchen12

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@charlotteliang @morganchen12 @paulb777