ReactDOMComponentTree #5190

sophiebits · 2015-10-15T23:13:09Z

New module! With this, we have a new module that uses the component tree to store DOM nodes. Most of the logic in this file relates to markup adoption -- if we were to drop that (along with server rendering) this file could just be a return inst._nativeNode;.

This works with useCreateElement only because we need to reference each node as it's created.

Events is now the only thing using hierarchical IDs and ReactMount.getNode -- I'll use ReactDOMComponentTree.getInstanceFromNode to make that work.

gaearon · 2015-10-15T23:27:02Z

src/renderers/shared/reconciler/__tests__/ReactEmptyComponent-test.js

-  it('should be able to switch between rendering null and a normal tag', () => {
-    spyOn(console, 'log');
-
+  it.only('should be able to switch between rendering null and a normal tag', () => {


Thanks, fixed.

facebook-github-bot · 2015-10-16T00:40:20Z

@spicyj updated the pull request.

facebook-github-bot · 2015-10-16T05:05:33Z

@spicyj updated the pull request.

facebook-github-bot · 2015-10-16T07:31:40Z

@spicyj updated the pull request.

gaearon · 2015-10-16T08:16:45Z

What is the big picture here? Getting rid of data-reactid eventually? Better resilience against evil browser plugins?

sophiebits · 2015-10-16T08:30:55Z

Getting rid of hierarchical IDs (replacing them with numbers, probably) and the giant hash map we have in ReactMount. We can probably kill data-reactid entirely when not using server-side rendering.

jimfb · 2015-10-16T14:57:25Z

@spicyj My hope is that we can kill data-reactid entirely especially when using server-side rendering. Rehydrating a markup tree does not benefit from having data-reactid afik.

sebmarkbage · 2015-10-16T17:57:58Z

src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js

@@ -52,7 +52,7 @@ describe('ReactDOMComponent', function() {
      var stubStyle = container.firstChild.style;

      // set initial style
-      var setup = {display: 'block', left: '1', top: 2, fontFamily: 'Arial'};
+      var setup = {display: 'block', left: '1px', top: 2, fontFamily: 'Arial'};


What's this about? Behavior change?

Styles may be unitless if they are integers, but strings must specify units if required (unitless strings may fire a warning). This looks fine to me.

@jimfb recently merged a PR that adds a warning for this case but it only checked the create case, not the update case which we use for createElement. I fixed that so the test for the warning passes with useCreateElement and fixed this test to not warn.

sophiebits · 2015-10-16T18:10:10Z

@jimfb I'm operating under the assumption that browser extensions will insert extra nodes into our markup and we need to skip over them.

sebmarkbage · 2015-10-16T18:19:02Z

Make sure you do your react-native homework

syranide · 2015-10-16T18:34:51Z

My hope is that we can kill data-reactid entirely especially when using server-side rendering. Rehydrating a markup tree does not benefit from having data-reactid afik.

@jimfb FYI, we can already kill data-reactid for server-rendered markup, it's rather trivial as long as you're ok with doing a fast pass over all the nodes once when starting the client.

facebook-github-bot · 2015-10-16T18:35:28Z

@spicyj updated the pull request.

sophiebits · 2015-10-16T18:38:17Z

We can do that trivially here too by changing precacheChildNodes to not look at IDs and just match up the nodes – but I'm worried about finding extra nodes.

syranide · 2015-10-16T18:47:13Z

@spicyj We can always mark the nodes with just data-react, but leave out the ID. That should give you most of the benefits, while also tolerating foreign nodes. Personally I think we should just be super strict and simply not allow unexpected nodes, they are crashes waiting to happen.

sophiebits · 2015-10-16T18:48:41Z

That's true, but I don't think including a numeric ID (data-reactid="17") is much overhead over that.

jimfb · 2015-10-16T18:52:44Z

Personally I think we should just be super strict and simply not allow unexpected nodes, they are crashes waiting to happen.

I tend to agree - if we find an unexpected node while restoring a SSR DOM tree, we can just kill it with fire. We don't even need a flag on the nodes to do that.

syranide · 2015-10-16T19:14:34Z

That's true, but I don't think including a numeric ID (data-reactid="17") is much overhead over that.

@spicyj data-react compresses trivially to virtually nothing, but yeah the overhead of the ID should be minimal too. Both are miles better than the current reactids anyway.

facebook-github-bot · 2015-10-18T01:32:32Z

@spicyj updated the pull request.

sophiebits · 2015-10-18T01:32:57Z

@jimfb @sebmarkbage The last two commits (edit: more by the time you read this, start with the one called "Events:") add events support so we don't rely on hierarchical IDs for events either. (Sorry for piling it all into the same PR – but github sucks at stacking PRs and I didn't want to merge this to master quite yet.) Either of you want to take a look? I'm a little nervous but all the FB test flows worked the first time with this applied.

facebook-github-bot · 2015-10-18T03:41:54Z

@spicyj updated the pull request.

sophiebits · 2015-10-18T19:15:47Z

Did a quick benchmark of rendering a PE-like hierarchy with Electron (Chrome) with a cold and warm JIT using my perf-counters as the timestamp.

With a warm JIT (rendering the hierarchy 100 times before measuring anything):

This PR is 3.3% faster than master.
Switching to numerical IDs (will be a separate PR, not quite done yet) brings us to 7.9% faster than master.
Numerical IDs with createElement instead of HTML generation is on par (also 7.9%).
Numerical IDs with createElement and not setting data-reactid at all (since we don't need it) gives another 1.4% boost for a total of 9.1%.

With a cold JIT, createElement looks a little bit slower (but this PR helps more compared to master!).

Our deltas of
[3.3%, 7.9%, 7.9%, 9.1%] with a warm JIT are
[4.9%, 9.6%, 8.3%, 8.9%] with a cold JIT.

Not sure if there are things we can do to improve the speed of createElement.

syranide · 2015-10-18T19:19:33Z

@spicyj Just for context, I assume those are React rendering times + innerHTML but do not include browser rendering/painting? PS. Also that's awesome :)

sophiebits · 2015-10-18T19:22:10Z

var a = now();
React.render(React.createElement(Benchmark), container);
container.offsetHeight;
var b = now();

There's probably some painting involved. I intentionally forced layout: I wanted to make sure all the markup was parsed to make sure the HTML mode wasn't doing stuff lazily. Any ideas on how to keep that guarantee while avoiding painting (and layout)?

syranide · 2015-10-18T19:30:16Z

@spicyj I'd assume that would just force layouting, realistically rendering/painting should only happen after JS execution ends. I'm pretty sure innerHTML is not deferred as you can measure differences in time by the number of tags/attributes/styles/etc present. It probably doesn't really make sense to defer it in practice either I would think.

sophiebits · 2015-10-18T19:34:48Z

You're probably right. I tried it again without the forced layout and it's about 24ms (on master: 210ms vs 186ms) faster on all five numbers, so the percentages are proportionally the same but go up a little because of a smaller base. Cold JIT, just React.render (no layout): [5.5%, 10.8%, 9.2%, 10.0%].

sophiebits · 2015-10-18T23:05:24Z

Numerical IDs are in #5205. Also, devtools don't work with this because they currently rely on ReactMount.getNode.

Initial render can still be a markup string.

New module! With this, we have a new module that uses the component tree to store DOM nodes. Most of the logic in this file relates to markup adoption -- if we were to drop that (along with server rendering) this file could just be a `return inst._nativeNode;`. This works with useCreateElement only because we need to reference each node as it's created. Events is now the only thing using ReactMount.getNode -- I'll introduce pointers back from the DOM nodes (and a `ReactDOMComponentTree.getInstanceFromNode`) and make that work.

Next step: take advantage of having the native instances in EventPropagators instead of converting right back to IDs.

This removes SimpleEventPlugin's dependency on ReactMount.getID.

facebook-github-bot · 2015-11-04T02:22:28Z

@spicyj updated the pull request.

sophiebits · 2015-11-04T19:47:03Z

Merged in #5205.

facebook-github-bot added GH Review: review-needed CLA Signed labels Oct 15, 2015

gaearon reviewed Oct 15, 2015
View reviewed changes

sophiebits force-pushed the cotree branch from af9fd07 to f82106e Compare October 16, 2015 00:37

sophiebits force-pushed the cotree branch 2 times, most recently from 569414c to 2188cf8 Compare October 16, 2015 04:55

sophiebits force-pushed the cotree branch from 2188cf8 to ca52f7f Compare October 16, 2015 07:30

sebmarkbage reviewed Oct 16, 2015
View reviewed changes

sebmarkbage added GH Review: accepted and removed GH Review: review-needed labels Oct 16, 2015

sophiebits force-pushed the cotree branch from ca52f7f to de4a18f Compare October 16, 2015 18:34

sophiebits force-pushed the cotree branch 3 times, most recently from de4a18f to 7ace2b9 Compare October 18, 2015 01:31

sophiebits mentioned this pull request Oct 18, 2015

Use incrementing numerical IDs to identify DOM components #5205

Merged

syranide mentioned this pull request Oct 20, 2015

[idea] Remove data-reactid and traverse immediately after insertion instead (nope, still lazy!) #1570

Closed

4 tasks

chicoxyzzy mentioned this pull request Nov 1, 2015

Inspector doesn't work properly with classic ES5 decorator syntax lahmatiy/component-inspector#11

Closed

sophiebits added 10 commits November 3, 2015 17:03

Make ReactEmptyComponent-test not swallow logs

19e2cf5

Remove obsolete tests with new tree-walking

5becbe3

Make ReactDOMComponent-test pass with useCreateElement

276ef73

Always do useCreateElement for updates

5a7c273

Initial render can still be a markup string.

Add functions to find the instance given a node

6d20556

Events: Pass native instances up until propagation

d946dee

Next step: take advantage of having the native instances in EventPropagators instead of converting right back to IDs.

Propagate events based on component tree, not IDs

5e83125

put/deleteListener take an instance, not an ID

9d93af2

This removes SimpleEventPlugin's dependency on ReactMount.getID.

Kill ReactMount.getNode/getID/purgeID with fire

c85e591

sophiebits force-pushed the cotree branch from 6755d1b to c85e591 Compare November 4, 2015 02:21

sophiebits closed this Nov 4, 2015

ReactDOMComponentTree #5190

ReactDOMComponentTree #5190

Uh oh!

Conversation

sophiebits commented Oct 15, 2015

Uh oh!

gaearon Oct 15, 2015

Choose a reason for hiding this comment

Uh oh!

sophiebits Oct 16, 2015

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Oct 16, 2015

Uh oh!

facebook-github-bot commented Oct 16, 2015

Uh oh!

facebook-github-bot commented Oct 16, 2015

Uh oh!

gaearon commented Oct 16, 2015

Uh oh!

sophiebits commented Oct 16, 2015

Uh oh!

jimfb commented Oct 16, 2015

Uh oh!

sebmarkbage Oct 16, 2015

Choose a reason for hiding this comment

Uh oh!

jimfb Oct 16, 2015

Choose a reason for hiding this comment

Uh oh!

sophiebits Oct 16, 2015

Choose a reason for hiding this comment

Uh oh!

sophiebits commented Oct 16, 2015

Uh oh!

sebmarkbage commented Oct 16, 2015

Uh oh!

syranide commented Oct 16, 2015

Uh oh!

facebook-github-bot commented Oct 16, 2015

Uh oh!

sophiebits commented Oct 16, 2015

Uh oh!

syranide commented Oct 16, 2015

Uh oh!

sophiebits commented Oct 16, 2015

Uh oh!

jimfb commented Oct 16, 2015

Uh oh!

syranide commented Oct 16, 2015

Uh oh!

facebook-github-bot commented Oct 18, 2015

Uh oh!

sophiebits commented Oct 18, 2015

Uh oh!

facebook-github-bot commented Oct 18, 2015

Uh oh!

sophiebits commented Oct 18, 2015

Uh oh!

syranide commented Oct 18, 2015

Uh oh!

sophiebits commented Oct 18, 2015

Uh oh!

syranide commented Oct 18, 2015

Uh oh!

sophiebits commented Oct 18, 2015

Uh oh!

sophiebits commented Oct 18, 2015

Uh oh!

facebook-github-bot commented Nov 4, 2015

Uh oh!

sophiebits commented Nov 4, 2015

Uh oh!

Uh oh!