Remove the need for s.static_framework = true by adding missing dependencies to React-CoreModules.podspec

[jtreanor]

Summary

It was pointed out in #25818 (comment) that the iOS framework tests were failing on the 0.61-stable branch. As a quick fix @grabbou reverted #25816. This got us out of the bind but has potentially undesirable implications of requiring s.static_framework = true in every podspec that depends on RN.

This is the clean, simple solution to avoid that. I have reverted the temporary patch and updated React-CoreModules.podspec to fix the build error.

Changelog

I don't think we need an entry for this.

Test Plan

test_ios_frameworks should be passing on CircleCI now.

[pull-bot]

	Warnings
⚠️	❔ Base Branch - The base branch for this PR is something other than master. Are you sure you want to merge these changes into a stable release? If you are interested in backporting updates to an older release, the suggested approach is to land those changes on master first and then cherry-pick the commits into the branch for that release. The Releases Guide has more information.

Messages

📖

📋 Verify Changelog Format - A changelog entry has the following format: [CATEGORY] [TYPE] - Message. CATEGORY may be: General iOS Android JavaScript Internal (for changes that do not need to be called out in the release notes) TYPE may be: Added, for new features. Changed, for changes in existing functionality. Deprecated, for soon-to-be removed features. Removed, for now removed features. Fixed, for any bug fixes. Security, in case of vulnerabilities. MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against e51227c

[jtreanor]

I'm not sure why the tests have failed, it looks like it might be a timeout. It is working locally for me.

[fkgozali]

Looks like this is for 0.61 RC branch, if this works, let's just merge it directly there. cc @grabbou

[grabbou]

I've restarted the job. I had this failure myself as well a couple of times.

[jtreanor]

Hmm, looks like its still hanging. The previous error is definitely fixed (that was a compile error). I'll take a look.

[jtreanor]

Oh, I am seeing the failure locally now:

"Unhandled JS Exception: TurboModuleRegistry.getEnforcing(...): 'PlatformConstants' could not be found. Verify that a module by this name is registered in the native binary

Looks like we will need more podspec changes. I'll get to digging.

[jtreanor]

Okay, so here's the problem. TurboModuleRegistry.js tries to load the exported PlatformConstants module which is refined in CoreModules (RCTPlatform.mm). It fails if not statically linked because CoreModules and Turbomodules are in different binaries (CoreModules.framework and ReactCommon.framework).

The best way to solve this would be to move CoreModules into ReactCommon.podspec as a subspec so it would all live under the same framework. This would mean moving React/CoreModules to ReactCommon/CoreModules. Is there any reason not to do this?

[jtreanor]

Update: FBReactNativeSpec would also need to be moved under ReactCommon.podspec to avoid cyclical dependencies, but that uncovers more of the C++ import errors 😞I'm sure this is solvable, but it is rapidly growing legs.

There should still be no problem with use_frameworks! in projects that do not use CoreModules even without s.static_framework = true. I'm not familiar enough with Core Modules to know how big of a deal that is. Any thoughts?

[fkgozali]

It fails if not statically linked because CoreModules and Turbomodules are in different binaries

Do you know what the failure is?

The best way to solve this would be to move CoreModules into ReactCommon.podspec as a subspec so it would all live under the same framework.

I'm not sure we can do that, because CoreModules have bunch of iOS-only stuffs, while ReactCommon is mostly C++.

[jtreanor]

Do you know what the failure is?

The failure is the one I posted above ("Unhandled JS Exception: TurboModuleRegistry.getEnforcing(...): 'PlatformConstants' could not be found. Verify that a module by this name is registered in the native binary). It is being thrown from here.

I've done some more digging and I'm still not totally clear why this is happening. I put a breakpoint in +(void)load of RCTPlatform.mm and the crash occurs before it ever gets a chance to call RCTRegisterModule(self);.

So for some reason, the classes in the CoreModules framework are not yet loaded when the module registry expects them to be. I guess making everything static works because everything is loaded at once.

It may be relevant that React-CoreModules is not a dependency of any of the other podspecs (its just in the Podfile) so perhaps this leads iOS to think it doesn't need to be loaded yet. I do admit that my knowledge of class loading is hazy however.

Maybe there is something we can do in React-CoreModules to force the classes to load (-force_load or similar). I'll try that tomorrow.

I'm not sure we can do that, because CoreModules have bunch of iOS-only stuffs, while ReactCommon is mostly C++.

Yeah, that could be a problem. Maybe there is another solution here.

[fkgozali]

Btw, did you have turbomodule enabled or disabled?

Just to double check re: class loading issue. If you add this bridge delegate method:

react-native/React/Base/RCTBridgeDelegate.h

Line 57 in 04ea976

- (BOOL)bridge:(RCTBridge *)bridge didNotFindModule:(NSString *)moduleName;

and disable turbomodule, did the method get called with the missing module name?

[jtreanor]

Btw, did you have turbomodule enabled or disabled?

Oh, good catch! They were enabled. I just tried with turbomodules disabled and the tests pass and everything seems to work.

Just to double check re: class loading issue. If you add this bridge delegate method and disable turbomodule, did the method get called with the missing module name?

With turbomodules disabled, bridge:didNotFindModule: is not called with the problematic module (PlatformConstants), but with turbomodules enabled, it is called with that.

Interestingly, without use_frameworks! and with turbomodules enabled bridge:didNotFindModule: is also called with PlatformConstants, but the tests pass.

So for some clarity:

	Turbomodules enabled	Turbomodules disabled
With use_frameworks!	Tests fail & delegate is called	Tests pass & delegate not called
Without use_frameworks!	Tests pass & delegate is called	Tests pass & delegate not called

We seem to be zeroing in on the cause here. Somehow turbomodules aren't playing nicely with use_frameworks!. I'm going to continue digging to see if I can find out why.

[fkgozali]

Oh, good catch! They were enabled. I just tried with turbomodules disabled and the tests pass and everything seems to work.

OK, good to know. RNTester has it enabled, so at least we now know that it's TM setup specific.

Interestingly, without use_frameworks! and with turbomodules enabled bridge:didNotFindModule: is also called with PlatformConstants, but the tests pass.

The delegate method will be called because the legacy system doesn't find the module. So that's normal. The question now is, what's different for use_frameworks! vs without. I wonder if this C function gets called correctly and whether it returns the right class for you:

https://github.com/facebook/react-native/blob/master/React/CoreModules/CoreModulesPlugins.mm#L27

[jtreanor]

The question now is, what's different for use_frameworks! vs without. I wonder if this C function gets called correctly and whether it returns the right class for you:

Yes, that function is getting called and appears to be working correctly. -[RCTTurboModuleManager provideRCTTurboModule:] is also called with PlatformConstants and seems to correctly create the module.

However, placing a breakpoint here in TurboModuleRegistry.js, I can see that global.__turboModuleProxy is undefined when get() is called. This is why it always returns null. This is despite the fact that TM has been installed/set up at this point.

[jtreanor]

TurboModuleBinding::install() is definitely getting called before the failure happens, so global.__turboModuleProxy should be set. I've confirmed other keys that are set in a similar way such as global.nativeLoggingHook are present.

[jtreanor]

Actually, on closer inspection, it looks like TurboModuleBinding::install() may not be called until after the code in TurboModuleRegistry.js, which would explain why global.__turboModuleProxy isn't yet set. I'm not totally certain on which happens first though.

If I'm right, this seems to be a race condition between TM being setup and the JS being executed. I don't have very much knowledge about how this works internally so my investigations here are a little slow. Any pointers would be much appreciated!

[fkgozali]

Actually, on closer inspection, it looks like TurboModuleBinding::install() may not be called until after the code in TurboModuleRegistry.js, which would explain why global.__turboModuleProxy isn't yet set. I'm not totally certain on which happens first though.

The binding is installed before js bundle is evaluated, so it should be available all the time, unless something changed or something threw a silent error somewhere.

Do you know if this works on master? I'll try to debug further on the master build.

[fkgozali]

Also, this is in RNTester right?

[jtreanor]

Also, this is in RNTester right?

Yes.

Do you know if this works on master? I'll try to debug further on the master build.

It's actually more broken on master, because it fails to build too, which is what 7f3a0e8 in this PR fixes. However, after cherry-picking that commit on master to fix the build error, I can reproduce the problem.

Here are the steps I'm using to reproduce:

1. cd RNTester && USE_FRAMEWORKS=1 bundle exec pod install
2. Open RNTesterPods.xcworkspace
3. Run the RNTesterIntegrationTests (or all the tests).
4. It will crash with the PlatformConstants error.

[fkgozali]

OK. I started some debugging yesterday and was hitting Xcode/simulator issue, likely unrelated to RNTester itself. I'll keep digging.

[jtreanor]

Thanks @fkgozali!

Its EOD here but I'll try check back later if there is anything I can clarify. I'll be able to spend more time investigating or fixing tomorrow if required.

[fkgozali]

I got the build working on master with use_frameworks!, with just the following 2 lines of change to React-CoreModules.podspec:

@@ -40,6 +40,8 @@
 
   s.dependency "FBReactNativeSpec", version
   s.dependency "Folly", folly_version
+  s.dependency "RCTTypeSafety", version
   s.dependency "React-Core/CoreModulesHeaders", version
+  s.dependency "React-RCTImage", version
   s.dependency "ReactCommon/turbomodule/core", version
 end

I didn't hit the redbox that you observed previously, so I wonder if we had different setup or something?

Note: I'll land the change on master regardless, so perhaps it can help you test master as well.

[jtreanor]

Oh, that’s interesting. The error I’m seeing is the CI failure here too afaik.

That will be helpful, thanks!

[fkgozali]

Here are the steps I'm using to reproduce:

did you happen to start up metro? If so, what command did you use?

[fkgozali]

here's the commit from master btw:

15b2353

[jtreanor]

did you happen to start up metro? If so, what command did you use?

I didn't start it manually, but it was launch by the build phase when I run the tests. However, I get the same failure when starting it with yarn start.

Do you see the tests passing for you? It is failing on CI both on master and here.

[fkgozali]

the test passed for me and in our CI. I wonder if there's some env difference, I'll try a fresh clone if the repo and explore more

[jtreanor]

Ah, so its passing in FB CI? I wonder what is different between that and CircleCI.

[fkgozali]

OK I can repro with running the test (fresh github clone). Running RNTester app itself is fine though, so maybe we're missing a test setup somewhere. I'll debug further.

[fkgozali]

I debugged this a bit more, it's definitely a race condition between the lookup of the module vs the __turboModuleProxy installation. We'll need to investigate further.

Note that RNTester app (with frameworks) seemed to work fine, it's just the tests that are failing. So I have a few suggestions:

• Does it make sense to disable the test run for when use_frameworks! is set? We can run the same set of tests without use_frameworks! and that's probably good enough?
• To unblock 0.61, we can move RCTPlatform back to be old NativeModule, instead of TurboModule. To do that, we can remove RCTTurboModule protocol from this definition:

  react-native/Libraries/FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec.h

  Line 1593 in 724fe11

  @protocol NativePlatformConstantsIOSSpec <RCTBridgeModule, RCTTurboModule>

  - if that works on the branch, we should just proceed with that

[fkgozali]

Does it make sense to disable the test run for when use_frameworks! is set?

I meant, the CI job should just verify that it compiles, no need to run the unit tests.

[fkgozali]

I found the issue: for whatever reason, the test may set up a brand new bridge, separate from RNTester app's bridge (that is set up in AppDelegate.mm). For example:

react-native/RNTester/RNTesterIntegrationTests/RCTLoggingTests.m

Line 41 in 2dadb9e

_bridge = [[RCTBridge alloc] initWithBundleURL:scriptURL moduleProvider:NULL launchOptions:nil];

We enabled TM in the RNTester AppDelegate.mm, so when the individual test setup runs, RCTTurboModuleEnabled() is YES even though it doesn't have the right configuration (e.g. no TM Manager, no TM manager delegate impl, etc).

This is non ideal, but for now let's disable TM for any test purpose. I'll send a fix.

[fkgozali]

@jtreanor 241091d disabled turbomodule for test runs, it should fix the issue you're facing

[fkgozali]

@grabbou, once we pick 241091d, this PR can land to 0.61 RC, so that lib owners aren't forced to use "static libs"

[MoOx]

Indeed having to patch all libs is not ideal 😂. Can't wait to have this merged & released!

[jtreanor]

Sorry about the delay in responding here. I have been away for a few days.

Thanks for continuing the investigation @fkgozali. I have rebased the branch and cherry picked 241091d. Hopefully that should be enough to resolve it now.

[jtreanor]

I'm not sure why this is failing now. I'll try take a look soon.

[fkgozali]

error: /Users/distiller/react-native/RNTester/Pods/Target Support Files/Yoga/Yoga.xcconfig: unable to open file (in target "Yoga" in project "Pods") (in target 'Yoga')

cc @axe-fb - was there Yoga podspec change?

[MoOx]

I got this yesterday

Fetching podspec for `yoga` from `../node_modules/react-native/ReactCommon/yoga`
[!] The name of the given podspec `Yoga` doesn't match the expected one `yoga`

I had to change my podfile from

pod 'yoga', :path => '../node_modules/react-native/ReactCommon/yoga'

to

pod 'Yoga', :path => '../node_modules/react-native/ReactCommon/yoga'

I think #26359 and/or #26360 try to fix this.

[fkgozali]

Maybe those PRs just need to be picked to RC? cc @grabbou

[jtreanor]

@fkgozali @grabbou It looks like these changes have been applied to the 0.61 branch. Thanks for taking care of it!

I think we can close this now.

[fkgozali]

Thanks for working on this as well, @jtreanor!

[ZachOrr]

Hey y'all - I'm seeing the TurboModuleRegistry/PlatformConstants exception that was being talked about when building against 0.61.1. The discussion seems to indicate this was fixed in the 0.61 release - am I missing a workaround or solution? Thanks!

For context - I am using Cocoapods with use_frameworks!

[kyle-ssg]

Note, due to this podspec change in Yoga, I believe people may have to git remove the Target Support Files folder from git and then re-add as it would almost certainly show the folder Yoga as "yoga". For me, this was breaking appcenter CI builds.