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:

• Support React Strict Mode #3361
• Breadcrumbs, OverflowList: Support React Strict Mode #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.

[adidahiya]

fixed by #4623, released in 4.0.0-alpha.0

[adidahiya]

Re-opening since this is now part of v5.0

[switz]

I'm still seeing this when using OverflowList in "@blueprintjs/core": "^4.3.2",

Warning: findDOMNode is deprecated in StrictMode. findDOMNode was passed an instance of Blueprint4.ResizeSensor which is inside StrictMode. Instead, add a ref directly to the element you want to reference. Learn more about using refs safely here: https://reactjs.org/link/strict-mode-find-node

[adidahiya]

Yep, I'm aware that Blueprint v4 still has incompatibilities with React strict mode. I'm hoping to get this resolved in the next 6-8 weeks, once I've made more progress on the Popover2 migration work.

[malechite]

@adidahiya How is the Popover2 migration going? I'd like to use Tooltip2 and Popover2 in React strict mode as soon as it's ready :)

[adidahiya]

Unfortunately I've had to reduce the scope of Blueprint v5.0 in order to ship it faster. React strict mode compatibility is now scheduled for Blueprint v6.0.

[adidahiya]

Most usage of findDOMNode has been removed at this point in v5.0.0-alpha.1, there is just one instance of it in the <Draggable> component. I might be able to fix that for the stable v5.0 release.

[adidahiya]

I removed the last instance of findDOMNode in Blueprint code via #6137.

Last remaining item is to use the nodeRef feature on <CSSTransition> so that our dependencies don't use findDOMNode either. See https://github.com/reactjs/react-transition-group/blob/2989b5b87b4b4d1001f21c8efa503049ffb4fe8d/CHANGELOG.md?plain=1#L41-L56

[adidahiya]

Some decomp on the problem of migrating Overlay to use react-transition-group's nodeRef feature:

• This will require Overlay's children to support the ref property so that we can inject a ref, similar to our requirements for ResizeSensor2
• We can't add this requirement without a breaking change, so it will have to come through a "V2" component named Overlay2
  • We can migrate Blueprint's internal overlay components (Dialog, Drawer, Popover, OverlayToaster, OverlayExample, Omnibar) to use Overlay2 in v5.0 v5.x (this is straightforward since they all use a <div> container element which supports ref)
• Consumers of <Overlay> will have to migrate to <Overlay2> to stop using findDOMNode
• Overlay2 will replace Overlay in Blueprint v6.0