findDOMNode produces warnings in Strict Mode

[jonkoops]

When Strict Mode is enabled Patternfly creates deprecation warnings due to the usage of findDOMNode.

This is deprecated functionality and the recommendations to handle this deprecation as stated in the React documentation should be followed.

Actual behavior

Patternfly creates the following exception:

Expected behavior

Patternfly does not produce any exceptions.

Steps to reproduce

Enable Strict Mode in an application running Patternfly.

[redallen]

FindRefWrapper is needed because we don't know what element you will pass to Popper. We could require you pass a component that forwards its ref, but that's a breaking change.

Maybe there's another creative way to grab refs @jschuler ?

[jonkoops]

@redallen Perhaps this might help: https://reactjs.org/docs/hooks-reference.html#useimperativehandle

[AlexSCorey]

I am having this same warning that comes from <PageHeaderToolbarItem>. I've tried all of the suggested fixes and none of them seem to work

[jonkoops]

Note that this behavior could be enforced using an ESLint rule.

[redallen]

Again, if there's a way to remove findDOMNode without breaking breaking existing components I'm all for it, but it's not as simple as "just use refs" or "stop using // eslint-disable-next-line react/no-find-dom-node."

These are the places we currently use findDOMNode:

packages/react-core/src/components/Dropdown/DropdownMenu.tsx:133:59:        this.refsCollection[index][innerIndex] = ReactDOM.findDOMNode(node) as HTMLElement;
packages/react-core/src/helpers/util.ts:182:30:    const element = ReactDOM.findDOMNode(refsCollection[nextIndex]) as HTMLElement;
packages/react-core/src/helpers/Popper/FindRefWrapper.tsx:11:29: * It has to be a class for findDOMNode to work
packages/react-core/src/helpers/Popper/FindRefWrapper.tsx:21:27:    const root = ReactDOM.findDOMNode(this);

Keeping findDOMNode as a last resort in DropdownMenu.tsx and util.tsx is fine. You should see warnings if we can't grab your refs normally. However it's more difficult for places we use FindRefWrapper like Popper because it allows passing any trigger and any menu which it then has to grab refs from.

Is there a reason the Popper's trigger and menu can't be wrapped with divs that have normal refs @jschuler ? I might be able to try it out this afternoon.

[jonkoops]

Again, if there's a way to remove findDOMNode without breaking breaking existing components I'm all for it, but it's not as simple as "just use refs" or "stop using // eslint-disable-next-line react/no-find-dom-node."

I was operating under the assumption that this rule had been disabled for the entire repo, my bad. I'd love to spend some time helping out to fix these issues, but unfortunately my work on Keycloak is keeping me busy,

[kunyan]

I think the Popper should be provide Render Props to allow user pass the ref by theirselves

A example is react-virtualized, they use registerChild

import React from 'react';
import ReactDOM from 'react-dom';
import { List, WindowScroller } from 'react-virtualized';
import 'react-virtualized/styles.css'; // only needs to be imported once

ReactDOM.render(
  <WindowScroller>
    {({ height, isScrolling, registerChild, scrollTop }) => (
      <div>
        <header>
          Table header
        </header>
        <div ref={registerChild}> {// User could send ref here}
          <List
            autoHeight
            height={height}
            isScrolling={isScrolling}
            rowCount={...}
            rowHeight={...}
            rowRenderer={...}
            scrollTop={scrollTop}
            width={...}
          />
        </div>
      </div>
    )}
  </WindowScroller>,
  document.getElementById('example')
);

The Popper usage could be

<Popover
  headerContent={<div>This is header content</div>}
  bodyContent={<div>This is body content</div>}
  isVisible={isVisible}
  position="auto"
>
  {({ registerChild }) => (
    <div ref={registerChild}>
      <Button>Hover me</Button>
    </div>
  )}
</Popover>;

The children props of Popper could be a React.Node or (props: { registerChild} => React.Node)) => void

If User provide a function as children, just use their ref,
If User only use a Element, findDOMNode could be as a fallback solution

references:

https://github.com/bvaughn/react-virtualized/blob/master/source/WindowScroller/WindowScroller.js

[redallen]

Yes, that's an option, but it's a breaking change. We currently export <Popper> to consumers.

[kunyan]

Well, I don't think is a breaking change, it just provide another type of children of <Popper>, provide another way to the user who want to prevent the warning.
It won't break the existing usage, just add a new :)

like:

Old way;

<Popper> need a ReactNode as children, it will using findDOMNode.

New way:

<Popper> accept a ReactNode or a function as children, it will using findDOMNode if get a ReactNode children, will handle ref by user if they send a function as children

[kunyan]

btw, I didn't check more details, but findDOMNode had been marked as deprecated, it means should be abandoned soon or later. The component library should be turn around in advance to adapt upcoming changes

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[jonkoops]

Not stale.