Why tvOS is a fork and not a module? #11
Comments
|
For some more context on this, see react-native-community/discussions-and-proposals#111 |
douglowder
changed the title
|
The key difference between tvOS support in React Native, versus other out of tree platforms like Windows and web, is that tvOS shares almost all its native code with iOS. The tvOS-specific native code can be generally divided into these types:
Ideally, the core repo would have all the first three types of changes, and (When I originally merged the tvOS changes, I added tvOS testing to the CI scripts run in open source; however, at the time, FB had a process that routinely merged changes only after running their own internal CI that didn't include tvOS.) I could provide a tvOS module that would apply a giant patch to all the iOS files in core, but that would be a VERY large patch, probably very brittle to small changes in core, etc., etc. So the best option I can see at the moment is running a fork, where I release versions based on specific core releases. The release notes for each tvOS version show exactly the commits applied on top of the base core release. |
This comment has been minimized.
This comment has been minimized.
|
When I use this fork the app builds and compiles but when I try npm install in changes I get this error : Why? |
I recommend you raise a new issue with a minimal reproduction: https://stackoverflow.com/help/minimal-reproducible-example I am not a maintainer of this module and it is not such a high priority for me to contribute any active support right now. |
|
I just tested this with a new app generated with I think the problem you are seeing is in the "react-native": "npm:react-native-tvos"This is not supported by If you are still seeing this problem, then (as @brodybits suggested above) please create a new issue with a minimal set of repro steps. |
|
From discussion in #14, I would now conclude that this project is really a fork to support one or more TV operating systems, as opposed to a truly separate "out-of-tree" module. This would be very similar to the way that Expo has its own Now downvoting and hiding my own idea in #11 (comment) as resolved. |
|
Since this is a fork of react-native, then the release schedule for this repo will practically not be the same as the parent repo, so in a scenario where I will need to upgrade ios/android mobile apps to latest react-native versions, I will have to wait for tvOS to make a release. Right? |
|
@keshavkaul you are correct, you will have to wait until this repo supports the latest RN core version. That would likely be a problem even if this were an add-on module instead of a fork. There have been quite a few changes in recent versions of RN core that broke tvOS.... |
|
Thanks! I'm now leaning towards having a monorepo project setup either with lerna or yarn workspaces. We plan on supporting mobile, tv, web, firetv, Xbox, so having each platform be a separate package with their own dependencies but re using common code seems ideally feasible. Any thoughts you'd like to share about advantages or disadvantages of monorepos will greatly help me out. Thanks again for the TV repo! I will surely be investing time to contribute here. |
|
@dlowder-salesforce great effort trying to keep react-native tvos alive 🥇 I wondered same when I tried to integrate this into I managed to get some of initial issues out of the way however there are some outstanding issues to tackle (which forced me to stick with RN + some overrides at least for now):
This made it very difficult to use it alongside standard react-native as renative has to support multiplatform monorepos
@keshavkaul multi-platform RN monorepo is definitely possible I think if above obstacles would be solved it could definitely be viable alternative to current RN situation on tvos platform @dlowder-salesforce I think most of the fundamentals are in RN and with good API (RN is moving towards smaller core anyway) tvos support could be achieved with smaller and more focused implementation as native extension and maybe with some overrides (at least that's how I managed to get it all somehow working) Just thinking out loud keep up the great work 👍 |
|
@pavjacko thanks for the excellent commentary -- sorry I didn't respond sooner, I was offline due to medical issues for a couple of weeks.
In a monorepo, I would either use this package, or the RN core package, but not both. This repo is intended to work for iOS, tvOS, and Android, and just add tvOS fixes to what is already in core.
This is discussed in detail in #32
Third party support for RN tvOS is a problem. I've managed to get a number of them working. Most of the time it's just adding tvOS to the podspec, but sometimes actual code changes are required.
I'd be interested to hear about the issues you encountered and your fixes. Did you submit fixes to RN core directly?
Not sure what you mean by this, but if you have ideas that would lead to easier tvOS maintenance, I'd love to hear them :) |
brodycj
changed the title
…(#46702) Summary: Pull Request resolved: facebook/react-native#46702 In facebook/react-native#44188, we've started combining multiple transactions in a single transaction, to meet React's atomicity requirements, while also dealing with the constraints of Android's Fabric implementation. This revealed a bug where in some scenarios (especially when using transitions), a node may be deleted and created during the same transaction. The current implementation of FabricMountingManager assumes it can safely reorder some operations, which it does to optimize the size of IntBufferBatch mount items. This is however incorrect and unsafe when multiple transactions are merged. **Example:** Differentiator output: ``` # Transaction 1 Remove #100 from #11 Delete #100 # Transaction 2 Create #100 Insert #100 into #11 ``` FabricMountingManager output ``` Remove #100 from #11 Insert #100 into #11 Delete #100 ``` Note that the create action is also skipped, because we only update `allocatedViewTags` after processing all mutations, leading FabricMountingManager to assume creation is not required. This leads to an invalid state in SurfaceMountingManager, which will be surfaced as a crash in `getViewState` on the next mutation that interacts with these views. Changelog: [Android][Fixed] Fix crash in getViewState when using suspense fallbacks. Reviewed By: sammy-SC Differential Revision: D63148523 fbshipit-source-id: 07ae26b2f7b7eba1b9784041dd3059b0956c035e
updated:
Summary: see title which was updated several times, by multiple people, and is now a question.
Resolution: see below - agreed on my part
Original rationale:
I did not realize until after I merged brodycj/create-react-native-module#91 that tvOS was actually moved into its own package separate from the core react-native package. I then raised facebook/react-native-website#1250 to add this to the documents.
original idea now withdrawn on my part:
But I am now wondering if we could find a way to improve on a couple things.
It seems to me that this repo is a full-blown fork of react-native. I think it would be been nicer if react-native-tvos would use react-native as a dependency, and reuse as much as possible, in a similar fashion to react-native-windows, react-native-dom, and react-native-turbolinks.
I can think of the following reasons:
(I would also like to see a similar idea applied for react-native-macos.)
more details withdrawn on my part
I think this would be a breaking change that should target an upcoming react-native release, perhaps 0.61.
Unfortunately I don't have so much time to look into this idea right now, hope I can take a look someday.
an old update now withdrawn on my part
P.S. I now think that tvOS should be built from its own `tvos` subdirectory, see my response below.The text was updated successfully, but these errors were encountered: