[TM] Add TurboModules (jsireact) to Xcode project

[radex]

Summary

TurboModules seem to be pretty much done, but there's no easy way to reference them on iOS side if you're linking the React project (the classic, non-CocoaPods way). So I added it.

Also updated RNTester (non-CP based) to compile TM samples

Changelog

[iOS] [Added] - RCTTurboModule now available from RCTTurboModules.xcodeproj

Test Plan

1. Add #import <jsireact/TurboCxxModule.h> to a .h file on iOS project -- see if it compiles correctly

1. Open RNTester/RNTester.xcodeproj, compile, and see if TurboModule sample works

[ericlewis]

@radex oh wow thank you! you just finished one of my todos ;) I will take a look & test this.

[ericlewis]

@radex one thing you will definitely need to change about this: TurboModule's aren't technically ready yet and need to be able to be included as a separate project (similar to how RCTImage or RCTImage are) called RCTTurboModule. The reason for this is we want to leave turbomodules optional until they are completely finished and tested.

As an aside, I am going to test this using RNTester, and make sure it not only compiles, but that it works end to end.

[radex]

@ericlewis Got it. Will refactor this. I'm trying to make RNTester work too, but having some header import issues :)

[ericlewis]

@radex to make it work in RNTester:

1. open RNTester.xcodeproj
2. go to build settings and add -DRN_TURBO_MODULE_ENABLED under "Other C Flags"
3. drag RNTesterTurboModuleProvider.h & RNTesterTurboModuleProvider.mm into the RNTester project. (can find at RNTester/RNTester)
4. the current header imports in the above provider are incorrect, this PR fixes them: [TM] Fixes wrong headers import #25002 it's a simple change, just copy paste. But I suspect you will still have problems with these specific imports when trying to import them this way. Since they are namespaced, so we may have to also change that.

edit: it does appear to work :) using the below for headers in RNTesterTurboModuleProvider.h

//#import "SampleTurboCxxModule.h"
#import "RCTSampleTurboModule.h"

btw, SampleTurboCxxModule lives in the the folder above platforms, it is shareable, and should also be included in the RNTester proj. It might even make sense to move it to the RNTester folder since it is now truly shared (and only used by RNTester)

[radex]

@ericlewis OK, this is WIP, but let me know if this approach is what you were looking for. I added RCTTurboModules.xcodeproj, and it compiles correctly

[radex]

Wooohoo! I actually got this working :)

[radex]

@ericlewis I think this is done now :)

[ericlewis]

Wonderful job! Just a few nits:

1. we should rename this to RCTTurboModule not RCTTurboModules for consistency sake.
   1.1 we should also rename the targets to be: RCTTurboModuleCore and RCTTurboModuleSamples (you already have the latter).
2. I am not sure the current location makes sense: /react-native/React, most other iOS libs live in /react-native/Libraries, there is even a /react-native/Libraries/TurboModule folder, so lets move the project file over there for consistency.

That said, I tested it and it appears to be working fine from what I can tell, including removing the library! Great work.

[ericlewis]

Another thing too, we will need to fix the iOS snapshot tests. I am not 100% sure what is wrong, but I am taking a look to see if I have some advice.

[radex]

I am not sure the current location makes sense: /react-native/React, most other iOS libs live in /react-native/Libraries, there is even a /react-native/Libraries/TurboModule folder, so lets move the project file over there for consistency.

Yeah, I didn't like it 100% either… the reason why I did it is because I wanted to reuse React/third-party.xcconfig (which references third-party libs using $SRCROOT)... I can clone the xcconfig, or just add those parameters to Xcode project, if you think that's appropriate.

[ericlewis]

I will defer to @fkgozali on that. Ideally, we don't repeat ourselves ofc. Otherwise, LGTM.

[radex]

@ericlewis Made the renaming changes :)

PS. General question: If this PR is accepted, is it worth it trying to set up a Fabric .xcodeproj in a similar way? Or is there too much going on there / or in general are hand-maintained Xcode project a dead end for RN community and we'll all be using CocoaPods anyway?

[ericlewis]

You can definitely *try* to do this if you like, nothing wrong with that. But it will be much harder to keep in sync than with cocoapods (fabric changes somewhat often). The general consensus too, is that we move all dep management to cocoapods, so it is considered quite low priority currently. There are also codegen steps that must be run as part of Fabric (this happens in prepare_command cocoapods currently). Not to mention running in to super weird issues like Float.h interfering with <Float.h> etc. It is a worthy and difficult challenge. Sent via Superhuman ( https://sprh.mn/[email protected] )

…

On Thu, May 30, 2019 at 1:19 PM, Radek Pietruszewski < ***@***.*** > wrote: @ ericlewis ( https://github.com/ericlewis ) Made the renaming changes :) PS. General question: If this PR is accepted, is it worth it trying to set up a Fabric .xcodeproj in a similar way? Or is there too much going on there / or in general are hand-maintained Xcode project a dead end for RN community and we'll all be using CocoaPods anyway? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub ( #25031?email_source=notifications&email_token=AAFEVRZ53DEAGX6F4UPR3NLPYAEAJA5CNFSM4HPRJFXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWS547I#issuecomment-497409661 ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AAFEVR6HLUUQY6T4VCPZWHTPYAEAJANCNFSM4HPRJFXA ).

[fkgozali]

Thanks for this PR. Since TM is not fully ready, I’m not sure if we want to activate it by default for everyone yet. Also, I’d like to see if we can eventually deprecate the xcodeproj in favor of cocoapods, so that we don’t have to duplicate the build configs here.

cc @hramos

[radex]

Since TM is not fully ready, I’m not sure if we want to activate it by default for everyone yet

Agreed. So that's the advantage of serving it from a separate Xcode file, so that only people "in the know" will try including it in their project (knowing the limitations)

[hramos]

@radex if this wouldn't turn TM on for everyone by default (e.g. they need to explicitly add the TM xcodeproj), then I think it's fine to merge once the iOS test failures are fixed. I believe that'd only require updating the snapshots. The caveat is that the RNTester xcodeproj will go away at some point when we move to the CocoaPods-based RNTesterPods workspace.

[facebook-github-bot]

@fkgozali has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[fkgozali]

I'm landing this for now. We're pretty close to being able to remove .xcodeproj though, but that shouldn't block this PR for now.

[react-native-bot]

This pull request was successfully merged by @radex in bd29112.

^{When will my fix make it into a release? | Upcoming Releases}