-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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: add tvOS support #8111
feat: add tvOS support #8111
Conversation
Adds compatibility with tvOS (using react-native-tvos) for RNFirebase packages: app, auth, firestore, functions, storage and app check
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
7243e3d
to
2c1a689
Compare
2c1a689
to
6707df3
Compare
this matches upstream style
macOS platform guard was missing, one method was missing all non-iOS guards
6707df3
to
be74e45
Compare
@@ -55,7 +55,7 @@ | |||
static __strong NSMutableDictionary *emulatorConfigs; | |||
// Used for caching credentials between method calls. | |||
static __strong NSMutableDictionary<NSString *, FIRAuthCredential *> *credentials; | |||
#if !TARGET_OS_TV | |||
#if TARGET_OS_IOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the multifactor (and phone) stuff is guarded this way, example: https://github.com/firebase/firebase-ios-sdk/blob/ae2554220afb03a5d6fc389c28a86061a12c7caa/FirebaseAuth/Sources/ObjC/FIRMultiFactorConstants.m#L18
@@ -55,18 +55,18 @@ - (void)configure:(FIRApp *)app | |||
} | |||
|
|||
if ([providerName isEqualToString:@"appAttest"]) { | |||
if (@available(iOS 14.0, macCatalyst 14.0, tvOS 15.0, watchOS 9.0, *)) { | |||
if (@available(iOS 14.0, macOS 11.0, macCatalyst 14.0, tvOS 15.0, watchOS 9.0, *)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just an oversight before, min versions guided by https://firebase.google.com/docs/ios/learn-more#firebase_library_support_by_platform
@"debug provider to guarantee invalid tokens in this invalid configuration."); | ||
self.delegateProvider = [[FIRAppCheckDebugProvider alloc] initWithApp:app]; | ||
} | ||
} | ||
|
||
if ([providerName isEqualToString:@"appAttestWithDeviceCheckFallback"]) { | ||
if (@available(iOS 14.0, *)) { | ||
if (@available(iOS 14.0, macOS 11.0, macCatalyst 14.0, tvOS 15.0, watchOS 9.0, *)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should have been guarded for all the platforms during initial implementation, oversight
@@ -77,7 +77,7 @@ | |||
"firebase": "11.4.0", | |||
"iosTarget": "13.0", | |||
"macosTarget": "10.15", | |||
"tvosTarget": "12.0" | |||
"tvosTarget": "13.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FirebaseAuth is currently the highest minimum version for iOS, consulted it here and it was 13 for tvOS as well
https://github.com/firebase/firebase-ios-sdk/blob/36da449b15619d301302faeba639c7893d576220/FirebaseAuth.podspec#L24
Which matches Core, so I chose 13
https://github.com/firebase/firebase-ios-sdk/blob/36da449b15619d301302faeba639c7893d576220/FirebaseCore.podspec#L23
Description
This supercedes:
It is based on that PR from @adamkoch but I can't push to that branch and I have done a series of little fixups on it
Related issues
Discussion:
Release Summary
This should be a squash merge, the PR title should serve for release as conventional commit message
Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
The base of the PR style-wise is adding the platform to package.json then using it in podspecs, that is tested already by library consumer so should work
The compile ifdefs will be tested on iOS at least and that should be sufficient
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter