Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fork React Native renderer into FB and OSS bundles #12625

Merged
merged 18 commits into from Apr 18, 2018

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Apr 16, 2018

Fork React Native renderer bundle into separate OSS and FB builds. This is being done in order to minimize unintentional open source impact (e.g. facebook/react-native/issues/18175) and to provide a way for Facebook to test features in RN early, as we do for React DOM.

High level changes

  • Added new "native-fb" and "native-fabric-fb" bundles.
  • Split RN_DEV and RN_PROD bundle types into RN_OSS_DEV, RN_OSS_PROD, RN_FB_DEV, and RN_FB_PROD. (This is a bit redundant but it seemed the least intrusive way of supporting a forked feature flags file for these bundles.)
  • Renamed FB_DEV and FB_PROD bundle types to be more explicitly for www (FB_WWW_DEV and FB_WWW_PROD)
  • Removed Haste @providesModule headers from the RB-specific RN renderer bundles to avoid a duplicate name conflicts.
  • Remove dynamic values from OSS RN feature flags. (Leave them in FB RN feature flags.)
  • Updated the sync script(s) to account for new renderer type.
  • Move ReactFeatureFlags.js shim to FB bundle only (since OSS bundle no longer needs dynamic values).

This is the new structure of the built RN bundles:

./build/react-native/
├── fb    # Used within FB; not synced to GitHub
│   ├── ReactFabric-dev.js
│   ├── ReactFabric-prod.js
│   ├── ReactFeatureFlags.js
│   ├── ReactNativeRenderer-dev.js
│   └── ReactNativeRenderer-prod.js
├── oss   # Ignored within FB; synced to GitHub
│   ├── ReactFabric-dev.js
│   ├── ReactFabric-prod.js
│   ├── ReactNativeRenderer-dev.js
│   └── ReactNativeRenderer-prod.js
└── shims # Shared between fb and oss renderers
    ├── NativeMethodsMixin.js
    ├── ReactDebugTool.js
    ├── ReactFabric.js
    ├── ReactNative.js
    ├── ReactNativeComponentTree.js
    ├── ReactNativeTypes.js
    ├── ReactNativeViewConfigRegistry.js
    ├── ReactPerf.js
    ├── ReactTypes.js
    └── createReactNativeComponentClass.js

Alternatives considered

  • Rather than creating a new bundle type, I also investigated adding the existing FB_DEV / FB_PROD types to the "react-native" bundle, but this bundle type seems to have some www-specific assumptions built in. It seemed safer to add a new type ("XPLAT_*") that could more closely resemble the current RN bundle while still giving us the ability to fork the feature flags file.
  • Keep a single renderer but fork the feature flags file internally. I tested how this would impact the RN prod OSS build size, and it looks like enabling performance timings in prod would increase the gzipped production build of RN by ~5.4% (and also probably impact TTI, although I don't know by how much). This particular change is something that we may want to experiment with (based on a chat with Spencer) so it seemed like a reasonable test case.

Testing

I build RN locally and synced to fbsource by running:

yarn build -- native --sync-fbsource=~/fbsource/

I compared the diff of the resulting bundles and verified they have only the changes I would expect:

  • The open source RN renderer no longer includes the dynamic feature flags require (require("ReactFeatureFlags"))
  • The FB/xplat specific renderer does.

TODO

  • Blacklist the OSS RN renderer @providesModule within fbsource so Facebook is not using that build.
  • Update Facebook's GitHub sync bot to blacklist the FB-specific renderer directory (so it doesn't get copied to RN GitHub).
  • Sync and test the new FB renderer type.

@bvaughn bvaughn changed the title [WIP] Added FB type (and feature flags) to React Native bundle [WIP] Fork React Native bundle and features flags for fbsource and OSS Apr 16, 2018
@pull-bot
Copy link

pull-bot commented Apr 16, 2018

Details of bundled changes.

Comparing: 039695c...d4baace

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
React-dev.js n/a n/a 0 B 45.8 KB 0 B 12.48 KB FB_WWW_DEV
React-prod.js n/a n/a 0 B 13.43 KB 0 B 3.73 KB FB_WWW_PROD

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactDOM-dev.js n/a n/a 0 B 618.04 KB 0 B 139.01 KB FB_WWW_DEV
ReactDOM-prod.js n/a n/a 0 B 284.94 KB 0 B 52.3 KB FB_WWW_PROD
ReactTestUtils-dev.js n/a n/a 0 B 36.89 KB 0 B 10.46 KB FB_WWW_DEV
ReactDOMUnstableNativeDependencies-dev.js n/a n/a 0 B 57.09 KB 0 B 14.56 KB FB_WWW_DEV
ReactDOMUnstableNativeDependencies-prod.js n/a n/a 0 B 26.34 KB 0 B 5.38 KB FB_WWW_PROD
ReactDOMServer-dev.js n/a n/a 0 B 94.19 KB 0 B 24.05 KB FB_WWW_DEV
ReactDOMServer-prod.js n/a n/a 0 B 31.62 KB 0 B 7.78 KB FB_WWW_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactART-dev.js n/a n/a 0 B 346.44 KB 0 B 70.26 KB FB_WWW_DEV
ReactART-prod.js n/a n/a 0 B 167.61 KB 0 B 27.75 KB FB_WWW_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js n/a n/a 0 B 456.74 KB 0 B 97.41 KB RN_FB_DEV
ReactNativeRenderer-prod.js n/a n/a 0 B 218.26 KB 0 B 36.66 KB RN_FB_PROD
ReactNativeRenderer-dev.js n/a n/a 0 B 456.49 KB 0 B 97.35 KB RN_OSS_DEV
ReactNativeRenderer-prod.js n/a n/a 0 B 215.01 KB 0 B 36.29 KB RN_OSS_PROD
ReactFabric-dev.js n/a n/a 0 B 439.16 KB 0 B 92.97 KB RN_FB_DEV
ReactFabric-prod.js n/a n/a 0 B 200.59 KB 0 B 33.55 KB RN_FB_PROD
ReactFabric-dev.js n/a n/a 0 B 439.19 KB 0 B 92.99 KB RN_OSS_DEV
ReactFabric-prod.js n/a n/a 0 B 200.63 KB 0 B 33.57 KB RN_OSS_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactTestRenderer-dev.js n/a n/a 0 B 345.1 KB 0 B 69.35 KB FB_WWW_DEV
ReactShallowRenderer-dev.js n/a n/a 0 B 14.41 KB 0 B 3.55 KB FB_WWW_DEV

react-is

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactIs-dev.js n/a n/a 0 B 4.16 KB 0 B 1.19 KB FB_WWW_DEV
ReactIs-prod.js n/a n/a 0 B 3.33 KB 0 B 953 B FB_WWW_PROD

Generated by 🚫 dangerJS

export const enablePersistentReconciler = false;
export const enableUserTimingAPI = __DEV__;
export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
export const warnAboutDeprecatedLifecycles = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this is turned off for open source builds.

@@ -45,7 +47,15 @@ function getBundleOutputPaths(bundleType, filename, packageName) {
case RN_PROD:
switch (packageName) {
case 'react-native-renderer':
return [`build/react-native/${filename}`];
return [`build/react-native/oss/${filename}`];
Copy link
Contributor Author

@bvaughn bvaughn Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things in the "oss" sub directory are blacklisted from FB configs in order to prevent Haste module conflicts. This added subdirectory should not impact open source builds though.


import typeof * as FeatureFlagsType from 'shared/ReactFeatureFlags';
import typeof * as FeatureFlagsShimType from './ReactFeatureFlags.native-oss';

Copy link
Contributor Author

@bvaughn bvaughn Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note open source builds no longer depend on dynamic flags.

}

await Packaging.copyAllShims();
await Packaging.prepareNpmPackages();

if (syncFBSourcePath) {
await Sync.syncReactNative('build/react-native', syncFBSourcePath);
await Sync.syncReactNativeRT('build/react-rt', syncFBSourcePath);
await Sync.syncReactNativeCS('build/react-cs', syncFBSourcePath);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The react-rt/react-cs commands were no longer being used.

@bvaughn bvaughn changed the title [WIP] Fork React Native bundle and features flags for fbsource and OSS Fork React Native renderer into FB and OSS bundles Apr 18, 2018
@gaearon
Copy link
Collaborator

gaearon commented Apr 18, 2018

I also investigated adding the existing FB_DEV / FB_PROD types to the "react-native" bundle, but this bundle type seems to have some www-specific assumptions built in

We should rename it to FB_WWW_* or something like this, it was always meant to be www-specific but wording didn't reflect that.

case RN_FB_PROD:
return 'shared/forks/ReactFeatureFlags.native-fb.js';
default:
return 'shared/forks/ReactFeatureFlags.native-oss.js';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use specific cases and throw for default?

case RN_FB_PROD:
return 'shared/forks/ReactFeatureFlags.native-fabric-fb.js';
default:
return 'shared/forks/ReactFeatureFlags.native-fabric-oss.js';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same

@bvaughn
Copy link
Contributor Author

bvaughn commented Apr 18, 2018

We should rename it to FB_WWW_* or something like this, it was always meant to be www-specific but wording didn't reflect that.

Yeah, good call. I almost did that earlier but was reluctant to introduce more churn. Done!

@bvaughn bvaughn merged commit 0887c7d into facebook:master Apr 18, 2018
@bvaughn bvaughn deleted the fb-rn-bundle branch April 18, 2018 20:16
rhagigi pushed a commit to rhagigi/react that referenced this pull request Apr 19, 2018
* Added new "native-fb" and "native-fabric-fb" bundles.
* Split RN_DEV and RN_PROD bundle types into RN_OSS_DEV, RN_OSS_PROD, RN_FB_DEV, and RN_FB_PROD. (This is a bit redundant but it seemed the least intrusive way of supporting a forked feature flags file for these bundles.)
* Renamed FB_DEV and FB_PROD bundle types to be more explicitly for www (FB_WWW_DEV and FB_WWW_PROD)
* Removed Haste @providesModule headers from the RB-specific RN renderer bundles to avoid a duplicate name conflicts.
* Remove dynamic values from OSS RN feature flags. (Leave them in FB RN feature flags.)
* Updated the sync script(s) to account for new renderer type.
* Move ReactFeatureFlags.js shim to FB bundle only (since OSS bundle no longer needs dynamic values).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants