Added new ReactARTFiber target based on ReactFiberReconciler #105

bvaughn · 2016-11-29T23:31:16Z

Also updated existing unit tests to run against both legacy and fiber ReactART builds.

The new target also fixes a bug with event handlers that exists in the legacy target. I believe the existing react-art renderer intended to use handleEvent as the event subscriber but was actually using the props.listener. This mostly worked, but failed in the case where a specific event handler (eg onClick) changed.

While testing, I noticed that the ClippingRectangle target doesn't work quite right for SVG mode. This is not a regression. It's something that needs to be fixed in the underlying Art library (if we care).

sebmarkbage

We can't really land this as is to replace ReactART. Maybe we should land something as ReactARTFiber first so that we can test it out first?

sebmarkbage · 2016-11-30T00:15:07Z

examples/vector-widget/VectorWidget.js

-var MAX_VEL = 11;
-var CLICK_ACCEL = 3;
-var BASE_VEL = 0.15;
+const React = require('react');


Can we split this file out into a separate PR? It's unclear if you needed to change something here to support the new renderer or if it is just a style change. Regardless it's unrelated.

Sure. This file was causing some console warnings before:

Warning: bind(): React component methods may only be bound to the component instance. See VectorWidget

No changes were required though. I just fixed the warnings and added a little better react-art component coverage. I can do both via a separate PR.

sebmarkbage · 2016-11-30T00:17:26Z

src/ReactART.js

+
+    this._mountNode = ARTRenderer.mountContainer(
+      this.props.children,
+      container,


This no longer passes context along. I think we'll need to provide the third argument here.

This may be a stupid question...but...why do ReactART components need context? The set of components being managed by the ARTRenderer only includes the ones explicitly defined in this module, none of which use context.

Because it can pass through from components above. That's how we use it at FB.

class Foo { getChildContext() { return { foo: 123 }; } render() { return <div><Surface><Group><Bar /></Group></Surface></div>; } } class Bar { render() { return <Group><Path /><Text>{this.context.foo}</Text></Group>; } }

Gotcha. That makes sense. Thanks!

sebmarkbage · 2016-11-30T00:18:00Z

src/ReactART.js

-        props.path
-      );
-      this._oldString = newString;
+    ARTRenderer.updateContainer(this.props.children, this._mountNode);


This also needs to pass the third argument.

sebmarkbage · 2016-11-30T00:23:37Z

src/ReactART.js

+    return instance;
+  },
+
+  createTextInstance(text, internalInstanceHandle) {


I believe that Fiber will turn any array of strings into createTextInstance calls. So I think that something like <Text>{"foo"}{"bar"}</Text> will break.

Ah. You are correct.

I'm unsure of how to best handle this since the text param is just a string with no associated props (eg font, color). At a glance, I think this information is accessible via internalInstanceHandle.return.pendingProps but I'm not sure if that would be acceptable or hacky, eg:

createTextInstance(text, internalInstanceHandle) { const props = Object.assign( {}, internalInstanceHandle.return.pendingProps, { text } ); return new Text(props); },

Hm, one option could be to make Text a composite that flattens the children and renders the lower-level host text component.

Seb probably has other ideas.

Yeah, I think I'll go with something like that after trying this as a first pass.

I think I may punt on supporting custom components that return text for this initial iteration. We can always add support for that in a follow-up.

The current API makes it difficult to associate a text string with its parent instance. I initially tried:

createTextInstance returns text (string).

appendInitialChild handles special case where child is a string by appending to an array stored on the ART text instance (for lack of a better place).

When applying props for text, use the aforementioned array whenever props.children is not a string.

Unfortunately commitTextUpdate only gets the updated text string and has no reference to the ART instance, so no way to update the text-array.

Also, createTextInstance is called before the ART instance is created and so the only thing I have access to at that point is the internalInstanceHandle which I shouldn't be crawling in the renderer. This also complicates the initial display because- without using internalInstanceHandle- there's no way to gather up text-children and pass them along to the eventual ART text instance.

You would need to have a place holder object for text instances with a parent pointer and store an expando with the list of text instances on the Text ART instance. So that when commitTextUpdate happens, you can do textInstance.text = newText; textInstance.parent.draw(textInstance.parent._textInstances.map(ti => ti.text).join(''))

Maybe we need an alternative text API that makes it easier for certain renderers. I'm not sure if it can be much more efficient than this though since these can be nested deeply. So maybe a helper lib on top of host config would be enough.

I tried that as well, but it didn't work for reasons I tried to explain above. (Maybe one of us is misunderstanding each other?) If it would be easier to chat on Hangouts I could. 😄

You would need to have a place holder object for text instances with a parent pointer

The problem is that when the text is first created (createTextInstance) I only have access to the text string itself and the internalInstanceHandle. I could crawl the return pointers to find the parent text node- but that's probably not a good idea to do in the renderer.

Also, the ART Text instance doesn't exist yet at this time (eg createInstance hasn't been called) so I would have to use the internal handle for temporary storage and then bundle along the text strings to the eventual ART instance when createInstance is called.

Overall it felt dirty/hacky.

sebmarkbage · 2016-11-30T00:26:32Z

src/ReactART.js

+/** COMPONENTS */

-const ContainerMixin = assign({}, ReactMultiChild, {
+class Node {


You have a wrapper class around every instance. However, they don't seem to have much interesting state associated with them so we're allocating them only to store a pointer to the ART instance.

Could you just use the ART instance as the instance and rely on pattern matching to pick the logic branch instead of OO methods?

Totally. I've removed the wrapper objects. The metadata I was storing on the wrapper instance (eg current Text string, previous path delta, etc) is now just being stored on the ART-created instance. Kind of hacky but it avoids the wrapper object.

sebmarkbage · 2016-11-30T00:42:04Z

src/ReactART.js

-    injectAfter(this.node, afterNode, childNode);
-  },
+    this._listeners = {};
+    this._subscriptions = {};


We don't need to store subscriptions because we can just rely on garbage collection to do the clean up for us. In fact, _removeEventListeners isn't even called.

I suppose the current ART API requires us to store the subscriptions to unsubscribe when you swap them out during updates though. Maybe we can just store them as an expando property on the ART instance as a hack?

In the normal case we don't actually use many of these listeners anyway. We attach top level listeners at the root and dispatch it ourselves.

edvinerikson · 2016-11-30T17:29:28Z

src/ReactART.js

+        instance = new Shape(props);
+        break;
+      case TYPES.TEXT:
+        instance = new Text(props);


does Text handle props.children as well? since <Text>Hello</Text> will not create a text instance.

Yes. Text handles single-children (eg <Text>Hello</Text>) and arrays of children (eg <Text>Hello, {name}</Text>).

bvaughn · 2016-11-30T19:33:10Z

We can't really land this as is to replace ReactART. Maybe we should land something as ReactARTFiber first so that we can test it out first?

Sure! I added a new ReactARTFiber.js file. Assuming you have the latest build of React running locally- to test it, either change package.json so that the main entry points at the fiber version (eg "main": "lib/ReactARTFiber.js",) or change the require statement to load react-art/lib/ReactARTFiber.

iamdustan · 2016-11-30T21:25:43Z

src/ReactARTFiber.js

+const Transform = require('art/core/transform');
+const invariant = require('fbjs/lib/invariant');
+const React = require('react');
+const ReactFiberReconciler = require('react-dom/lib/ReactFiberReconciler');


curious, are you testing this against React@master?

Yes. Pulled as recently as an hour or two ago. Why?

Was curious about your setup for running against master. I was thinking of starting a fiber implementation for https://github.com/iamdustan/react-hardware.

Oh! Gotcha. 😄

I tried using npm link at first but ran into problems with Babel trying to reprocess the dist for 'react' and 'react-dom'. Seems like the symlink created by npm link caused the exclude for 'node_modules' not to work for those packages. So I gave up on that after a while.

What I ended up doing was a bit of a hack- but it worked, so...

I checked out React master and copied the built versions of 'react' and 'react-dom' into the node_modules folder of 'react-art'. So basically...

cd path/to/react cp -r ./build/packages/react path/to/your/project/node_modules/ cp -r ./build/packages/react-dom path/to/your/project/node_modules/

Thanks! 😄

I wonder if http://react.zpao.com/builds/master/latest/ is still kept up to date for a daily sync, too...

That looks like UMD builds, which won't provide a way to load the new fiber reconciler.

I think the react-dom.tgz files are the packages for npm assets...

bvaughn · 2016-12-01T19:27:42Z

~~I've updated the unit tests to run against both version of ReactART.~~

Edit: Unfortunately this failed due to the error: "TypeError: ref is not a function". Chatted with Ben out of band and this is expected to work. The failure likely has something to do with my local setup being wonky (eg how I'm pulling in the latest react and react-dom builds). I'm not yet sure how and so for now I've reverted this change.

sophiebits · 2016-12-01T19:28:17Z

String refs should work; grep for coerceRef.

bvaughn · 2016-12-06T22:33:25Z

This change set was approved in Phabricator. I'm going to merge here to keep Github in sync.

sebmarkbage suggested changes Nov 30, 2016

View reviewed changes

edvinerikson reviewed Nov 30, 2016

View reviewed changes

bvaughn force-pushed the fiber branch from e171342 to 3d976a0 Compare November 30, 2016 19:46

iamdustan reviewed Nov 30, 2016

View reviewed changes

bvaughn changed the title ~~ReactART components based on ReactFiberReconciler~~ Added new ReactARTFiber target based on ReactFiberReconciler Dec 1, 2016

bvaughn force-pushed the fiber branch from 0f09acd to c3c3fd7 Compare December 1, 2016 19:52

This was referenced Dec 2, 2016

Fixed event handler update bug in (non-fiber/legacy) ReactART #106

Merged

Update library to work with React 15.4.1 #108

Closed

bvaughn force-pushed the fiber branch from c3c3fd7 to 8cbd3fb Compare December 6, 2016 22:29

Added new ReactARTFiber renderer, be based on ReactFiberReconciler

e5e4dcd

bvaughn force-pushed the fiber branch from 8cbd3fb to e5e4dcd Compare December 6, 2016 22:34

bvaughn merged commit 1c5e96d into reactjs:master Dec 6, 2016

bvaughn deleted the fiber branch December 6, 2016 22:44

bvaughn mentioned this pull request Dec 7, 2016

New fiber-based ReactART renderer facebook/react#8521

Merged

Added new ReactARTFiber target based on ReactFiberReconciler #105

Added new ReactARTFiber target based on ReactFiberReconciler #105

Uh oh!

Conversation

bvaughn commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebmarkbage left a comment

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!

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!

sebmarkbage Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn commented Nov 30, 2016 • 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!

Choose a reason for hiding this comment

bvaughn commented Nov 29, 2016 •

edited

Loading

sebmarkbage Nov 30, 2016 •

edited

Loading

bvaughn Nov 30, 2016 •

edited

Loading

bvaughn Nov 30, 2016 •

edited

Loading

bvaughn Nov 30, 2016 •

edited

Loading

bvaughn commented Nov 30, 2016 •

edited

Loading

bvaughn commented Dec 1, 2016 •

edited

Loading