-
Notifications
You must be signed in to change notification settings - Fork 45.6k
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
ReactNative flat renderer bundles #9626
Conversation
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.
…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
Can we just remove them from source? RN was the last thing relying on them. |
scripts/rollup/build.js
Outdated
@@ -273,16 +305,33 @@ function getPlugins( | |||
case UMD_PROD: | |||
case NODE_PROD: | |||
case FB_PROD: | |||
case RN_PROD: | |||
plugins.push( | |||
replace(stripEnvVariables(true)), | |||
// needs to happen after strip env | |||
commonjs(getCommonJsConfig(bundleType)), | |||
uglify( | |||
uglifyConfig( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it time to switch to named arguments here?
@@ -6,7 +6,7 @@ | |||
* LICENSE file in the root directory of this source tree. An additional grant | |||
* of patent rights can be found in the PATENTS file in the same directory. | |||
* | |||
* @providesModule ReactErrorUtils | |||
* @providesModule SyntheticEvent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to expose SyntheticEvent?
NativeMethodsMixin: require('NativeMethodsMixin'), | ||
|
||
// Used by react-native-github/Libraries/ components | ||
PooledClass: require('PooledClass'), // Components/Touchable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ReactGlobalSharedState: require('ReactGlobalSharedState'), // Systrace | ||
ReactNativeComponentTree: require('ReactNativeComponentTree'), // InspectorUtils, ScrollResponder | ||
ReactNativePropRegistry: require('ReactNativePropRegistry'), // flattenStyle, Stylesheet | ||
ReactPerf: require('ReactPerf'), // ReactPerfStallHandler, RCTRenderingPerf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we put ReactPerf
and ReactDebugTool
imports under __DEV__
?
I think they are currently only imported from DEV-only modules in RN.
ReactGlobalSharedState: require('ReactGlobalSharedState'), // Systrace | ||
ReactNativeComponentTree: require('ReactNativeComponentTree'), // InspectorUtils, ScrollResponder | ||
ReactNativePropRegistry: require('ReactNativePropRegistry'), // flattenStyle, Stylesheet | ||
ReactPerf: require('ReactPerf'), // ReactPerfStallHandler, RCTRenderingPerf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question about DEV here.
|
||
__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED: { | ||
// Used for Flow types | ||
SyntheticEvent: require('SyntheticEvent'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! 😁
No. Our Jest tests rely on them. |
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. |
Maybe. Would be nice. Could always do in a follow-up :D |
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.
scripts/rollup/build.js
Outdated
@@ -150,6 +150,12 @@ function updateBundleConfig(config, filename, format, bundleType, hasteName) { | |||
}); | |||
} | |||
|
|||
function setUseFiberEnvVariable(useFiber) { | |||
return { | |||
'process.env.ROLLUP_USE_FIBER': useFiber, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have two versions of FeatureFlags, one with Fiber enabled and other with it disabled? And use aliasing to feed it either one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that leave the same problems I was trying to solve by injecting it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't understand. They would still be specified at the build time, just without a need to look at process
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
a12dfc8
to
e97da7f
Compare
affc0bc
to
b158443
Compare
…and PooledClass from SECRET exports. Converted Rollup helper function to use named params.
b158443
to
0553103
Compare
dfc4688
to
9f59c4c
Compare
…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.
…single files to be synced to fbsource
535c543
to
fe00c57
Compare
Let's go over this PR? 😁 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for the review @trueadm ! |
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.
Changes
Rollup changes
@providesModule
headers for RN_* builds.@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.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.PooledClass
to fbsource rather than including it in the RN renderer bundle based on PR feedback.ReactTypes
andReactNativeTypes
files as well to avoid Flow inferring the types (which is super slow / impossible).sync-fbsource
flag added to enable copying the built RN bundle (and shims) to fbsource (cc @trueadm).Flat renderer bundle
__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED
object toReactNative
for objects required by fbsource.NativeRenderer
(the thing returned fromReactFiberReconciler
) out ofReactNativeFiber
and into its own package,ReactNativeFiberRenderer
. (This way the fiber version offindNodeHandle
can accessNativeRenderer.findHostInstance
without having to require all ofReactNativeFiber
.)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 thatReactNative
be loaded before findNodeHandle can be guaranteed to be functional.takeSnapshot
uses new forkedfindNodeHandler
wrappers (rather thanReactNative.findNodeHandle
) and avoids requiringReactNative
directly.ReactTypes
andReactNativeTypes
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
NativeMethodsMixin
andReactNativeFiberHostComponent
to correctly share the same underlying Flow type between them.