-
Notifications
You must be signed in to change notification settings - Fork 52
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
[...] make tvOS support optional #126
Conversation
POTENTIALLY CONTROVERSIAL as tvOS becomes not enabled by default with update of the out-of-tree platform support in README.md
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 actually fine with this approach, especially since RN versions < 0.60 are not supported on tvOS.
Requesting changes on this PR:
- README should say that tvOS works only on 0.60 and above (where linking is done with Cocoapods)
- Same for the --tvos-enabled options text
- Can CLI disallow --tvos-enabled flag if 0.59 or lower RN version is selected?
Thanks @dlowder-salesforce for the quick feedback.
I just raised PR #131 to take care of this step. This PR will need to be updated if PR #131 is approved and merged. In case any changes are needed in PR #131, a "suggested change" comment would be appreciated.
+1, a "suggested change" comment would be appreciated.
Too complex, I think. In general, this module just reacts to the user input if not invalid and I would like to keep it that way if possible. Especially since the exampleReactNativeVersion is not expected to follow straightforward semver rules. My idea now is to resolve this by dropping support for React Native 0.59, as I proposed in #132 and PR #130, and keeping the proposed documentation that use with react-native-tvos@0.59 is not supported. As a side note, I wonder if you guys should tell npm that react-native-tvos pre-0.60 versions are now deprecated? There are still some TODO items before I think we can drop React Native 0.59, big one is automating the |
FYI I am about to merge this update with documentation fixes, will add the tvOS platform fork requirement to the option help text first. I am taking this action in a hurry in order to ship a long-awaited minor release that I will be able to support. Unfortunately I do not have so much extra time resources to actively support working with the tvOS platform fork right now. /cc @dlowder-salesforce |
I have addressed all items in the review, except for dealing with 0.59 for reasons already stated. Merging now in order to unblock new release with tvOS not actively tested for now.
Now merged, with changes cleaned up in the squash merge commit and similar updates applied to the description of this PR. Long awaited release should be published soon. Thanks again @dlowder-salesforce for your contribution of the feature in the first place! |
(tvOS not enabled by default)
UPDATED: with the requirements included in the --tvos-enabled option description
see also:
/cc @dlowder-salesforce