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: bump Firebase iOS SDK 10.5.0 #10532

Merged
merged 35 commits into from
Mar 16, 2023
Merged

feat: bump Firebase iOS SDK 10.5.0 #10532

merged 35 commits into from
Mar 16, 2023

Conversation

russellwheatley
Copy link
Member

@russellwheatley russellwheatley commented Feb 27, 2023

Description

iOS

Considering the implications for each scenario mentioned in this comment on the iOS SDK regarding Firebase Messaging, I believe we only need to set a dummy APNS token to get manually run tests to pass successfully.

Taken directly from that comment:

Scenarios:

On a fresh install of the app,
Firebase configure flow triggers the FCM register flow even though there isn’t a APNS token. You will see an error message due to this flow. This error can be ignored as the FCM token is refetched once an APNS token is provided. I am investigating if this can be fixed in a future SDK release.

For Objective C or Swift (not SwiftUI) projects, if swizzling is enabled,
FCM token refetch happens automatically when we get the APNS token. The swizzling will take care of listening for when APNS token is available. The FCM messaging delegate callback will be called once an FCM token is available.

func messaging(_ messaging: Messaging, didReceiveRegistrationToken fcmToken: String?)

If swizzling is disabled or if your project is pure SwiftUI (swizzling does not work well with SwiftUI projects)
You must implement the UIApplicationDelegate callbacks to listen for APNS token availability. Check the SwiftUI sample in the FCM SDK codebase for how to achieve this.

Testing: If you have unit or integration tests running in the simulator, it is not possible to get an APNS token unless specific environment is used - iOS 16 Simulator + macOS 13 + Apple Silicon HW. If that environment condition is not met, there isn’t an APNS token available in iOS simulator. As a result, any tests for fetching FCM token will error out in the Firebase 10.4.0 SDK. Either reconsider the tests or try to provide fake APNS token for test purposes only. If you provide a fake APNS token, be sure to delete any FCM token created from this process. Example - the FIRMessagingPubSubTest in the Firebase SDK uses a fake APNS token.

As far as I can tell, and after running the example app, I believe the first 3 scenarios mentioned here are taken care of as we set the APNS token here on firebase_messaging for iOS.

This leaves just setting a dummy token for the iOS simulator when it doesn't support receiving a device token (taken from the message above, this environment supports receiving the token on a simulator: iOS 16 Simulator + macOS 13 + Apple Silicon HW.), I have simply set a dummy APNS token here and wrapped it in an iOS simulator macro and checked the device token wasn't received. This means it will only set a dummy APNS token on simulator builds that don't support receiving the device token.

macOS

firebase_auth needs keychain sharing capability on macOS applications. The tests pass locally once keychain sharing capability is allowed and also code signing which is necessary for the keychain sharing capability to be enabled. This won't affect production apps as code signing is necessary for distribution.

e2e Tests

I also removed the requestPermission() API request for getToken() & deleteToken() as it is not needed and was timing out the tests when running manually. Presumably because requesting permission requires user response.

Related Issues

I've skipped firebase_auth tests that will fail due to keychain sharing capability. We need the ability to allow provisioning profile updates via xcode build. I've create this issue towards that aim: flutter/flutter#121702

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.

@russellwheatley russellwheatley changed the title feat: iOS SDK 10.5.0 feat: bump Firebase iOS SDK 10.5.0 Feb 27, 2023
@russellwheatley russellwheatley added the blocked: do-not-merge PR blocked externally label Mar 2, 2023
@russellwheatley
Copy link
Member Author

Open issue on Flutter repo for automatic provisioning profile update flag that appears to be required for keychain entitlements: flutter/flutter#121702

@mikehardy
Copy link
Contributor

Just wanted to mention a couple things based on my experience adopting this in react-native-firebase:

1- there are a lot of reasons an APNS token fetch may fail for an app, and developers will (for the first time ever!) be confronted by them. The APNS fetch failures are real problems, and there are a few causes, but they've been ignorable until now. Be prepared to update documentation explaining that disabling swizzling will cause failure (and show what it means...), that network timeouts may cause a failure (so, perhaps folks need to check getAPNSToken() before getToken() or subscribe to topic now, and retry a couple times...). This is a real pain point for library users

2- "(it is not possible to receive messages on a simulator)," - it is possible! If it is latest macOS + iOS, on a M1 (or newer) chip, the iOS simulator can actually do FCM, it's awesome and really useful for devs. I had to remove some compiler #ifdefing to allow it though as the assumption has always been Simulator can't do FCM/APNS. Now it can if the code allows it...

3- exposing setAPNSToken as an API and calling it testing works great on Simulators that cannot do FCM/APNS (that is: current github macOS runners which are x86-64)

@russellwheatley
Copy link
Member Author

Thanks for the feedback, @mikehardy! It's a little different on FlutterFire. We already check device token here and set it as the APNS token here and possibly here depending on the circumstances.

You raise a good point about the latest simulator situation which I was not aware of. I have addressed here.

Effectively, if the apnsToken is nil, it will set a dummy token for APNS in a simulator environment because it means the simulator does not support receiving the device token.

@russellwheatley russellwheatley changed the title feat: bump Firebase iOS SDK 10.5.0 feat!: bump Firebase iOS SDK 10.5.0 Mar 8, 2023
@russellwheatley russellwheatley removed the blocked: do-not-merge PR blocked externally label Mar 8, 2023
reason: 'Stream should only have been called 3 times',
),
);
group(
Copy link
Member Author

Choose a reason for hiding this comment

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

To save reviewers time, the only changes I've made to auth tests is to skip failing tests because of keychain sharing requirement:

    // macOS skipped because it needs keychain sharing entitlement. See: https://github.com/firebase/flutterfire/issues/9538
    skip: defaultTargetPlatform == TargetPlatform.macOS,

Copy link
Member Author

Choose a reason for hiding this comment

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

This file was previously unformatted which is why there is a massive diff.

@russellwheatley russellwheatley added the blocked: do-not-merge PR blocked externally label Mar 10, 2023
@russellwheatley russellwheatley changed the title feat!: bump Firebase iOS SDK 10.5.0 feat: bump Firebase iOS SDK 10.5.0 Mar 14, 2023
@russellwheatley russellwheatley removed the blocked: do-not-merge PR blocked externally label Mar 16, 2023
@Lyokone Lyokone merged commit c77fc4d into master Mar 16, 2023
@Lyokone Lyokone deleted the ios-sdk-10.5.0 branch March 16, 2023 12:37
@firebase firebase locked and limited conversation to collaborators Apr 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants