Fabric: working podspecs & works in RNTester #23803

ericlewis · 2019-03-07T06:45:41Z

Summary

This is the couple of hacks I used after I finished #23802 in order to get fabric working on RNTester. This is inspired from prior work by @kmagiera.

The goal of this PR is to show others what I’m struggling with, and to eventually merge it sans hacks.

Directions to use

Yarn Install
Uncomment the commented out pods in RNTester's pod file
Open RNTesterPods workspace
Run App

Notes

this is only for pods, the non-pod RNTester will no longer work until updated with fabric too.
SurfaceHostingView & SurfaceHostingProxyRootView both try to start the surface immediately, this leads to a race condition due to the javascript not having loaded yet, the hack here is:
1. Swizzle the start method on RCTFabricSurface to no-op when called.
2. Add observer for RCTJavaScriptDidLoadNotification
3. Call private method _startAllSurfaces on _surfacePresenter in AppDelegate when we receive RCTJavaScriptDidLoadNotification.

Changelog

[General] [Added] - Use Fabric in RNTester

Test Plan

Pod install, and run RNTester.

JoshuaGross · 2019-03-07T19:21:12Z

Wow! Clearly a lot of work went into this. This is awesome, thanks for doing it.

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.
flow found some issues.

ReactCommon/fabric/components/rncore/ViewConfigs.js

analysis-bot

Code analysis results:

shellcheck found some issues.

scripts/generate-rncore.sh

pull-bot · 2019-03-08T02:14:05Z

	Warnings
⚠️	🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against 31c110c

analysis-bot

Code analysis results:

shellcheck found some issues.

scripts/generate-rncore.sh

Libraries/Renderer/shims/ReactNative.js

Libraries/StyleSheet/StyleSheetTypes.js

RNTester/Podfile

ReactCommon/jsi/JSCRuntime.cpp

third-party-podspecs/Folly.podspec

analysis-bot

Code analysis results:

shellcheck found some issues.

scripts/generate-rncore.sh

analysis-bot

Code analysis results:

shellcheck found some issues.

scripts/generate-rncore.sh

analysis-bot

Code analysis results:

shellcheck found some issues.

scripts/generate-rncore.sh

analysis-bot

Code analysis results:

shellcheck found some issues.

scripts/generate-rncore.sh

analysis-bot

Code analysis results:

shellcheck found some issues.

scripts/generate-rncore.sh

analysis-bot

Code analysis results:

shellcheck found some issues.

scripts/generate-rncore.sh

analysis-bot

Code analysis results:

shellcheck found some issues.

scripts/generate-rncore.sh

ericlewis · 2019-03-13T01:01:53Z

The react-native side of things is now hack free! RNTester implements the RCTSurface race condition hack itself, so no need to dirty react native code :)

The podspecs are complete, I am not sure if it is optimal to have a Folly Fabric podspec, but it is easiest without breaking existing users. We can converge once fabric is "done". I think it is fairly safe to import everything except for the changes to RNTester- as they are hacks.

fkgozali

Thanks for the hard work! I put some comments, but at high level:

RNTester Fabric mode needs to be configurable
Let's find out how to not depend on libevent
You probably should split this PR into a few parts: React.xcodeproj changes, pod file changes, then RNTester changes

fkgozali · 2019-03-13T16:51:41Z

package.json

@@ -127,6 +127,7 @@
    "eslint-plugin-react-native": "3.5.0",
    "eslint-plugin-relay": "0.0.28",
    "flow-bin": "^0.94.0",
+    "flow-remove-types": "^1.2.3",


what is this for? is it to run the codegen?

Yes, for codegen.

scripts/generate-rncore.sh

third-party-podspecs/libevent.podspec

fkgozali · 2019-03-13T16:54:35Z

RNTester/RNTester/AppDelegate.m

+}
+@end
+
+// FIXME: this is a hack, remove when surfaces start correctly


Could you open an issue then link that issue here? We'll look into it.

Yes, will do that shortly.

let's put this link in the FIXME comment: https://github.com/facebook/react-native/issues/23910

RNTester/RNTester/AppDelegate.m

analysis-bot

Code analysis results:

shellcheck found some issues.

scripts/generate-rncore.sh

analysis-bot

Code analysis results:

shellcheck found some issues.

scripts/generate-rncore.sh

analysis-bot

Code analysis results:

shellcheck found some issues.

scripts/generate-rncore.sh

fkgozali · 2019-03-13T18:21:38Z

One other note:

I think in general we want to make Fabric (including compiling it) optional at the moment, because if you include it the core React.xcodeproj or React podspec, then every app has to pay the extra cost of having fabric even if they don't use it (they shouldn't use it for now). This means extra app size or latent bugs for them. So let's split it in this PR.

Given that, I think the ideal place I'd like to go to is:

Having fabric-only configuration (in podspec, or separate ReactFabric.xcodeproj, etc) that apps can pull in if they decide to activate fabric
Having these configurations define a compiler flag, e.g. FABRIC_ENABLED, so that apps can choose to activate Fabric surface vs old RootView in their AppDelegate

Now, re: .xcodeproj vs podspec, I'd rather just maintain 1 thing going forward. So if the general community is moving towards CocoaPods, we can just drop support for the fabric in .xcodeproj setup.

Long-long term, I recognize it's tedious to manually "sync" these configurations across BUCK/podspec/gradle, so we can have a separate discussion about either utilizing buck project, or even simple parsing of BUCK files to generate the relevant podspec/gradle files.

fkgozali · 2019-03-13T18:28:00Z

RNTester/RNTesterPods.xcodeproj/project.pbxproj

@@ -17,7 +17,9 @@
 		3D13F84A1D6F6AFD00E69E0E /* OtherImages.xcassets in Resources */ = {isa = PBXBuildFile; fileRef = 3D13F8451D6F6AF200E69E0E /* OtherImages.xcassets */; };


unrelated: I thought we're not supposed to check in .xcodeproj/.xcworkspace file when using cocoapods? should probably remove this in different PR

Generally you check in the xcodeproj but not the xcworkspace when working with pods.

ok sounds good

Fabric requires a lot more stuff than react, lets just setup a diff podspec if you want to work with fabric.

This reverts commit b626ede.

this greatly simplifies the work required to enable fabric and keeps existing podspecs sparkling clean.

Update to RN_FABRIC_ENABLED

And some small formatting

msand · 2019-03-15T00:18:12Z

Great work! (sorry for the noise, just wanted to thank you :)

facebook-github-bot

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

react-native-bot · 2019-03-16T07:05:05Z

This pull request was successfully merged by @ericlewis in 97e6ea1.

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

ericlewis requested a review from fkgozali March 7, 2019 06:45

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 7, 2019

This comment has been minimized.

Sign in to view

react-native-bot added the PR: Includes Changelog label Mar 7, 2019

analysis-bot suggested changes Mar 7, 2019

View reviewed changes

analysis-bot suggested changes Mar 8, 2019

View reviewed changes

scripts/generate-rncore.sh Outdated Show resolved Hide resolved

scripts/generate-rncore.sh Outdated Show resolved Hide resolved

analysis-bot suggested changes Mar 8, 2019

View reviewed changes

scripts/generate-rncore.sh Outdated Show resolved Hide resolved

ericlewis commented Mar 12, 2019

View reviewed changes

analysis-bot suggested changes Mar 12, 2019

View reviewed changes

scripts/generate-rncore.sh Show resolved Hide resolved

analysis-bot suggested changes Mar 12, 2019

View reviewed changes

scripts/generate-rncore.sh Show resolved Hide resolved

analysis-bot suggested changes Mar 12, 2019

View reviewed changes

scripts/generate-rncore.sh Show resolved Hide resolved

analysis-bot suggested changes Mar 12, 2019

View reviewed changes

scripts/generate-rncore.sh Show resolved Hide resolved

analysis-bot suggested changes Mar 13, 2019

View reviewed changes

scripts/generate-rncore.sh Show resolved Hide resolved

analysis-bot suggested changes Mar 13, 2019

View reviewed changes

scripts/generate-rncore.sh Show resolved Hide resolved

analysis-bot suggested changes Mar 13, 2019

View reviewed changes

scripts/generate-rncore.sh Show resolved Hide resolved

ericlewis marked this pull request as ready for review March 13, 2019 01:02

ericlewis requested review from cpojer and hramos as code owners March 13, 2019 01:02

ericlewis mentioned this pull request Mar 13, 2019

Fabric: add podspecs #23802

Closed

ericlewis changed the title ~~Fabric: works in RNTester~~ Fabric: working podspecs & works in RNTester Mar 13, 2019

This comment has been minimized.

Sign in to view

fkgozali requested changes Mar 13, 2019

View reviewed changes

analysis-bot suggested changes Mar 13, 2019

View reviewed changes

scripts/generate-rncore.sh Show resolved Hide resolved

analysis-bot suggested changes Mar 13, 2019

View reviewed changes

scripts/generate-rncore.sh Show resolved Hide resolved

analysis-bot suggested changes Mar 13, 2019

View reviewed changes

scripts/generate-rncore.sh Show resolved Hide resolved

fkgozali reviewed Mar 13, 2019

View reviewed changes

ericlewis added 22 commits March 14, 2019 17:19

Swizzle fabric methods so we don’t crash.

51c6997

remove shim hack, use flag to indicate fabric.

f64646b

Clean-up podspecs

45062d1

use a diff podspec for folly on fabric

cde57e8

Fabric requires a lot more stuff than react, lets just setup a diff podspec if you want to work with fabric.

Fix header search paths

6df82b9

Revert "Use strikethrough instead of line-through"

aec36fd

This reverts commit b626ede.

vastly simpler folly podspec

4ebf5a7

Feature flag fabric, remove enable by default.

c590e9c

Add comment to generate-rncore.sh

d9ba285

create fabric subspec for folly

b511635

this greatly simplifies the work required to enable fabric and keeps existing podspecs sparkling clean.

Update podlock

5d6872f

add copyright header

d345857

Silence warning, names won’t be non-alphanumberic

8a68949

Fix missing run scheme

5279477

update folly podspec

707af1b

Update react-fabric folly dep

3235db2

fix style nit

7eec116

Update fixme to point to issue

9676dfd

Add revisit todos to weakObject implementations

d2671a9

Flag nit

b40483b

Update to RN_FABRIC_ENABLED

JSI: flag fabric specific code

043694a

And some small formatting

Fix header

31c110c

ericlewis force-pushed the fabric-rntester-hacks branch from 2f9977e to 31c110c Compare March 14, 2019 21:20

facebook-github-bot reviewed Mar 15, 2019

View reviewed changes

facebook-github-bot closed this in 97e6ea1 Mar 16, 2019

react-native-bot added the Merged This PR has been merged. label Mar 16, 2019

jtreanor mentioned this pull request Jun 25, 2019

Fix compatibility with CocoaPods frameworks #25393

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fabric: working podspecs & works in RNTester #23803

Fabric: working podspecs & works in RNTester #23803

ericlewis commented Mar 7, 2019 •

edited

Loading

This comment has been minimized.

JoshuaGross commented Mar 7, 2019

analysis-bot left a comment

analysis-bot left a comment

pull-bot commented Mar 8, 2019 •

edited

Loading

analysis-bot left a comment

analysis-bot left a comment

analysis-bot left a comment

analysis-bot left a comment

analysis-bot left a comment

analysis-bot left a comment

analysis-bot left a comment

analysis-bot left a comment

ericlewis commented Mar 13, 2019

This comment has been minimized.

fkgozali left a comment

fkgozali Mar 13, 2019

ericlewis Mar 13, 2019

fkgozali Mar 13, 2019

ericlewis Mar 13, 2019

ericlewis Mar 13, 2019

fkgozali Mar 14, 2019

analysis-bot left a comment

analysis-bot left a comment

analysis-bot left a comment

fkgozali commented Mar 13, 2019

fkgozali Mar 13, 2019

ericlewis Mar 13, 2019

fkgozali Mar 13, 2019

msand commented Mar 15, 2019 •

edited

Loading

facebook-github-bot left a comment

react-native-bot commented Mar 16, 2019

		@@ -17,7 +17,9 @@
		3D13F84A1D6F6AFD00E69E0E /* OtherImages.xcassets in Resources / = {isa = PBXBuildFile; fileRef = 3D13F8451D6F6AF200E69E0E / OtherImages.xcassets */; };

Fabric: working podspecs & works in RNTester #23803

Fabric: working podspecs & works in RNTester #23803

Conversation

ericlewis commented Mar 7, 2019 • edited Loading

Summary

Directions to use

Notes

Changelog

Test Plan

This comment has been minimized.

JoshuaGross commented Mar 7, 2019

analysis-bot left a comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

pull-bot commented Mar 8, 2019 • edited Loading

analysis-bot left a comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

ericlewis commented Mar 13, 2019

This comment has been minimized.

fkgozali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

fkgozali commented Mar 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msand commented Mar 15, 2019 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Mar 16, 2019

ericlewis commented Mar 7, 2019 •

edited

Loading

pull-bot commented Mar 8, 2019 •

edited

Loading

msand commented Mar 15, 2019 •

edited

Loading