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: Add initial tvOS support to some firebase packages #8109

Closed
wants to merge 1 commit into from

Conversation

adamkoch
Copy link
Contributor

@adamkoch adamkoch commented Nov 1, 2024

Adds compatibility with tvOS (using react-native-tvos) for RNFirebase packages: app, auth, firestore, functions, storage and app check

Description

Two main changes:

  • Updates the podspec files of packages to add tvOS deployment targets.
  • Updates RNFBAuthModule.m to only remove incompatible tvOS APIs

This 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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS (well sort of, tvOS)
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

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:

Copy link

vercel bot commented Nov 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 1, 2024 11:21pm

@CLAassistant
Copy link

CLAassistant commented Nov 1, 2024

CLA assistant check
All committers have signed the CLA.

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']

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"

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

Copy link
Contributor Author

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?

Copy link
Collaborator

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:

https://github.com/firebase/firebase-ios-sdk/blob/ae2554220afb03a5d6fc389c28a86061a12c7caa/FirebaseAuth.podspec#L22-L25

So in final form what I will do here is keep them separate, but bump to 13.0

@mikehardy mikehardy mentioned this pull request Nov 2, 2024
8 tasks
@mikehardy
Copy link
Collaborator

I couldn't push to this fork:branch combo with my fixups so I've posted them all here:

I will close this PR but obviously it is the base of that one - if you could head over there and check the add-on commits to see the changes I made, I think they should work?

You can grab the patch set for testing via patch-package by following the details link on the CI patch-package workflow and looking at the summary there

@mikehardy mikehardy closed this Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants