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

Use forward refs in Components to access ref instances #3819

Open
mxschmitt opened this issue Oct 27, 2019 · 35 comments · May be fixed by #4233
Open

Use forward refs in Components to access ref instances #3819

mxschmitt opened this issue Oct 27, 2019 · 35 comments · May be fixed by #4233

Comments

@mxschmitt
Copy link

mxschmitt commented Oct 27, 2019

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.

@mxschmitt mxschmitt changed the title Upgrade Use forward refs in Components to access ref instances Oct 27, 2019
@mxschmitt mxschmitt changed the title Use forward refs in Components to access ref instances Use forward refs in Components to access ref instances Oct 27, 2019
@patrickp-dev
Copy link

patrickp-dev commented Jan 17, 2020

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
Copy link

jacobweber commented Feb 6, 2020

@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
Copy link

patrickp-dev commented Feb 14, 2020

@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
Copy link

jacobweber commented Feb 14, 2020

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

@layershifter
Copy link
Member

layershifter commented Jul 16, 2020

Summary:

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

@layershifter
Copy link
Member

layershifter commented Aug 1, 2020

#3915 (comment)

@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
Copy link
Member

layershifter commented Aug 1, 2020

@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
Copy link
Member

brianespinosa commented Aug 3, 2020

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
Copy link
Author

mxschmitt commented Aug 3, 2020

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

@layershifter
Copy link
Member

layershifter commented Aug 4, 2020

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

@layershifter
Copy link
Member

layershifter commented Aug 10, 2020

  "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
Copy link
Member

layershifter commented Aug 17, 2020

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

@layershifter
Copy link
Member

layershifter commented Oct 2, 2020

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)
  • convert existing class components to be functional & use React.forwardRef()
  • deprecate Ref component

@sparrowV
Copy link

sparrowV commented Nov 7, 2020

@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?

@dvtkrlbs
Copy link

dvtkrlbs commented Dec 4, 2020

Are there any progress on this ?

@YuriFontella
Copy link

YuriFontella commented Jan 18, 2021

I didn't understand if this had any solution, but anyway. I'm trying to use the react-hook-form.

Directly with input this works:

const {register} = useForm ()
<input name="name" ref={register} />

Using the component does not work:

const {register} = useForm ()
<Form.Input name="name" ref={register} />

"Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?"

@layershifter
Copy link
Member

layershifter commented Jan 18, 2021

@YuriFontella

const {register} = useForm ()
<Form.Input name="name" input={{ ref: register }} />

Should work for your case

@layershifter
Copy link
Member

layershifter commented Jan 18, 2021

@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?

Warnings discussed in this PR are coming from SUIR, there is nothing that you can do with it until changes will be done on library side.

@YuriFontella
Copy link

YuriFontella commented Jan 18, 2021

Thanks @layershifter.

And how would it look on other form components?

Form.TextArea
Form.Checkbox
Form.Select

I was using html instead of components, but the select totally loses the style.

@brandonm
Copy link

brandonm commented Jan 18, 2021

This question is probably better off for StackOverflow but for components that don't have a native element to ref react-hooks-form provides Controller. Here is an example for something I needed:

        <Controller // react-hook-form (validation)
            control={control}
            name={name}
            defaultValue={value}
            rules={fieldValidators}
            render={({ name: renderName, value: renderValue, onChange }) => (
                <Form.Field error={error}>
                    <label htmlFor={name}>{label}</label>
                    <Select
                        className={className}
                        name={renderName}
                        defaultValue={renderValue}
                        placeholder={placeholder}
                        options={options}
                        error={error}
                        icon={icon}
                        onChange={(event, { value: newValue }) => onChange(newValue)}
                    />
                    {error && (
                        <Label prompt className="ui pointing above prompt">
                            {error.message}
                        </Label>
                    )}
                </Form.Field>
            )}
        />

@YuriFontella
Copy link

YuriFontella commented Jan 18, 2021

Edit: Using the Controller with Select and Checkbox.

<Controller
  name="type"
  control={control}
  defaultValue=""
  render={props => 
    <Form.Select 
      onChange={(e, { value }) => props.onChange(value)}
      value={props.value}
      options={[
        { text: 'Privada', value: 'privada' },
        { text: 'ONG', value: 'ong' },
        { text: 'Independente', value: 'independente' },
        { text: 'Sociedade', value: 'sociedade', },
        { text: 'Governamental', value: 'governamental' }
      ]}
    />
  }
/>
<Controller
  name="privacy"
  defaultValue=""
  control={control}
  render={props => 
    <Form.Checkbox radio onChange={(e, { value }) => props.onChange(value)} checked={props.value === 'public'} value="public" />
  }
/>

@ZachHaber
Copy link

ZachHaber commented Feb 4, 2021

I had to go a bit further on an application I was working on. I was running into errors because I needed to make the focus exposed to react hook form, plus the added benefit of having it scroll to the offending field is really nice.

It mostly involved similar things, but in this case, I ended up performing a query selector for the Select's input to focus on it. The others I made a wrapper for, I could just use the ref provided by the component

https://gist.github.com/ZachHaber/7f443450bb83c92d2c8dbc74dd936f2a

@sholladay
Copy link

sholladay commented Feb 11, 2021

FYI, with v7 of React Hook Form (which is in beta right now), this situation is going to get slightly worse, in that the usage of refs is sort of implicit and hidden.

With version 7, instead of explicitly passing register into the ref prop, we're supposed to call register, which will return an object of the shape { name, onBlur, onChange, ref } and we're supposed to spread this object into the component.

Here's how it's supposed to work:

import React from 'react';
import { useForm } from 'react-hook-form';
import { Form } from 'semantic-ui-react';

const MyForm = () => {
    const { handleSubmit, register } = useForm();

    return (
        <Form onSubmit={handleSubmit(onSubmit)}>
            <Form.Input
                id="firstName"
                label="First Name"
                {...register('firstName')}
            />
            <Form.Button content="Submit" />
        </Form>
    );
};

But because <Form.Input> doesn't forward the ref to the underlying <input>, this won't work.

It seems like input={register('firstName')} does work, although I'm not clear on whether this is entirely correct to do. For example, that means the name prop will be passed directly to the input instead of via the form field and I'm not sure whether or not the field uses name for anything. Same goes for the event handlers.

@neutraali
Copy link

neutraali commented Apr 1, 2021

To add to the above, a similar scenario is with <Form.Field>. Using shorthand and trying to pass a ref to the underlying Component won't really pass anything down:

let input = React.useRef();
<Form.Field
	required
	control={CustomInput}
	ref={input}
	label='Required field:'
	name='basic-input'
/>
// input: { current: undefined  }

@brianespinosa
Copy link
Member

brianespinosa commented Apr 5, 2021

@neutraali in your example, you would want to attach your ref to your CustomInput component and not the Form.Field wrapper. Even with React.forwardRef you would still need to attach to your input as your ref in your use case.

@sholladay
Copy link

sholladay commented Apr 5, 2021

@brianespinosa That doesn't seem right. Why couldn't FormField forward the ref to a custom control?

@Hexavolus
Copy link

Hexavolus commented Apr 6, 2021

Is there a list anywhere of which components currently forward correctly and which ones need to be updated? Otherwise it's a bit inconvenient to have to try out or look through all the code for each component.
The Ref documentation seems to imply that you should just use Ref everywhere for now, for conveniences sake.

@brianespinosa
Copy link
Member

brianespinosa commented Apr 6, 2021

Why couldn't FormField forward the ref to a custom control?

@sholladay If I am creating a custom input and place a ref on the FormField and not the custom input... I would be doing that because I explicitly want the ref on the FormField and not the custom input. This is a similar pattern with controlled components. The moment you reach in to component state, we can no longer reliably know what you want to do and you need to control it on your own.

Is there a list anywhere of which components currently forward correctly and which ones need to be updated?

@Hexavolus I do not think this exists. @layershifter was working on another major version bump that would encompass changing the rest of these warnings but it is not done yet. There will be updates here on this ticket when that does get released.

@sholladay
Copy link

sholladay commented Apr 6, 2021

@Hexavolus I don't think any SUIR components currently implement forwardRef. That's really what this issue is all about, taking the first step.

@Hexavolus
Copy link

Hexavolus commented Apr 6, 2021

Ah, my mistake. The documentation said not all components support native ref forwarding which sounded like some did. No worries.

I assume this commonly comes up due to react-hook-form which does provide the Controller component and useController as a workaround for now.

@sholladay
Copy link

sholladay commented Apr 7, 2021

Yeah, RHF is awesome and has become very popular, so that's a notable use case.

Aside from that, there's also file inputs, where you need to use a ref to get access to the file for uploading, etc.

dariodjuric pushed a commit to dariodjuric/github-user-search that referenced this issue Apr 21, 2021
Otherwise "findDOMNode is deprecated in StrictMode" warning is displayed in the console.

Related issue: Semantic-Org/Semantic-UI-React#3819
@fourpastmidnight
Copy link

fourpastmidnight commented May 19, 2021

I love Semantic UI React, and I, too, was having trouble integrating it with react-hook-form v7. But I recently found some good examples online. The example shown above in this thread, wrapping the SUIR compnoent with a RHF Controller component is exactly the right solution, but it is missing one thing that I did find in the RHF documentation (but didn't realize what I was looking at) and an online example (that helped me understand what I was seeing in the documentation):

<Form.Field>
  <Controller
    name="username"
    control={control}
    render={({field: { ref, ...rest }, fieldState }) => {
      return <Form.Input
        label='Username'
        icon='user'
        iconPosition='left'
        {...rest}
        {...fieldState}
        required error={errors.username ? true : false}
      /> }
    rules={{ required: true}}
  />
</Form.Field>

By destructuring the field property of the UseControllerProps, we can separate out the ref property of the field property so it doesn't get passed along, avoiding the warning message about functional components not being able to accept refs. Note that I didn't even require an onChange method (in this case--as I was not doing any value transformation, i.e. string to number).

If I had a lot of forms and/or form elements, wrapping these all the time would be tedious. But RHF does provide the useController hook. The idea is you could wrap, for example, a SUIR component in your own HOC and utilize useController, and then consume that HOC in your form where everything is already wired up.

I spent a good day trying to find this solution and figure out how it worked (I'm a DevOps guy--not a FE dev! 😋 So it took me a bit longer to figure this out). Anyway, HTH.

It would be nice if we could just forward the ref--and if I'm reading this issue correctly, that work is underway. That'll be a great addition to this library. Thanks for all the hard work. It has been GREAT working with SUIR.

@DamonBlais
Copy link

DamonBlais commented Jul 16, 2021

The only thing that still throws this deprecation error in StrictMode for me out of semantic-ui-react is use of the SidebarPusher and Sidebar having to find each-other without an explicit ref being set/sent anywhere. It still works, but has to rely on findDomNode to do so.

Is there a work-around for this specific issue at the moment?

I've found this thread great for information on getting around the other issues involving libraries that expect you to pass ref about.

@layershifter
Copy link
Member

layershifter commented Jul 23, 2021

You can follow #4233 for progress 🎉

@bonellia
Copy link
Contributor

bonellia commented Aug 25, 2021

@fourpastmidnight Thank you very much for the example. I can confirm it works fine with RHF v7. I have been stuck here, getting slowly depressed how can I not just create a form using Semantic and RHF for over a week. I was trying to find a solution checking online (mostly outdated) examples and RHF issues. Apparently this ref-related issue was known on the Semantic side. Good thing people are working on it :)

I was wondering if you could provide examples with select and checkboxes. I should be able to test them by modifying the snipper you provided, but a working example would be appreciated immensely, because I don't wanna give up on Semantic even for forms alone.

Thanks in advance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.