Blueprint: Removing findDOMNode

[mormahr]

Update:
I think my analysis about how this can be solved is wrong. See my comment.

---

As part of the move to strict and concurrent mode support, I'd like to tackle removing usages of findDOMNode. I analyzed the scope of changes needed in the first-party components and possible implications on third-party consumers.

Alternatives to findDOMNode

Refs (preferred)

Refs are probably the standard way to get access to an HTML node. Instead of explicitly wiring refs through the consuming component, they can often be set through cloneElement.
Downsides:

• Can reference JSX elements instead of DOM nodes

Workarounds:

• Add a warning in the documentation and print a runtime warning
• Some components already only allow a single DOM node as a child (e.g. ResizeSensor)
  
  
  
  • Can clone the element and attach a ref that way
  
  

Wrapper element

Downsides:

• Can cause problems with DOM nesting and layout
• Possible performance overhead

Workarounds:

• display: contents; (doesn't work all most usages)
• Provide tagName, etc. properties

Usages

ResizeSensor

ResizeSensor uses findDOMNode to attach a ResizeObserver to the child node. It requires a single DOM node child and can use cloneElement to attach a ref. I don't know about the performance implications of cloneElement (resize sensor explicitly mentions this in the source comments), but ContextMenuTarget also uses cloneElement. My understanding is that cloneElement is pretty cheap. I don't even think it's slower than findDOMNode, so I'd say cloneElement and attaching a ref is the way to go here.

ContextMenuTarget

The element returned by findDOMNode is used to check if dark mode is enabled or not, by calling a helper method. ContextMenuTarget already requires a DOM node and uses cloneElement to attach the event-listener so that we can attach a ref in the same call.

Overlay

If I understand this correctly TransitionGroup creates a container by instantiating the component passed to it. As Overlay requests a div, this div is the element we want a reference to. I currently don't know how to get a ref to this div. Afaik TransitionGroup doesn't expose a reference. This needs some further investigation.

Conclusion

Without considering Overlay, I'd say this is doable without breaking changes. I would split this into three separate tasks.

1. Using cloneElement in ContextMenuTarget to attach a ref is a non-breaking change.
2. Using cloneElement in ResizeSensor to attach a ref is a non-breaking change.
3. Overlay needs some further investigation.

There have been previous issues that mentioned findDOMNode. For reference:

• #3361
• #3797

[LoiKos]

Hi there,

I comment here in this issue because i get a lot of struggle with refs due to the fact their aren't forwarded (e.g: Card). It get better since you add some elementRef to some components. It's just for "Basic" component like Card, Button,... e.g the way Suggest is build is totally fine.

From react docs :

In most cases, you can attach a ref to the DOM node and avoid using findDOMNode at all.

I don't know if it's in your planning right now but i think i will be a great improvement.

[mormahr]

Oh well, I wanted to comment on this for a long long time. I tried to get this to work without breaking the API (by explicitly passing the ref), but I'm pretty sure it's impossible. I learned a lot about cloneElement / createElement along the way. Unfortunately what I learned was, that it doesn't work how I thought it would work. I think the only way to do this (and the "right" way) is to indeed manually forward the refs. So the whole conclusion, that it can be done without breaking changes, seems highly unlikely. Except if someone smarter than me figured out how to do it.

I'm looking forward to the day blueprint will be strict mode ready!

[adidahiya]

Thanks for investigating @mormahr. I took a look at this last week as well and attempted to do it without breaking API; I was unable to do so. Once React 17 is released and all our internal applications are migrated to React 16 (likely by the end of this year), I will make it a priority to work on this issue and other 4.x breaking changes.