[React Native] Inline calls to FabricUIManager in shared code

[elicwhite]

My previous commit (1b2159a) caused crashes when running in Fabric when navigating from a paper screen to a fabric screen.

The NativeMethodsMixin and ReactNativeComponent code is shared in both the Paper renderer and Fabric renderer. The previous commit caused the top of the paper renderer to include require('FabricUIManager').

FabricUIManager.js is essentially just:

module.exports = global.nativeFabricUIManager;

Because this code was getting called at the top of the paper renderer, FabricUIManager was exporting undefined.

When transitioning to a Fabric screen, c++ installs that global, and then the FabricRenderer has this check:

if (FabricUIManager.registerEventHandler) {
  ...
}

This manifested as a crash cannot access registerEventHandle of undefined. This is because FabricUIManager was cached from being accessed too early.

The "right" fix

We will fix this correctly by having native always, on startup, define and set the object:

global.nativeFabricUIManager = {
  createNode,
  cloneNode,
  measure,
  measureInWindow,
  // ...
} 

When starting with the paper renderer, these functions will just throw.

Then, when Fabric is loaded, it will replace the implementation of those functions with the actual correct implementation.

This will ensure there is no timing issues with when JavaScript reads and caches the value of the global or any of the functions.

We will need to ensure we replace the implementations of the functions and not redefine them because we will need to support JS reading the functions early like this:

const {createNode, measure} = global.nativeFabricUIManager;

Reading them early, and then calling them after Fabric has loaded will need to work.

The "short term" fix

This PR includes the short term fix which is to inline these requires so that it isn't called early. Normally this would happen by default as we run everything inline requires however this file is blacklisted from inline requires in React Native as it is faster this way.

Test Plan:

Opened Marketplace (this worked before this change)
Navigating from Catalyst (paper) to Fabric RNTester (fabric). This crashed before with the error above and no longer crashes.

[sebmarkbage]

I don't think we want any inline requires in React code as this will mess with future builds. Is there another way we can solve this?

[bvaughn]

We already have a few inline requires for our www/fbsource forks. I didn't think there was harm adding another one temporarily.

[sebmarkbage]

Yea, no this is not the right fix. It's effectively a side-effect in the module init which is problematic for many reasons. Like if we eagerly initialize more for better start up perf.

A better fix would be to inline global.nativeFabricUIManager directly instead of relying on the module system working like if it's a function call.

[sebmarkbage]

We already have a few inline requires for our www/fbsource forks. I didn't think there was harm adding another one temporarily.

Previous bad ideas are never good precedence for future bad ideas. That's how small bad ideas leak and become large bad ideas.

[bvaughn]

Uh, ok. I was not aware that we had established this particular thing as a "bad idea" 😄

[sizebot]

Details of bundled changes.

Comparing: 3f058de...9242802

react-native-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
ReactNativeRenderer-dev.js	+0.1%	+0.1%	629.11 KB	629.81 KB	133.6 KB	133.7 KB	RN_FB_DEV
ReactNativeRenderer-prod.js	-0.0%	-0.0%	243.9 KB	243.88 KB	42.37 KB	42.36 KB	RN_FB_PROD
ReactNativeRenderer-profiling.js	-0.0%	-0.0%	252.08 KB	252.06 KB	43.94 KB	43.93 KB	RN_FB_PROFILING
ReactNativeRenderer-dev.js	+0.1%	+0.1%	629.02 KB	629.72 KB	133.57 KB	133.67 KB	RN_OSS_DEV
ReactNativeRenderer-prod.js	-0.0%	-0.0%	243.92 KB	243.89 KB	42.36 KB	42.36 KB	RN_OSS_PROD
ReactNativeRenderer-profiling.js	-0.0%	-0.0%	252.1 KB	252.08 KB	43.94 KB	43.93 KB	RN_OSS_PROFILING
ReactFabric-dev.js	+0.1%	0.0%	617.79 KB	618.56 KB	130.89 KB	130.96 KB	RN_FB_DEV
ReactFabric-prod.js	0.0%	0.0%	237.66 KB	237.69 KB	41.1 KB	41.1 KB	RN_FB_PROD
ReactFabric-profiling.js	0.0%	0.0%	245.31 KB	245.34 KB	42.71 KB	42.71 KB	RN_FB_PROFILING
ReactFabric-dev.js	+0.1%	0.0%	617.7 KB	618.47 KB	130.86 KB	130.92 KB	RN_OSS_DEV
ReactFabric-prod.js	0.0%	0.0%	237.67 KB	237.69 KB	41.09 KB	41.1 KB	RN_OSS_PROD
ReactFabric-profiling.js	0.0%	0.0%	245.33 KB	245.35 KB	42.71 KB	42.71 KB	RN_OSS_PROFILING

Generated by 🚫 dangerJS

[sebmarkbage]

I'm surprised lint and Flow lets you do this.

Would be nice to declare it as a top level global property here:

https://github.com/facebook/react/blob/master/scripts/flow/react-native-host-hooks.js#L100

[elicwhite]

I'm surprised lint and Flow lets you do this.

Would be nice to declare it as a top level global property here:

https://github.com/facebook/react/blob/master/scripts/flow/react-native-host-hooks.js#L100

Apparently accessing something with global.foo doesn't complain, but just accessing foo does.

Also, apparently maybeInstance.node as well as the mountSafeCallback_NOT_REALLY_SAFE return are both typed as any, so defining the flow types don't help catch anything. We should fix that at some point.