ReactNative flat renderer bundles by bvaughn · Pull Request #9626 · react/react

bvaughn · 2017-05-08T23:12:49Z

Introduces ReactNative flat renderer bundles.

There's more that could be done to improve consistency and sharing between the www and fbsource sync scripts. Given the size and duration of this PR I'd prefer to defer that work to a follow-up issue, #9763.

Building and syncing ReactNative flat renderer

This PR introduces an optional flag to the Rollup build scripts that enables syncing the built ReactNative renderer (and its Flow types and shims) to fbsource.

# Build ReactNative flat bundles
yarn build -- --type=RN

# Build bundles and sync (to default location)
yarn build -- --type=RN --sync-fbsource

# Build bundles and sync (to custom location)
yarn build -- --type=RN --sync-fbsource=/path/to/fbsource/

Changes

Rollup changes

Updated the Rollup build script to strip Haste @providesModule headers for RN_* builds.
Updated Rollup build script to add @noflow header to flat bundle to tell Flow not to try to infer types. (We'll have to use explicit types instead.) This is necessary to avoid causing huge memory and CPU uses in Flow.
Added a new REACT_NATIVE_USE_FIBER env variable to prevent a mismatch of stack+fiber components at runtime.
- ReactNativeFeatureFlags uses this env variable to enable tests to cover both versions.
- The Rollup build script uses this variable as well to support proper bundles.
Packaging script copies PooledClass to fbsource rather than including it in the RN renderer bundle based on PR feedback.
Packaging script copies ReactTypes and ReactNativeTypes files as well to avoid Flow inferring the types (which is super slow / impossible).
New sync-fbsource flag added to enable copying the built RN bundle (and shims) to fbsource (cc @trueadm).

Flat renderer bundle

Added __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED object to ReactNative for objects required by fbsource.
Added shims for modules directly required in fbsource that read from the new secret property.
Re-organized code to avoid circular dependencies that would break Rollup:
- Move NativeRenderer (the thing returned from ReactFiberReconciler) out of ReactNativeFiber and into its own package, ReactNativeFiberRenderer. (This way the fiber version of findNodeHandle can access NativeRenderer.findHostInstance without having to require all of ReactNativeFiber.)
- Fork findNodeHandle internally (based on feature flag) rather than relying on injecting. (This is only temporary, until stack has been deprecated.) This will ease the requirement that ReactNative be loaded before findNodeHandle can be guaranteed to be functional.
- takeSnapshot uses new forked findNodeHandler wrappers (rather than ReactNative.findNodeHandle) and avoids requiring ReactNative directly.
Collocated externally exposed ReactTypes and ReactNativeTypes into single files to be synced to fbsource automatically since types can no longer be inferred. (This mostly involved moving a few types around and changing their imports.) These types are used internally as well to ensure they stay in sync with the implementations.

Miscellaneous

Addressed a TODO in NativeMethodsMixin and ReactNativeFiberHostComponent to correctly share the same underlying Flow type between them.

Hopefully this is sufficient to work around Rollup circular dependency problems. (To be seen in subsequent commits...)

This allowed me to remove the ReactNative -> findNodeHandle injections, which should in turn allow me to require a fully-functional findNodeHandle without going through ReactNative. This will hopefully allow ReactNativeBaseomponent to avoid a circular dependency.

…ion impl

…ndle Instead it uses the new, renderer-specific wrappers (eg findNodeHandleFiberWrapper and findNodeHandleStackWrapper) to ensure the returned value is numeric (or null). This avoids a circular dependency that would trip up Rollup.

…r than ReactNative This works around a potential circular dependency that would break the Rollup build

gaearon · 2017-05-09T10:26:27Z

Updated the Rollup builds script to strip Haste @providesModule headers for RN_* builds. (Should we do this for FB_* builds as well?)

Can we just remove them from source? RN was the last thing relying on them.

gaearon · 2017-05-09T10:27:48Z

        // needs to happen after strip env
        commonjs(getCommonJsConfig(bundleType)),
        uglify(
          uglifyConfig(


Is it time to switch to named arguments here?

gaearon · 2017-05-09T10:28:57Z

 * of patent rights can be found in the PATENTS file in the same directory.
 *
- * @providesModule ReactErrorUtils
+ * @providesModule SyntheticEvent


Why do we need to expose SyntheticEvent?

gaearon · 2017-05-09T10:30:07Z

+    NativeMethodsMixin: require('NativeMethodsMixin'),
+
+    // Used by react-native-github/Libraries/ components
+    PooledClass: require('PooledClass'), // Components/Touchable


Can we copy and paste PooledClass into RN repo? It is relatively small and completely isolated.
I'd like to avoid treating it as part of React's contract, and be able to safely change or remove it.

gaearon · 2017-05-09T10:32:33Z

+    ReactGlobalSharedState: require('ReactGlobalSharedState'), // Systrace
+    ReactNativeComponentTree: require('ReactNativeComponentTree'), // InspectorUtils, ScrollResponder
+    ReactNativePropRegistry: require('ReactNativePropRegistry'), // flattenStyle, Stylesheet
+    ReactPerf: require('ReactPerf'), // ReactPerfStallHandler, RCTRenderingPerf


Should we put ReactPerf and ReactDebugTool imports under __DEV__?
I think they are currently only imported from DEV-only modules in RN.

gaearon · 2017-05-09T10:33:02Z

+    ReactGlobalSharedState: require('ReactGlobalSharedState'), // Systrace
+    ReactNativeComponentTree: require('ReactNativeComponentTree'), // InspectorUtils, ScrollResponder
+    ReactNativePropRegistry: require('ReactNativePropRegistry'), // flattenStyle, Stylesheet
+    ReactPerf: require('ReactPerf'), // ReactPerfStallHandler, RCTRenderingPerf


Same question about DEV here.

gaearon · 2017-05-09T10:33:24Z

+
+  __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: {
+    // Used for Flow types
+    SyntheticEvent: require('SyntheticEvent'),


I think this is wrong. You don't need to expose SyntheticEvent for Flow types. SyntheticEvent is a global in Flow (yes, Flow can be weird).

Yup, this is on the Quip doc checklist. Initially I saw SE was being required a dozen or so places internally but then realized it was a built-in type and just haven't ticked off that item yet. Thanks for pointing it out though! 😁

bvaughn · 2017-05-09T15:41:16Z

Updated the Rollup builds script to strip Haste @providesModule headers for RN_* builds. (Should we do this for FB_* builds as well?)

Can we just remove them from source? RN was the last thing relying on them.

No. Our Jest tests rely on them.

gaearon · 2017-05-09T15:59:01Z

Oh. I wonder if there's any way to tell Jest to just use filenames? I thought that was the plan for www—curious if Jest already supports it via some obscure switch.

bvaughn · 2017-05-09T16:01:43Z

Maybe. Would be nice. Could always do in a follow-up :D

gaearon · 2017-05-09T16:02:35Z

Sounds good. 😄

This shouldn't have been declared as an external. This was causing it to be inserted as a side-effects require before RN was exported, causing a circular dep that broke the bundle.

gaearon · 2017-05-11T17:08:59Z


+function setUseFiberEnvVariable(useFiber) {
+  return {
+    'process.env.ROLLUP_USE_FIBER': useFiber,


Could we have two versions of FeatureFlags, one with Fiber enabled and other with it disabled? And use aliasing to feed it either one.

Wouldn't that leave the same problems I was trying to solve by injecting it?

Hmm, I don't understand. They would still be specified at the build time, just without a need to look at process.

Oh. Maybe I misunderstood what you were suggesting them (and I actually don't know after all). Could you give a more concrete example?

This is kind of a hacky solution, but it is temporary. It works around the fact that ReactNativeFeatureFlag values need to be set at build time in order to avoid a mismatch between runtime flag values. DOM avoids the need to do this by using injection but Native is not able to use this same approach due to circular dependency issues.

…and PooledClass from SECRET exports. Converted Rollup helper function to use named params.

…roblems When Flow tries to infer such a large file, it consumes massive amounts of CPU/RAM and can often lead to programs crashing. It is better for such large files to use .flow.js types instead.

…me Flow type

…single files to be synced to fbsource

bvaughn · 2017-05-24T11:45:46Z

Let's go over this PR? 😁

trueadm

LGTM

bvaughn · 2017-05-24T16:06:41Z

Thanks for the review @trueadm !

@noflow

* Split ReactNativeFiber into separate ReactNativeFiberRenderer module Hopefully this is sufficient to work around Rollup circular dependency problems. (To be seen in subsequent commits...) * Split findNodeHandle into findNodeHandleFiber + findNodeHandleStack This allowed me to remove the ReactNative -> findNodeHandle injections, which should in turn allow me to require a fully-functional findNodeHandle without going through ReactNative. This will hopefully allow ReactNativeBaseomponent to avoid a circular dependency. * Un-forked findNodeHandle in favor of just inlining the findNode function impl * takeSnapshot no longer requires/depends-on ReactNative for findNodeHandle Instead it uses the new, renderer-specific wrappers (eg findNodeHandleFiberWrapper and findNodeHandleStackWrapper) to ensure the returned value is numeric (or null). This avoids a circular dependency that would trip up Rollup. * NativeMethodsMixin requires findNodeHandler wrapper(s) directly rather than ReactNative This works around a potential circular dependency that would break the Rollup build * Add RN_* build targets to hash-finle-name check * Strip @providesModule annotations from headers for RN_* builds * Added process.env.REACT_NATIVE_USE_FIBER to ReactNativeFeatureFlags This is kind of a hacky solution, but it is temporary. It works around the fact that ReactNativeFeatureFlag values need to be set at build time in order to avoid a mismatch between runtime flag values. DOM avoids the need to do this by using injection but Native is not able to use this same approach due to circular dependency issues. * Moved a couple of SECRET exports to dev-only. Removed SyntheticEvent and PooledClass from SECRET exports. Converted Rollup helper function to use named params. * Split NativeMethodsMixins interface and object-type * Add @noflow header to flat-bundle template to avoid triggering Flow problems When Flow tries to infer such a large file, it consumes massive amounts of CPU/RAM and can often lead to programs crashing. It is better for such large files to use .flow.js types instead. * NativeMethodsMixin and ReactNativeFiberHostComponent now share the same Flow type * Collocated (externally exposed) ReactTypes and ReactNativeTypes into single files to be synced to fbsource. ReactNativeFiber and ReactNativeStack use ReactNativeType Flow type * Build script syncs RN types and PooledClass automatically * Added optional sync-RN step to Rollup build script * Added results.json for new RN bundles

Brian Vaughn added 12 commits May 5, 2017 10:32

Stashing

11b01fa

Stashing

4d1d517

Split ReactNativeFiber into separate ReactNativeFiberRenderer module

3c4ead6

Hopefully this is sufficient to work around Rollup circular dependency problems. (To be seen in subsequent commits...)

Un-forked findNodeHandle in favor of just inlining the findNode funct…

e65a4a3

…ion impl

NativeMethodsMixin requires findNodeHandler wrapper(s) directly rathe…

21337e4

…r than ReactNative This works around a potential circular dependency that would break the Rollup build

Re-added SyntheticEvent shim to ReactNative renderers

7d1e82f

Reverted changes to results.json

e1d683d

Renamed findNodeHandler stack/fiber RN wrapper

50aabf6

Add RN_* build targets to hash-finle-name check

94a5631

Strip @providesModule annotations from headers for RN_* builds

a12bd55

facebook-github-bot added the CLA Signed label May 8, 2017

gaearon reviewed May 9, 2017

View reviewed changes

Brian Vaughn added 3 commits May 10, 2017 11:10

Merge branch 'master' into react-native-flat-bundles

1f56050

Prettier and lint

89f159f

Removed createReactNativeComponentClass from RN bundle externals

fb6f494

This shouldn't have been declared as an external. This was causing it to be inserted as a side-effects require before RN was exported, causing a circular dep that broke the bundle.

gaearon reviewed May 11, 2017

View reviewed changes

bvaughn force-pushed the react-native-flat-bundles branch from a12dfc8 to e97da7f Compare May 11, 2017 17:09

Merge branch 'master' into react-native-flat-bundles

7533d38

bvaughn force-pushed the react-native-flat-bundles branch from affc0bc to b158443 Compare May 12, 2017 21:29

Moved a couple of SECRET exports to dev-only. Removed SyntheticEvent …

0553103

…and PooledClass from SECRET exports. Converted Rollup helper function to use named params.

bvaughn force-pushed the react-native-flat-bundles branch from b158443 to 0553103 Compare May 12, 2017 21:40

Split NativeMethodsMixins interface and object-type

9f59c4c

bvaughn force-pushed the react-native-flat-bundles branch from dfc4688 to 9f59c4c Compare May 23, 2017 16:04

Brian Vaughn added 3 commits May 23, 2017 22:25

Add @noflow header to flat-bundle template to avoid triggering Flow p…

4540e2c

…roblems When Flow tries to infer such a large file, it consumes massive amounts of CPU/RAM and can often lead to programs crashing. It is better for such large files to use .flow.js types instead.

NativeMethodsMixin and ReactNativeFiberHostComponent now share the sa…

c002642

…me Flow type

Collocated (externally exposed) ReactTypes and ReactNativeTypes into …

fe00c57

…single files to be synced to fbsource

bvaughn force-pushed the react-native-flat-bundles branch from 535c543 to fe00c57 Compare May 24, 2017 10:20

Brian Vaughn added 4 commits May 24, 2017 11:24

Build script syncs RN types and PooledClass automatically

bddfdf3

Added optional sync-RN step to Rollup build script

a9125b7

Merged master

5d844c5

Added results.json for new RN bundles

37920d7

bvaughn requested review from acdlite, gaearon and trueadm May 24, 2017 11:45

bvaughn changed the title ~~React native flat bundles [WIP]~~ React native flat bundles May 24, 2017

Brian Vaughn added 2 commits May 24, 2017 13:55

ReactNativeFiber and ReactNativeStack use ReactNativeType Flow type

22afd0e

RN sync script now accepts fbsource path and logs

d34f9a7

bvaughn changed the title ~~React native flat bundles~~ ReactNative flat renderer bundles May 24, 2017

bvaughn mentioned this pull request May 24, 2017

Improve React and ReactNative sync scripts #9763

Closed

6 tasks

Brian Vaughn added 2 commits May 24, 2017 14:34

Added RN feature flags shim

ff597d5

Added ReactNative shim

e9aad99

trueadm approved these changes May 24, 2017

View reviewed changes

bvaughn merged commit c22b94f into react:master May 24, 2017

bvaughn deleted the react-native-flat-bundles branch May 24, 2017 16:06

Conversation

bvaughn commented May 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Building and syncing ReactNative flat renderer

Changes

Rollup changes

Flat renderer bundle

Miscellaneous

Uh oh!

gaearon commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented May 9, 2017

Uh oh!

gaearon commented May 9, 2017

Uh oh!

bvaughn commented May 9, 2017

Uh oh!

gaearon commented May 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented May 24, 2017

Uh oh!

trueadm left a comment

Choose a reason for hiding this comment

Uh oh!

bvaughn commented May 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bvaughn commented May 8, 2017 •

edited

Loading

gaearon commented May 9, 2017 •

edited

Loading