-
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 initial tvOS support to some firebase packages #8109
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Adds compatibility with tvOS (using react-native-tvos) for RNFirebase packages: app, auth, firestore, functions, storage and app check
@@ -10,6 +10,7 @@ if coreVersionDetected != coreVersionRequired | |||
end | |||
firebase_ios_target = appPackage['sdkVersions']['ios']['iosTarget'] | |||
firebase_macos_target = appPackage['sdkVersions']['ios']['macosTarget'] | |||
firebase_tvos_target = appPackage['sdkVersions']['ios']['tvosTarget'] |
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.
You can simplify this and just keep tvOS target the same as iOS.
@@ -76,7 +76,8 @@ | |||
"ios": { | |||
"firebase": "11.4.0", | |||
"iosTarget": "13.0", | |||
"macosTarget": "10.15" | |||
"macosTarget": "10.15", | |||
"tvosTarget": "12.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 be 13.4 or higher for compatibility with RNTV 0.73 and later
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.
Bit of a catch :) If we use firebase_ios_target for tvOS then there isn't a way set tvosTarget to to 13.4 without also setting iosTarget, which I'm not sure we want to do when most users aren't targeting tvOS?
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.
I'm okay with having these separate, and we align the minimums with the firebase-ios-sdk podspecs not with react-native. The Auth podspec over there is the highest one so it determines all, and it is currently at 13.0:
So in final form what I will do here is keep them separate, but bump to 13.0
Adds compatibility with tvOS (using react-native-tvos) for RNFirebase packages: app, auth, firestore, functions, storage and app check
Description
Two main changes:
RNFBAuthModule.m
to only remove incompatible tvOS APIsThis allows running the updated Firebase packages on tvOS.
Related issues
Not aware of an open issue but discussion here:
#5722 (comment)
Release Summary
Adds compatibility with tvOS (using react-native-tvos) for RNFirebase packages: app, auth, firestore, functions, storage and app check
Checklist
Android
iOS
(well sort of,tvOS
)e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
Not sure how to add a test plan for this change.
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter