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

Migrate from findDOMNode to refs #13221

Closed
10 tasks done
TrySound opened this issue Oct 12, 2018 · 8 comments
Closed
10 tasks done

Migrate from findDOMNode to refs #13221

TrySound opened this issue Oct 12, 2018 · 8 comments
Assignees
Labels
priority: important This change can make a difference

Comments

@TrySound
Copy link
Contributor

TrySound commented Oct 12, 2018

React team just finally announced deprecating findDOMNode.

facebook/react#13841

Would be good to start providing better refs in each component to get rid from RootRef.

Edit @eps1lon: Hijacking OP to document progress.

If we need the DOM node we still use findDOMNode but we call it with refs. If the refs are attached to host components you won't get a strict mode warning. Only if they are attached to class components.

Strict-mode ready findDOMNode
@eps1lon
Copy link
Member

eps1lon commented Oct 12, 2018

I believe this would be a breaking change. The API allows in many cases that users pass a custom root component. We still need findDOMNode if those components don't forward their refs properly.

@oliviertassinari
Copy link
Member

This is such a big move from React. For a long time, I thought they weren't going to execute on this. From what I understand, the motivation is around simplifying React internal core. This makes sense. Let's see how we can fulfill this new constraint.

@eps1lon
Copy link
Member

eps1lon commented Oct 24, 2018

React 16.6 will log deprecation warnings in strict mode about this. Will be good way to gauge the impact of this change

@TroySchmidt
Copy link
Contributor

This is also important because React recommends passing StrictMode before enabling concurrent React. It is unstable right now but should be finalized this year.

@eps1lon
Copy link
Member

eps1lon commented Apr 4, 2019

This is also important because React recommends passing StrictMode before enabling concurrent React. It is unstable right now but should be finalized this year.

This is the actual driving factor for us. We've made some progress but react-transition-group will probably be the last remaining blocker. Unless we fork it but it's quite a bit of bundle bloat we would introduce if react-transition-group is used in other parts of the app bundle.

@eps1lon
Copy link
Member

eps1lon commented Jun 14, 2019

findDOMNode remains in the codebase for backwards compatibility with class components. If you forward refs to host instances where necessary you should get no more deprecation warnings in StrictMode.

For more details see https://material-ui.com/guides/composition/#caveat-with-refs

@eps1lon eps1lon closed this as completed Jun 14, 2019
@NMinhNguyen
Copy link
Contributor

NMinhNguyen commented Jun 15, 2019

@eps1lon

you should get no more deprecation warnings

Is that because findDOMNode(validElementRef) is safe?

backwards compatibility with class components

Can't class components be wrapped in forwardRef as well? Or do you mean that you don't want yet another breaking change and for existing class components (that do not correctly forward refs) to work?

@eps1lon
Copy link
Member

eps1lon commented Jun 15, 2019

Or do you mean that you don't want yet another breaking change and for existing class components (that do not correctly forward refs) to work?

Yes. Just reducing the amount of breaking changes.

Is that because findDOMNode(validElementRef) is safe?

At least for now. We're still waiting for an official confirmation but so far findDOMNode doesn't throw if called with host instances.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: important This change can make a difference
Projects
None yet
Development

No branches or pull requests

5 participants