{"action":"created","issue":{"url":"https://api.github.com/repos/facebook/react/issues/1570","labels_url":"https://api.github.com/repos/facebook/react/issues/1570/labels{/name}","comments_url":"https://api.github.com/repos/facebook/react/issues/1570/comments","events_url":"https://api.github.com/repos/facebook/react/issues/1570/events","html_url":"https://github.com/facebook/react/pull/1570","id":33844139,"number":1570,"title":"[idea] Remove data-reactid and traverse immediately after insertion instead (nope, still lazy!)","user":{"login":"syranide","id":1714255,"avatar_url":"https://avatars.githubusercontent.com/u/1714255?v=3","gravatar_id":"","url":"https://api.github.com/users/syranide","html_url":"https://github.com/syranide","followers_url":"https://api.github.com/users/syranide/followers","following_url":"https://api.github.com/users/syranide/following{/other_user}","gists_url":"https://api.github.com/users/syranide/gists{/gist_id}","starred_url":"https://api.github.com/users/syranide/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/syranide/subscriptions","organizations_url":"https://api.github.com/users/syranide/orgs","repos_url":"https://api.github.com/users/syranide/repos","events_url":"https://api.github.com/users/syranide/events{/privacy}","received_events_url":"https://api.github.com/users/syranide/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/facebook/react/labels/GH%20Review:%20needs-revision","name":"GH Review: needs-revision","color":"e11d21"}],"state":"closed","locked":false,"assignee":null,"milestone":null,"comments":22,"created_at":"2014-05-19T22:38:47Z","updated_at":"2015-06-01T08:19:12Z","closed_at":"2014-10-25T11:06:48Z","pull_request":{"url":"https://api.github.com/repos/facebook/react/pulls/1570","html_url":"https://github.com/facebook/react/pull/1570","diff_url":"https://github.com/facebook/react/pull/1570.diff","patch_url":"https://github.com/facebook/react/pull/1570.patch"},"body":"Part 1 of #1550\r\n\r\nThis PR removes `data-reactid` from the DOM and reconstructs it on-demand, then storing it as a hidden property `__reactID` on the node instead. Nodes and instances are traversed as pairs, in sync, when reconstructing.\r\n\r\n**Peformance comparison:**\r\nWe have the option to choose between keeping the current on-demand traversal of nodes, or switching to immediate traversal after insertion into the DOM. Running my old super synthetic benchmark, creating 11 level deep trees of plain divs, measuring mount+unmount. The lazy method is roughly 15% faster it seems, adding just one attribute to all nodes reduces it to about 9%, adding one more reduces it to about 5% (for IE8 and Chrome).\r\n\r\nHowever, it is important to note that these numbers are significantly inflated as they do *not* include reflow (which seems to increase running time of my tests by about 75%), actual rendering nor any other common React overhead such as composite components, handling event listeners, etc. It should also be noted that as of current, React will immediately traverse to any node containing an onClick-listener or an image with onLoad/onError-listeners or if you call getDOMNode in componentDidMount. Additionally, any updates applied to the DOM also require traversal to each affected node. None of which are included in this benchmark.\r\n\r\nSo while end-user measurable performance impact is hard to quantify, I think it's safe to say that at worst it may be effectively increased by 5% (excluding rendering), add a bunch of attributes, add some React overhead, onClick-listeners and we're down to perhaps 1-3%? In that context, I think it's important to consider that there are important real-life implications of on-demand traversal:\r\n\r\n**For immediate traversal;** testing whether a node is a React-node is always O(1). We also avoid a lot of complex code that is otherwise there to ensure that a node exists in the cache, which means that going from node to instance to node is super cheap, which may open up some interesting potential down the road. We also detect invalid DOM nesting on insertion (for free). The size of React decreases \"measurably\" and code complexity is decreased.\r\n\r\n**For on-demand traversal;** we must be able to traverse up the component instance parent tree and each node instance must immediately register itself with ReactMount. Testing whether a node is a React node is O(1) for cached React nodes and \"up to\" O(depth) for uncached React nodes or non-React nodes. We also lose the guaranteed/immediate validation of DOM nesting (although we can enable it for DEV).\r\n\r\nThere are a lot of improvements that this PR enables that is independent of immediate or on-demand traversal, but lets not make this wall of text any longer. I can see the benefit of both methods, but I think immediate traversal intrigues me the most given its modest cost, it's also *uniform* and deterministic during insertion and over time (not that it really matters given the modest performance implications, perhaps).\r\n\r\n----\r\n\r\n#1568 and #1569 must be fixed before this PR can be taken (or simple temporary workarounds have to be put in place). Add #1578 too and React leaves no lasting print in the visible DOM.\r\n\r\nThis PR has commits for both the immediate and on-demand traversal, both are fairly final implementations (obviously pending some decisions / feedback though).\r\n\r\n----\r\n\r\n- [ ] Waiting for #1434 to land (intersecting code refactored)\r\n- [ ] Depends on #1568 to land\r\n- [ ] Depends on fix for #1569 (@sebmarkbage?)\r\n- [ ] Lazy or immediate traversal?"},"comment":{"url":"https://api.github.com/repos/facebook/react/issues/comments/107356473","html_url":"https://github.com/facebook/react/pull/1570#issuecomment-107356473","issue_url":"https://api.github.com/repos/facebook/react/issues/1570","id":107356473,"user":{"login":"syranide","id":1714255,"avatar_url":"https://avatars.githubusercontent.com/u/1714255?v=3","gravatar_id":"","url":"https://api.github.com/users/syranide","html_url":"https://github.com/syranide","followers_url":"https://api.github.com/users/syranide/followers","following_url":"https://api.github.com/users/syranide/following{/other_user}","gists_url":"https://api.github.com/users/syranide/gists{/gist_id}","starred_url":"https://api.github.com/users/syranide/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/syranide/subscriptions","organizations_url":"https://api.github.com/users/syranide/orgs","repos_url":"https://api.github.com/users/syranide/repos","events_url":"https://api.github.com/users/syranide/events{/privacy}","received_events_url":"https://api.github.com/users/syranide/received_events","type":"User","site_admin":false},"created_at":"2015-06-01T08:19:12Z","updated_at":"2015-06-01T08:19:12Z","body":"@callmevlad What can be done somewhat non-invasively (i.e. an easy to maintain fork) at the moment is the \"immediate traversal\" version of this PR. There are two alternatives:\r\n\r\n1. The very simplest being to simply immediately traverse any rendered nodes, copy `data-reactid` into `node.reactid`, remove `data-reactid` and update `internalGetID` to read from `node.reactid` instead (more or less). You still have to ship `data-reactid` with server-side markup though.\r\n\r\n2. Again, immediately traverse any rendered nodes but reconstruct `data-reactid` (quite easy too) instead and store it in `node.reactid`. The rest is same as above, but this doesn't require you to ship `data-reactid` with the server-side markup.\r\n\r\nI can't imagine the devs being OK with having this \"hack\" in the release version so it would have to be in a separate fork. But as I said earlier I could imagine the server-side part of alternative 2 being shipped though (but not my call) which would make the client-side fork near trivial. I don't imagine the performance impact of this being significant.\r\n\r\nFor reference, most of the changes in this PR is because of the lazy traversal and updated tests. It's possible I overlooked something minor above, but this PR was functional so it's definitely doable."}}