ReactNative fiber renderer #8560

bvaughn · 2016-12-12T23:10:02Z

This PR adds a new, fiber-based renderer for native dubbed ReactNativeFiber. Existing native renderer has been renamed to ReactNativeStack and a new flags file ReactNativeFeatureFlags has been added to switch between the 2 renderers. (Other than injecting a findNodeHandle strategy ReactNativeStack has not been modified.)

I've done some basic smoke-testings with Android and iOS and the fiber renderer seems to work well with the following applications: Catalyst, Ads Manager, and Facebook "Events" tab.

I have also added/enhanced Flow types for UIManager with this PR. If it's desirable for that work to be split into a separate PR, let me know and I can break it off.

Known issues

The inspector does not work with the Fiber-based renderer. The react-native InspectorUtils.getOwnerHierarchy reaches into stack internals in a way that is not Fiber-compatible and causes a runtime error.
Testing various internal apps yields the following:
- Facebook app Events page seems fine.
- Catalyst seems mostly fine, but errors with "Unexpected context pop" when I open the "App - UIExplorer Browser" component.
- Ads Manager errors with "Unexpected context pop" on some pages.

Open question: How should we handle children?

ReactNativeFiber currently creates wrapper objects to bundle each native tag with its children and viewConfig. This creates one wrapper object per native view which may be undesirable. We could remove the wrapper object but we would need to pass some additional information to the HostConfig methods:

~~Pass type to the following methods: commitUpdate. (This is necessary to get the viewConfig for validAttributes and uiViewClassName.)~~ Update Already done via PR Drop runtime validation and lower case coercion of tag names #8563. However viewConfig can't be removed for the time being because NativeMethodsMixin depends on it.
Pass children to the following methods: appendChild, finalizeInitialChildren, insertBefore, removeChild. (This is necessary to add/set/move/remove children.)
Provide a hook to recursively call remove children (so that we can uncache all child nodes). (Without this the RCTUIManager errors with "Invalid view set to be the JS responder".)

As for tracking the children array there are 2 options to consider:

Option 1: Manage index information in Fiber

Fiber has enough information to pass the additional params to the HostConfig but it would need to calculate and/or store the child information in order to do so. The benefit provided to ReactNativeFiber may be outweighed by Fiber having to do this for other renderers that don't require the information.

Option 2: Update native bridge

A better solution to this issue might be to change the iOS and Android bridges to move away from the index-based add/remove methods.

For example, iOS provides methods that could be used instead of our current index-based ones:

addSubview for appendChild
insertSubview(_:aboveSubview:) for insertBefore
removeFromSuperview for removeChild

This would also need to be supported on the Android side, perhaps with the following methods:

addView for appendChild
View getZ and setZ methods for insertBefore (should verify with someone more familiar with Android)
removeView for removeChild

edvinerikson

Looks good! I guess we need a way to cleanup native views.
https://twitter.com/sebmarkbage/status/799027829073727489

edvinerikson · 2016-12-13T07:29:41Z

src/renderers/native/ReactNativeViewConfigRegistry.js

+
+const viewConfigs = new Map();
+
+const prefix = 'topsecret-';


I guess this will bloat devtools a little bit if all host components are prefixed with this?

yeah we should figure out what we actually want to do here. I think this can land first though.

edvinerikson · 2016-12-13T07:32:57Z

src/renderers/native/ReactNativeFiber.js

+  scheduleDeferredCallback: window.requestIdleCallback,
+
+  shouldSetTextContent(props : Props) : boolean {
+    return false;


Haven't checked the Fiber code for a while. Does this make it so that all text get it's own instance? <Text>Hej</Text> Did not get a text instance before since it's a single child.

I don't actually know enough to answer that question. (Maybe someone who knows more about Fiber than me can chime in?)

Seems to be the case actually. Kind of handy since you don't have to handle that special case in createInstance/commitUpdate.

https://github.com/facebook/react/blob/master/src/renderers/shared/fiber/ReactFiberBeginWork.js#L228

Yeah, let's change this so we have fewer views created.

Actually I think this is correct as-is. Returning true from this method will create an RCText view with no visible text. Looking at the underlying react-native source, at least on the Java side, I think it is expected that such a text view should contain FlatTextShadowNode children.

If I update shouldSetTextContent to behave more like the DOM fiber renderer and return true for string/number children, then the apps I've been testing lose most of their visible text.

I believe that the current behavior (in this PR) of creating both RCTRawText and RCText views matches native stack behavior (eg <Text>string</Text> creates an RCTRawText view for the inner "string").

Please correct if you feel I'm mistaken here. I'm new so it's quite possible. 😄

Yeah I am not sure how much overhead the fiber and child work have in this case either. It felt kind of awkward implementing it in my PR since it was basically duplicating the createTextInstance method.

Agreed. From a renderer's perspective, this is something that could be made simpler to implement. Maybe it will change.

How about for this PR, I'll add a TODO comment to revisit this specific area? 😄

Sounds good to me 😊

If we wanted to avoid the extra Fiber then Fiber itself could have this logic to call createTextInstance directly. I don't think it would need to live in the renderer.

Aye, that's what I was thinking by

From a renderer's perspective, this is something that could be made simpler to implement.

edvinerikson · 2016-12-13T07:33:58Z

src/renderers/native/ReactNativeFiber.js

+    // Noop?
+  },
+
+  scheduleAnimationCallback: window.requestAnimationFrame,


Maybe it should be global instead of window (probably doesn't matter)?

edvinerikson · 2016-12-13T07:36:36Z

src/renderers/native/ReactNativeFiber.js

+      UIManager.manageChildren(
+        parentInstance._nativeTag, // containerTag
+        [], // moveFromIndices
+        [], // moveToIndices


If you are tired of forgetting the arguments I added flowtypings for this function in my PR https://github.com/facebook/react/pull/8361/files#diff-0480067610415f2abfec7a69ca45997eR37

Great suggestion. I will update the flow types in this PR as well.

edvinerikson · 2016-12-13T07:46:05Z

Also in case you missed it, there has been some discussions in #8361

sophiebits

I think this is good and we can land it – just a few inlines.

sophiebits · 2016-12-14T19:45:53Z

src/renderers/native/ReactNativeComponentTree.js

 function uncacheNode(inst) {
-  var tag = inst._rootNodeID;
+  // TODO (bvaughn) Clean up once Stack is deprecated


Don't you always know at the caller whether this is stack or Fiber? If you're using this function still, let's split it up into two.

Good catch. Yeah, this change can be reverted.

sophiebits · 2016-12-14T19:47:43Z

src/renderers/native/ReactNativeFiber.js

+
+type Container = number;
+type Props = Object;
+type Instance = any;


Can you add a real type here and try to get rid of uses of any?

sophiebits · 2016-12-14T19:49:56Z

src/renderers/native/ReactNativeFiber.js

+    child : Instance | TextInstance,
+    beforeChild : Instance | TextInstance
+  ) : void {
+    const children = (parentInstance : any)._children;


This is unsound if it ever is the container. Can we at least throw intentionally so it's clearly unimplemented?

sophiebits · 2016-12-14T19:50:22Z

src/renderers/native/ReactNativeFiber.js

+  },
+
+  prepareForCommit() : void {
+    // Noop?


Yes, this corresponds to the wrappers in ReactNativeReconcileTransaction and should be a noop.

sophiebits · 2016-12-14T19:52:03Z

src/renderers/native/ReactNativeStack.js

+var ReactNativeComponentTree = require('ReactNativeComponentTree');
+var ReactNativeInjection = require('ReactNativeInjection');
+var ReactNativeStackInjection = require('ReactNativeStackInjection');
+


extra blank line?

This was also git mved from the pre-existing ReactNative.js but I'll delete the extra line! 😄

sophiebits · 2016-12-14T19:52:10Z

src/renderers/native/ReactNativeStack.js

+ */
+'use strict';
+
+// Require ReactNativeDefaultInjection first for its side effects of setting up


inaccurate comment (you don't require it first, you don't require any module with that name even, and requiring it is no longer side-effectful because of the explicit .inject())

This was git mved from the pre-existing ReactNative.js. The only modifications to this file was the @providesModule name and the 6-line findNodeHandle.injection calls.

I'll delete the comment though.

sophiebits · 2016-12-14T19:53:08Z

src/renderers/native/ReactNativeViewConfigRegistry.js

+
+const viewConfigs = new Map();
+
+const prefix = 'topsecret-';


yeah we should figure out what we actually want to do here. I think this can land first though.

sophiebits · 2016-12-14T19:55:22Z

src/renderers/native/ReactNative.js

-}
-
-module.exports = ReactNative;
+module.exports = ReactNativeFeatureFlags.useFiber


For now let's commit this as always exporting Stack so we can sync to RN and not bundle Fiber accidentally. You can make a mock for it like src/renderers/dom/mocks/ReactDOM.js and switch on it in scripts/jest/test-framework-setup.js so we get jest tests for it though.

sophiebits · 2016-12-14T20:34:55Z

src/renderers/native/NativeMethodsMixin.js

      this.viewConfig.uiViewClassName,
-      updatePayload
+      (updatePayload : any)


These casts smell funny to me. .create() returns ?Object, while updateView returns Object. Can you figure out if one of them is wrong or if we should call .updateView only conditionally? Same with createView elsewhere.

Hmm. I didn't modify this code other than to add the type-cast. But you're right. It looks fishy.

ReactNativeAttributePayload.create may return null if no props have changed. So that return type is correct.

The Java implementation of updateView doesn't specify a @Nullable annotation for the incoming payload param, but the underlying UIImplementation class does check for null before doing anything with them.

~~I think the right thing to do here though, regardless, is to check for non-null before calling. I'll do that.~~

Actually I think the right thing to do is to change the flow type to annotate the property as nullable. Because the native implementation handles nullable props for both createView andupdateView functions.

sophiebits · 2016-12-14T23:38:32Z

Would be nice to get rid of the viewConfig eventually.

facebook-github-bot added the GH Review: review-needed label Dec 12, 2016

bvaughn force-pushed the react-native-fiber branch from c80ff3d to 3d11b18 Compare December 12, 2016 23:43

edvinerikson reviewed Dec 13, 2016

View reviewed changes

bvaughn force-pushed the react-native-fiber branch 2 times, most recently from 182d45b to abacd2f Compare December 14, 2016 06:27

bvaughn assigned sophiebits Dec 14, 2016

sebmarkbage mentioned this pull request Dec 14, 2016

[Fiber] Umbrella for remaining features / bugs #7925

Closed

sophiebits requested changes Dec 14, 2016

View reviewed changes

sophiebits reviewed Dec 14, 2016

View reviewed changes

bvaughn force-pushed the react-native-fiber branch from c4b089b to 915264c Compare December 14, 2016 21:49

sophiebits approved these changes Dec 14, 2016

View reviewed changes

Added new fiber-based renderer for native dubbed ReactNativeFiber

5266ca9

bvaughn force-pushed the react-native-fiber branch from b5d9f3c to 5266ca9 Compare December 15, 2016 02:42

bvaughn merged commit ab72f62 into facebook:master Dec 15, 2016

bvaughn deleted the react-native-fiber branch December 15, 2016 04:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ReactNative fiber renderer #8560

ReactNative fiber renderer #8560

bvaughn commented Dec 12, 2016 •

edited

Loading

edvinerikson left a comment

edvinerikson Dec 13, 2016

sophiebits Dec 14, 2016

edvinerikson Dec 13, 2016 •

edited

Loading

bvaughn Dec 13, 2016

edvinerikson Dec 13, 2016

sophiebits Dec 13, 2016

bvaughn Dec 14, 2016 •

edited

Loading

edvinerikson Dec 14, 2016

bvaughn Dec 14, 2016

edvinerikson Dec 14, 2016

sophiebits Dec 14, 2016

bvaughn Dec 14, 2016

edvinerikson Dec 13, 2016

edvinerikson Dec 13, 2016

bvaughn Dec 13, 2016

edvinerikson commented Dec 13, 2016

sophiebits left a comment

sophiebits Dec 14, 2016

bvaughn Dec 14, 2016

sophiebits Dec 14, 2016

sophiebits Dec 14, 2016

sophiebits Dec 14, 2016

sophiebits Dec 14, 2016

bvaughn Dec 14, 2016 •

edited

Loading

sophiebits Dec 14, 2016

bvaughn Dec 14, 2016 •

edited

Loading

sophiebits Dec 14, 2016

sophiebits Dec 14, 2016

sophiebits Dec 14, 2016

bvaughn Dec 14, 2016 •

edited

Loading

sophiebits commented Dec 14, 2016

ReactNative fiber renderer #8560

ReactNative fiber renderer #8560

Conversation

bvaughn commented Dec 12, 2016 • edited Loading

Known issues

Open question: How should we handle children?

Option 1: Manage index information in Fiber

Option 2: Update native bridge

edvinerikson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edvinerikson Dec 13, 2016 • edited Loading

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

bvaughn Dec 14, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edvinerikson commented Dec 13, 2016

sophiebits 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

bvaughn Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Dec 14, 2016 • edited Loading

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

bvaughn Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

sophiebits commented Dec 14, 2016

bvaughn commented Dec 12, 2016 •

edited

Loading

edvinerikson Dec 13, 2016 •

edited

Loading

bvaughn Dec 14, 2016 •

edited

Loading

bvaughn Dec 14, 2016 •

edited

Loading

bvaughn Dec 14, 2016 •

edited

Loading

bvaughn Dec 14, 2016 •

edited

Loading