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

Adding tvOS support #91

Merged
merged 2 commits into from
Sep 8, 2019
Merged

Adding tvOS support #91

merged 2 commits into from
Sep 8, 2019

Conversation

douglowder
Copy link
Collaborator

@brodybits brodybits merged commit 5277485 into brodybits:dev Sep 8, 2019
@brodybits
Copy link
Owner

Thanks @dlowder-salesforce for the nice contribution. (You can probably see that I did yarn jest -u to update the tests.)

I will try to release it in the next few days.

I would like to add a tip for the future to avoid proposing changes from the default branch.

@douglowder
Copy link
Collaborator Author

Ah ok thanks for the tip about the snapshot tests. Should I add a PR for tvOS info in the README.md?

@brodybits
Copy link
Owner

That would be nice, thanks.

I have some general thoughts on the tvOS platform that I will probably raise in a new issue there.

@brodybits
Copy link
Owner

TBH I am actually a bit reluctant to provide formal support for tvOS at this point for several reasons:

  • impact on generated project: modification to react-native dependency entries in generated package.json and generated example would be needed
  • possibly inconsistent support between RN 0.59, 0.60, and newer versions in the future
  • view support (using the --view option) is unknown, to me at least
  • testing effort that would be needed to maintain the support in upcoming releases

My idea right now is to just document a pointer to an issue where we discuss what users would need to do to support tvOS.

As a side point, I wonder if we could make a similar change to support macOS?

Thanks again for the contribution.

@douglowder
Copy link
Collaborator Author

douglowder commented Sep 9, 2019

TBH I am actually a bit reluctant to provide formal support for tvOS at this point for several reasons:

impact on generated project: modification to react-native dependency entries in generated package.json and generated example would be needed

This would not be needed. My testing shows that the existing react-native dependency entries are fine for importing a module into a project that depends on react-native-tvos. We could add a command line option like --use-react-native-tvos for developers that wanted the example to work on tvOS out of the box.

possibly inconsistent support between RN 0.59, 0.60, and newer versions in the future

I'm fine with only supporting tvOS in 0.60 and higher. There is a working 0.59 react-native-tvos release -- I will test with that version to see if there are issues.

view support (using the --view option) is unknown, to me at least

View support would be the same.... a module with a view manager would just have to not use things like sliders and switches that don't exist on tvOS.

testing effort that would be needed to maintain the support in upcoming releases

I'll have a look at your test automation to see how to add that support. Do you have a set of manual tests that you run regularly?

My idea right now is to just document a pointer to an issue where we discuss what users would need to do to support tvOS.

Definitely, we should have an issue or a doc where we put the things that developers should watch out for.

As a side point, I wonder if we could make a similar change to support macOS?

I don't know much about the state of macOS support....

@brodybits
Copy link
Owner

I am also curious about the motivation to support tvOS, as it seems to me not so widely used. Am I missing anything?

I am also having trouble testing with tvOS and would appreciate some pointers.

And thanks for your answers, my response coming soon.

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.

None yet

2 participants