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.

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

chore: prepare for Firebase SDK version bumps #6120

Merged
merged 12 commits into from
May 12, 2021
Merged

chore: prepare for Firebase SDK version bumps #6120

merged 12 commits into from
May 12, 2021

Conversation

Salakar
Copy link
Member

@Salakar Salakar commented May 12, 2021 •
edited
Loading

Description

See individual commits for details of changes.

Related Issues

Unblocks #5936 & App Check.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

@google-cla google-cla bot added the cla: yes label May 12, 2021
@Salakar Salakar marked this pull request as ready for review May 12, 2021 10:50
@Salakar Salakar requested review from Ehesp and kroikie as code owners May 12, 2021 10:50
@github-actions
Copy link

github-actions bot commented May 12, 2021 •
edited
Loading

Visit the preview URL for this PR (updated for commit 09d730b):

https://flutter-firebase-docs--pr6120-salakar-bump-sdks-ven6emy8.web.app

(expires Wed, 19 May 2021 16:04:37 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

russellwheatley
russellwheatley approved these changes May 12, 2021
View reviewed changes
Copy link
Member

@russellwheatley russellwheatley left a comment

Choose a reason for hiding this comment

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

providing it passes tests, LGTM.

mikehardy
mikehardy approved these changes May 12, 2021
View reviewed changes
Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

LGTM - clever leaving admob in but unhitching it like that. I'll see if that's possible with RNFB...

@Salakar
Copy link
Member Author

Salakar commented May 12, 2021 •
edited
Loading

LGTM - clever leaving admob in but unhitching it like that. I'll see if that's possible with RNFB...

@mikehardy I had to go quite far back on the iOS version of the pod since FlutterFire AdMob hasn't been updated for a while - to keep the build happy. I reckon RNFB will be ok with the latest version however since it was kept in sync with Firebase core.

Salakar added 3 commits May 12, 2021 15:28
@Salakar
refactor(messaging)!: remove support for senderId named argument on…
58c3ce6 
… `getToken` & `deleteToken` methods since the native Firebase SDKs no longer support it cross-platform.
@Salakar
refactor(core,web): firebase JS SDK version check logic should happen…
69ee89c 
… after other assertions
@Salakar
chore(android,auth,tests): disable phone auth related tests until And…
09d730b 
…roid SDK issue is resolved
@Salakar Salakar force-pushed the @salakar/bump-sdks branch from 0e8bfcf to 09d730b Compare May 12, 2021 16:01
@Salakar Salakar merged commit ae683e5 into master May 12, 2021
@Salakar Salakar deleted the @salakar/bump-sdks branch May 12, 2021 17:41
mikehardy
mikehardy reviewed May 12, 2021
View reviewed changes
...roid/src/main/java/io/flutter/plugins/firebase/messaging/FlutterFirebaseMessagingPlugin.java
Comment on lines -166 to +156
String token = FirebaseInstanceId.getInstance().getToken(senderId, "*");
String token = Tasks.await(FirebaseMessaging.getInstance().getToken());
Copy link
Contributor

Choose a reason for hiding this comment

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

This is potentially a big deal - needs testing to determine if deleteToken/getToken actually behave now, c.f.

firebase/firebase-ios-sdk#6433 (comment)
invertase/react-native-firebase#3714 (comment)
invertase/react-native-firebase#1510 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

For anyone checking this later, if that ever happens. I added a test for this scenario in react-native-firebase that exercised the underlying ios / android SDKs and their behavior appears correct now, this is not an issue (anymore) as far as I can tell. 👍

@firebase firebase locked and limited conversation to collaborators Jun 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mikehardy mikehardy mikehardy approved these changes

@russellwheatley russellwheatley russellwheatley approved these changes

@kroikie kroikie kroikie approved these changes

@Ehesp Ehesp Ehesp approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Salakar @mikehardy @Ehesp @kroikie @russellwheatley