#2327 : Use this._rootNodeID in componentDidMount of ReactDOMInput

[ThomasCrevoisier]

No description provided.

[syranide]

If we do this, we should do it for componentWillUnmount too.

I think this makes sense, if we're going to rely on internal implementation details we should rely on the best implementation details available, reading from the DOM is just dirty (and slow). In the future this will use Map (unless we can get rid of it entirely), the DOM node is irrelevant (EDIT: but that's not actually true... hmm), we just want something stringy that is unique to each component.

@zpao @sebmarkbage I added review-needed, slap me if I'm doing it wrong if you prefer to handle it yourselves. :)

[ThomasCrevoisier]

Yes, I thought that while reading the code but I wanted to restricted myself as close as the issue which was about componentDidMount. And I didn't know if there were reasons to get the DOM node in componentWillUnmount.
Do you want me to modify it also in componentWillUnmount in my PR ?

[syranide]

@ThomasCrvsr Yeah I'd say so, getting the same ID from two different places is just asking for trouble :)

[syranide]

It's used in _handleChange too.

[ThomasCrevoisier]

Can I call directly otherNode._rootNodeID in _handleChange ? I have weird behavior in test when I try to use this.

[syranide]

Hmm, no you can't because it's a DOM node. I guess _handleChange will just have to remain as-is if we go with this.

Argh, the correct solution is probably to leave it as it was, but only set/use instancesByReactID if it's a radio.

[ThomasCrevoisier]

It's already the case isn't it ?

[syranide]

@ThomasCrvsr for _handleChange yes, but not componentDidMount and componentWillUnmount which are the ones we actually care about, as they currently apply to all variations of input type.

[syranide]

After some more thought...

The radio workaround is pretty much a big hack, are we OK with simply assuming that instance._rootNodeID === getID(node) for now? Or better to just do it properly and only use instancesByReactID when type="radio"? I'm tempted to just throw it all out and not query the DOM nodes at all, simply storing instances by name and iterating that list.

Devs, what's your take?

[sebmarkbage]

I was planning on refactoring the internals so that they're not exposed on composite component classes. This would not work in the future anyway. Will close out for now. This perf improvement is noted though.