[POC] Replace concatenated IDs with monotonic IDs

[syranide]

Continuation of: #934

This is just a proof-of-concept, so the code is far from nice. It generates monotonic IDs prefixed with the rootID and store the full representation internally. It's currently a very simple implementation.

It's possible to get rid of reactIDtoMountID. It would also be possible to use the mountID as the internal ID as well and only use the concatenated ID where necessary, but even that isn't strictly necessary as the concatenated ID could be replaced with a mountID to parentMountID map (I believe would work and suffice).

If I understood @petehunt correctly, this (or with minor changes) should work for server-rendering as well, and uniqueness is guaranteed by the use of the rootID prefix (at least to the degree we guarantee it today).

Again, just a proof-of-concept, but I figure it's easier to get the discussion started this way. :)

I have it compiled and running here: http://dev.cetrez.com/jsx/2/react/

[syranide]

cc @sebmarkbage @benjamn @petehunt @spicyj

[jordwalke]

Took a quick glance at your diff. I couldn't tell, but it looks like it preserves the original 'x.y' ID scheme inside of rootNodeID. When implementing the current ID scheme, it seemed like the easiest way to guarantee a fairly deterministic ID assignment such that errors are easily spotted and programmatically detected - especially in the context of server rendering. What I mean by this, is if you traverse the ID path generated on the server on some event, you could easily tell when your local version of nodes is different than what the server generated. If we chose rootNodeIDs of 1, 2, 3, it seemed more difficult to detect the non-determinism. Luckily, @petehunt implemented markup validation for server rendered markup! So that means some of the reasoning behind the x.y style IDs may no longer apply. So _rootNodeID could itself take the form of monotonically (and deterministically) increasing integers.

The main change would be in event handling, where we can no longer parse the ID string to bubble/capture. I've actually found that (at least on a non-JIT VM) considerable time can be spent parsing these string x.y IDs during event propagation and I'm hoping numeric ids provide an opportunity to increase the performance here. Also, there would be far fewer string concats, though I don't think that's our bottleneck. Implementing this means you'd likely need a global lookup map of child to parent ID. Thoughts?

[syranide]

@jordwalke Yep, you are completely correct. It was discussed a bit in #934 and I thought I'd put up a new PR instead. I glanced over it above, but I basically see the following progression (but I don't know all the technical details right now):

---

1. Create monotonic IDs for the DOM and create monotonicID => concatenated ID.
2. Use monotonic ID => DOM component internally instead of concatenated ID => DOM component, keep monotonic ID => concatenated ID for event handling.
3. Replace monotonic ID => concatenated ID with monotonic ID => parent monotonic ID.
4. Replace monotonic ID => parent monotonic ID with DOM component => parent monotonic ID, i.e. a property on component.
5. Replace DOM component => parent monotonic ID with DOM component => parent DOM component.

Result: Only monotonic ID => DOM component and DOM component => parent DOM component.

---

So that would be my internal deconstruction of the issue, and if I haven't overlooked something along the way then ending up at the last step seems like the way to go? Intuitively it also seems like it should be simpler than it sounds, as we could just replace the passing of rootID with parent DOM component during construction.

I'm not 100% familiar with all the extremities of React and event handling in particular, so it's very possible that I've overlooked something important, just shout if you spot it. :) I'll think it through once more when I get home later.

[jordwalke]

Incremental refactors like that are almost always better, so I'm glad you've thought about it that way. But it might be nice to get them all staged up stacked on top of each other just to prove that the final outcome actually does what we want, and so we can get a sense of the performance improvements/harms. There could be something we're overlooking in the implementation as well.

[johnthethird]

+1

I am working on server-rendering code for Rails and caching the resulting markup is not really possible with the current random IDs polluting things.

[syranide]

@johnthethird This will not actually fix the IDs for you, the random rootID will still have to be there. I'm not familiar with server-rendering, but I think you can override the random rootID and provide a fixed, but you better not get conflicts. ;)

[sophiebits]

@johnthethird Even though the generated markup changes each time, I don't think you should have any problem from caching it.

[johnthethird]

@spicyj Yes of course you are correct. Not sure what I was thinking there. The cache key could be a hash of the props.

[syranide]

@benjamn (cc @jordwalke) I've kind of hit a snag with this PR, this PR itself currently implements step 1, which in itself is completely isolated change.

However, it makes ReactMount stateful, which first of brings up the question of renderComponentToString. Server-rendering testing is currently done by using the same React-instance for both server and client, as we now use monotonic IDs, when the client now renders it the IDs continue where the server left off (i.e, checksum mismatch).

There are a bunch of tests that now fail because they simply "just happened to work" before because ReactMount was stateless, I managed to fix some of them. There are also a bunch of tests that I couldn't quite figure out what's wrong, I'm strongly suspecting that it's multiple ReactMount instances that is the problem, but I can't really seem to "fix it".

[zpao]

Are we still considering this? (pinging @sebmarkbage @petehunt @jordwalke)

[syranide]

Closing in favor of #1550.