Skip to content

Conversation

mjpsyapse
Copy link

This change removes the deprecated findDOMNode function.

@mjpsyapse mjpsyapse changed the title Remove findDOMNode fix: Remove findDOMNode Jun 6, 2019
@coveralls
Copy link

Coverage Status

Coverage increased (+4.9%) to 73.864% when pulling 243c5c5 on mjpsyapse:findDOMNode into 8418bba on react-component:master.

@mjpsyapse
Copy link
Author

tl;dr Don't merge this.

I have been exploring the entire antd / react-component ecosystem, and findDOMNode is used heavily, which makes this migration require a lot of untangling, if not rewriting. For example, rc-animate should be replaced with react-transition-group. In general, I'd say that react-components and antd try to wrap existing libraries that do DOM manipulation with react (like rc-align). It will be a large shift to refactor out all this history.

We need to figure out a strategy for moving forward or else antd will be left behind as react adds new features. i.e. no suspense, no concurrent react, and whatever else comes next.

@afc163 afc163 requested a review from zombieJ June 17, 2019 06:01
@zombieJ
Copy link
Member

zombieJ commented Jun 17, 2019

This can not get real dom node:

class MyComponent {
  render() {
    return (<div {...this.props} />);
  }
}

findDOMNode will get instance instead and makes getBoundingClientRect failed.

In low level api, findDOMNode seems still necessary since user may not pass real dom ref out.

But for animate level, yes, we have plan to switch to more react way.

@mjpsyapse
Copy link
Author

@zombieJ Yes, I agree that this is a breaking change. And I agree that findDOMNode is useful, but if it's deprecated, there should be a plan to remove it. Right?

@zombieJ
Copy link
Member

zombieJ commented Jun 18, 2019

Here are some info by React doc: https://reactjs.org/docs/forwarding-refs.html#forwarding-refs-in-higher-order-components

User should always pass dom with forwardRef and then findDomNode can be removed. But it also makes that if instance do provides some functions breaks. Or may require user use render props return node & ref.
Still thinking of it since it's a hard decision.

@mjpsyapse
Copy link
Author

Closing this since it will never get merged.

@mjpsyapse mjpsyapse closed this Nov 26, 2019
@mjpsyapse mjpsyapse deleted the findDOMNode branch November 26, 2019 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants