visx
visx copied to clipboard
airbnb
ReactDOM.findDOMNode() in @vx/bounds
Hi, we encountered this warning
Warning: findDOMNode is deprecated in StrictMode. findDOMNode was passed an instance of withBoundingRects() which is inside StrictMode. Instead, add a ref directly to the element you want to reference. Learn more about using refs safely here: https://fb.me/react-strict-mode-find-node
in div (created by Tooltip)
in Tooltip (created by TooltipWithBounds)
in TooltipWithBounds (created by withBoundingRects())
Is it possible to replace findDOMNode with a ref in WrappedComponent.componentDidMount here?
Previous discussion: https://github.com/hshoff/vx/pull/509#discussion_r331679227
Hey @sosloow, I would love to help. Can you please explain the scope of the changes like have to update this accross the repo or just few files?
@hshoff I have made some changes to replace the findDOMNode with ref. I tried to test with jest but doesn't get any idea. Can you please help me how I can test that changes doesn't breaking things? thanks :)
Came across this same issue when using <TooltipWithBounds />:
https://codesandbox.io/s/discontinuous-dates-visx-3ve9k?file=/src/App.js
If I had a suggestion about how to overcome this it would be to pass a ref from the bounding element:
const ref = useRef()
const {} = useTooltip({containerRef: ref})
return (
<div>
<svg ref={ref}>
...
</svg>
<TooltipWithBounds />
</div>
)
Using a ref is definitely the right fix here (and this is now what we do for useTooltipInPortal), but we'd like to do it in a way that isn't a breaking change just yet. This probably means that we need a way to support both (in both @visx/bounds and in @visx/tooltip where it's used).
One option would be to leave the withBoundingRects as is, and introduce a new hook useBoundingRects + a new NewTooltipWithBounds (with better name) which could require a ref/ResizeObserver polyfill (see below) and not be a breaking change. In a future breaking change these would then become the defaults:
@visx/bounds changes
// @visx/bounds
import useMeaure from 'react-use-measure';
// `useMeasureOptions` may include `ResizeObserver` polyfill else it must be
// defined globally
function useBoundingRects(useMeasureOptions) {
const [privateContainerRef, containerBounds] = useMeasure(useMeasureOptions);
const [privateParentRef, parentBounds] = useMeasure(useMeasureOptions);
// set both container + parent container in public ref
const containerRef = useCallback(
(container: HTMLElement | SVGElement | null) => {
privateContainerRef(container);
privateParentRef(container?.parentElement ?? null);
},
[privateContainerRef, privateParentRef],
);
return { containerRef, containerBounds, parentBounds };
}
@visx/tooltip changes
// @visx/tooltip
import { useBoundingRects } from `@visx/bounds';
// a new component avoids a breaking change because `useBoundingRects` / `react-use-measure`
// requires ResizeObserver polyfill to be passed or defined globally
function NewTooltipWithBounds(useMeasureOptions) {
const { containerRef, containerBounds, parentBounds } = useBoundingRects(useMeasureOptions);
// ...bounds detection logic in existing `TooltipWithBounds`
return (
<Tooltip ref={containerRef} {...}>{...}</Tooltip>
);
}
and finally usage
// your app
function MyChart() {
// ...
return (
<div>
<svg>{...}</svg>
<NewTooltipWithBounds />
</div>
);
}
hey, can we help somehow to fix this issue? Is there any blocker or does a solution already exist?
I'm happy to review a PR if someone is interested in creating the new version (ideally done in a way that is not breaking, and then in the next major release we will deprecate the findDOMNode() version. Heads up I am on leave all of Dec so probably won't review until Jan.
Hi, what's the current status on this? I see this for example when I run the example at: https://airbnb.io/visx/areas
Is it recommended that we use a Portal for everything and use useTooltipInPortal even if we aren't running into any z-index issues?
Bump, still seeing findDOMNode was passed an instance of withBoundingRects() console warnings
