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.

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

feat!: forward port to firebase-ios-sdk v7+ / firebase-android-sdk v26+ #4249

Merged
merged 3 commits into from
Jan 7, 2021

Conversation

mikehardy
Copy link
Contributor

@mikehardy mikehardy commented Nov 27, 2020

Description

This is a forward port of FlutterFire to firebase-ios-sdk v7+ and firebase-android-sdk v26+

Each commit may be viewed in isolation, and in fact that is recommended.

Related Issues

Unsure if there are other issues to relate but:
#4013 - updating to SDK26 for firestore
#3969 - auth useEmulator discussion - it is gated on this but is trivial once the forward port is done

Checklist

This is my first Dart code, first Flutter code, and first batch of FlutterFire PRs
I did the same port for react-native-firebase though, so I'm new here but have been in the area a lot. Be gentle ;-)

I am not sure what should be done in the CHANGELOG or with regard to version numbers here.

  • 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 (flutter 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

iOS minimum deployment target is moved to 10+
RemoteConfig debugMode no longer exists. minimumFetchTime / fetchTimeout replace it

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@google-cla
Copy link

google-cla bot commented Nov 27, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Nov 27, 2020
@mikehardy

This comment has been minimized.

@google-cla google-cla bot added cla: yes and removed cla: no labels Nov 27, 2020
@mikehardy mikehardy marked this pull request as draft November 27, 2020 21:31
@mikehardy mikehardy force-pushed the sdk-upgrades branch 3 times, most recently from c6724aa to c94b8e9 Compare November 29, 2020 00:39
@mikehardy
Copy link
Contributor Author

@Salakar / @Ehesp this is ready for review, I believe iOS and Android are both working now, should clear the way for auth emulator as a quick followon

I understand there may be concurrent work with regards remote-config port, happy to coordinate on that though I was very very light with regards to my forward-port treatment of it here and kept the commits pretty granular so it should be easy to deal de-conflict / integrate

@mikehardy mikehardy marked this pull request as ready for review November 29, 2020 00:44
@mikehardy
Copy link
Contributor Author

mikehardy commented Nov 29, 2020

GitHub actions aren't wrapped in retry so the android gradle ones flake without recovering as they attempt to pull remote dependencies. Something like this would fix it though it will need to be flutter-ified invertase/react-native-firebase@f1b9af9

Here's a first cut at de-flaking E2E: #4265

One of the storage tests flaked for ios, it's passed before during this PR development and passes locally so should be unrelated to the commit:


[firebase_storage_example]:   updateMetadata
[firebase_storage_example]:      ✔ updates metadata
[firebase_storage_example]:      ✘ errors if metadata update removes existing data
[firebase_storage_example]:        [E]: Expected: {'action': 'updateMetadata test'}
[firebase_storage_example]:               Actual: {'activity': 'test', 'action': 'updateMetadata test'}

If you retry both (I don't have permission) it should go green after one or two attempts, they work locally each time

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.

Ahoy @mikehardy, looking good, just a couple of minor points that I think might be an issue.

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.

LGTM

Copy link
Member

@Salakar Salakar left a comment

Choose a reason for hiding this comment

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

LGTM

This is aligned with firebase-ios-sdk v7 requirements

Example projects' deployment targets bumped to 10
remote-config debugMode is removed, use minimumFetchInterval instead

this should work on ios and android platforms
one symbol in underlying native code changed, but this is not breaking,
the previous commit to move firebase-ios-sdk contained the only breaking
change that was visible in the Dart code / Flutter API
@Salakar Salakar merged commit 73e6820 into firebase:master Jan 7, 2021
@mikehardy mikehardy deleted the sdk-upgrades branch January 7, 2021 16:18
@firebase firebase locked and limited conversation to collaborators Feb 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants