Semantic-ui-react: Use forward refs in Components to access ref instances

[mxschmitt]

Feature Request

Problem description

Currently, not all of components properly forward the passed in ref prop to the underlying component. This leads to end-users not being able to access the underlying DOM node and execute their browsers native functions.

For example when you try to execute checkValidity on the form element itself.

Proposed solution

Upgrade the components to forward refs: https://reactjs.org/docs/forwarding-refs.html

MVP

https://codesandbox.io/s/naughty-solomon-fqk4f?fontsize=14

Here you see who examples, the current <Form> component and the native browsers one. The ref instances are totally different.

[patrickp-dev]

Here's a workaround that takes advantage of semantics 'as' prop and prop forwarding to give you access to the rendered DOM element. It'll work with any semantic components, you just need to pass in the render type, which in most cases will probably be the default.

https://codesandbox.io/s/semantic-ui-react-refs-workaround-ibnnv

[jacobweber]

@patrickp-dev Thanks for the workaround, but it seems to be causing some bad side-effects for me. I added an controlled Input and a useState hook to your example:

https://codesandbox.io/s/semantic-ui-react-refs-workaround-2hr0e

But with this workaround, the input loses focus after typing.

Not sure what's happening, but I'm wondering if there's any other way to work around this.

(Use case: I'm trying to capture keyboard events in a Modal, which means I need to focus() the modal's div when it's rendered. So I need a ref to focus. I saw some mention of a Ref component in other tickets, but it seems to be removed.)

[patrickp-dev]

@jacobweber Hey jacob, had a look at your example. The lose of input focus was caused by the fact the 'as' prop was being reassigned a new instance of the higher order component that was returned by the forwardRefAs on each rerender, (my bad). All that's required is pulling the HOC instantiation out of the 'App' component so that it doesn't remount the entire form on each state change. I've updated your example to get it working and have update my original example. https://codesandbox.io/s/semantic-ui-react-refs-workaround-h0f64

[jacobweber]

@patrickp-dev Thanks! I'll give it a try.

[layershifter]

Summary:

• React.forwardRef is only a solution. All components should forward refs properly
• Ref component should removed and deprecated

[layershifter]

https://github.com/Semantic-Org/Semantic-UI-React/issues/3915#issuecomment-665188625

@levithomason should we update the docs to mention that since we have a full major version release that we want to stick to semantic versioning while in maintenance?

Probably we should put a this notice about Semver or a badge in README.md.

@layershifter I think this would also have to be a breaking major version change now that we officially published at 1.0.0 version.

@brianespinosa It's a good question.

I don't think that it will be breaking change for function components as they don't have refs at all. For class components it's it will a breaking change as third party code may rely on some private class methods. What are you suggestions around it?

[layershifter]

@mxschmitt BTW, for the original issue. Ref component can do what you want:

<Ref innerRef={testRef}>
  <Form />
</Ref>

https://codesandbox.io/s/semantic-ui-react-refs-issue-u7zpv?file=/src/index.js

[brianespinosa]

I don't think that it will be breaking change for function components as they don't have refs at all. For class components it's it will a breaking change as third party code may rely on some private class methods. What are you suggestions around it?

I think now that we have released a full version we should stick with semantic versioning as I think that will be everyone's expectation once a full major version is out. I think it is also React team's recommendation for library authors that we should treat this as a breaking change and release a new major version.

[mxschmitt]

@layershifter looks great! For me it would be fine with closing this issue. 👍

[layershifter]

@mxschmitt I will keep this issue opened for tracking work related to React.forwardRef() changes as the huge amount work should be done 🐤

[layershifter]

  "version": "17.0.0-alpha.0",

https://github.com/facebook/react/blob/2704bb5374e52ed548db96df2d975dae42158dfb/packages/react/package.json#L7
This is a good motivation for changes. Going to start there soon.

[layershifter]

FYI: this #4039 PR restores docs for Ref component and clarifies when it should be used.

[layershifter]

Small update there. SUIR 2.0.0 was released yesterday and I started to work on this item. I will stage a v3 branch soon.

• [ ] do not use .state() in Enzyme (_done, waits for PR_)
• [ ] make existing functional components to use React.forwardRef() (create a list)
• [ ] convert existing class components to be functional & use React.forwardRef() (create a list)
• [ ] deprecate Ref component

[sparrowV]

@layershifter so in order to take care of that warning we should use functional components and React.forwardRef() ? BTW does this warning implies any security/performance issues? Is it ok to leave it in production code?