Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ReactDOM] Add strict mode test for findDOMNode #15091

Closed

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Mar 12, 2019

#13841 deprecated presumably "all usage" of findDOMNode. This is not the case if passed a host component1. I wanted to verify if this is intended or not since we have a use case for not issuing a deprecation warning for host components:

We have a component library that lets the user pass a custom component that is rendered. It can be a different DOM node or an actual component:

class LibraryComponent() {
  // this is incomplete. It's just one instance where we get it
  componentDidMount() {
    this.button = ReactDOM.findDOMNode(this);
    listenForFocusKeys(ownerWindow(this.button));

    if (this.props.action) {
      this.props.action({
        focusVisible: () => {
          this.setState({ focusVisible: true });
          this.button.focus();
        },
      });
    }
  }

  render() {
    const { as: Component } = this.props;
	return <Component />
  }
}

However we required the DOM node of the rendered component for focus handling (essentially adding a focus-visible polyfill). Before deprecation we used findDOMNode(this). Now that findDOMNode is deprecated we want to move away from that usage while enabling some backwards compatibility:

class LibraryComponent() {
+ buttonRef = React.createRef();
  // this is incomplete. It's just one instance where we get it
  componentDidMount() {
-   this.button = ReactDOM.findDOMNode(this);
-   listenForFocusKeys(ownerWindow(this.button));
+   listenForFocusKeys(ownerWindow(this.getButtonNode()));

    if (this.props.action) {
      this.props.action({
        focusVisible: () => {
          this.setState({ focusVisible: true });
-         this.button.focus();
+         this.getButtonNode().focus();
        },
      });
    }
  }
  
+ getButtonNode() {
+   return ReactDOM.findDOMNode(this.buttonRef.current);
+ }

  render() {
    const { as: Component } = this.props;
-	return <Component />
+	return <Component ref={this.buttonRef} />
  }
}

This change:

  1. is breaking usage with function components <LibraryComponent as={SomeFunctionComponent} />
  2. strict mode compatible with <LibraryComponent as="div" /> or any other host component or ref forwarding component
  3. not strict mode ready but also not breaking for <LibraryComponent as={SomeClassComponent} />

The test added with this PR is basically ensuring the third behavior.

Followup on #13841 (comment)

1 Not sure if this is the correct term.


// outside of dev toWarnDev would always pass which means negating it would always fail
if (__DEV__) {
expect(() => ReactDOM.findDOMNode(child)).not.toWarnDev(['**']);
Copy link
Collaborator Author

@eps1lon eps1lon Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this also match a potential future warning? I had the full message originally but I'm not sure how it would actually look like

Suggested change
expect(() => ReactDOM.findDOMNode(child)).not.toWarnDev(['**']);
expect(() => ReactDOM.findDOMNode(child)).not.toWarnDev(['Warning: findDOMNode is deprecated in StrictMode.**']);

@sizebot
Copy link

sizebot commented Jun 15, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@eps1lon eps1lon force-pushed the test/findDOMNode-host-strict-mode branch from 3796d53 to 6b01aa1 Compare October 1, 2019 12:56
@sizebot
Copy link

sizebot commented Oct 1, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 6b01aa1

@stale
Copy link

stale bot commented Jan 9, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 9, 2020
@eps1lon
Copy link
Collaborator Author

eps1lon commented Jan 10, 2020

Still active

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@eps1lon eps1lon force-pushed the test/findDOMNode-host-strict-mode branch from 6b01aa1 to 041b47e Compare January 26, 2020 22:12
@sizebot
Copy link

sizebot commented Jan 26, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 2469628

@sizebot
Copy link

sizebot commented Jan 26, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 2469628

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 27, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2469628:

Sandbox Source
distracted-night-8gf82 Configuration

Unsure if we won't get false positves if the error message changes
slightly
@eps1lon
Copy link
Collaborator Author

eps1lon commented Apr 14, 2020

Closing due to lack of interest. We test this compatibility layer in our own code base on React canaries.

@eps1lon eps1lon closed this Apr 14, 2020
@eps1lon eps1lon deleted the test/findDOMNode-host-strict-mode branch April 14, 2020 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants