-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Remove forwardRef references from useRef and Manipulating the DOM with Refs pages #7364
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Size changes📦 Next.js Bundle Analysis for react-devThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
|
||
However, if you try to put a ref on **your own** component, like `<MyInput />`, by default you will get `null`. Here is an example demonstrating it. Notice how clicking the button **does not** focus the input: | ||
When you put a ref on a built-in component that outputs a browser element like `<input />`, React will set that ref's `current` property to the corresponding DOM node (such as the actual `<input />` in the browser). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing. You need to walk through these stages:
- Passing a ref to
<input>
- Creating your own component — it doesn't work anymore
- Fixing it by spreading props
Spreading props isn't a default thing people would reach for.
The paragraph above doesn't match the code below because the code below introduces spreading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it might be better to remove spreading from examples below and just do ref={ref}
so that it's obvious it works like any other prop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
const MyInput = forwardRef((props, ref) => { | ||
const MyInput = (props, ref) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ref is a prop now, so it won't be a second argument. I'd suggest to directly pass it as a prop in this example and remove spreading to make the pattern very clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
## Troubleshooting {/*troubleshooting*/} | ||
|
||
### I can't get a ref to a custom component {/*i-cant-get-a-ref-to-a-custom-component*/} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still a problem. You'll just get a ref with undefined
current value and no warning. We need to change the wording of this but the problem is real and needs to be described.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above:
- let's fix the examples to show passing
ref
directly without spreading - fix code example that has second argument, it's wrong
- keep the troubleshooting section but reword to match the new problem
74581f3
to
5083ad6
Compare
5083ad6
to
649211b
Compare
Fixed! |
const MyInput = forwardRef((props, ref) => { | ||
return <input {...props} ref={ref} />; | ||
}); | ||
const MyInput = ({ref}) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we generally put spaces around destructuring like ({ ref })
.
the rest of the docs also uses a function declaration style, like function Input({ ref })
, no need for an arrow here
import { useRef } from 'react'; | ||
|
||
function MyInput(props) { | ||
return <input {...props} />; | ||
function MyInput({ref}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same nit
@@ -343,75 +343,39 @@ Read more about [how this helps find bugs](/reference/react/StrictMode#fixing-bu | |||
|
|||
## Accessing another component's DOM nodes {/*accessing-another-components-dom-nodes*/} | |||
|
|||
When you put a ref on a built-in component that outputs a browser element like `<input />`, React will set that ref's `current` property to the corresponding DOM node (such as the actual `<input />` in the browser). | |||
<Pitfall> | |||
Refs are an escape hatch that should be used sparingly. Manually manipulating _another_ component's DOM nodes makes your code even more fragile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so i'm wondering if this is a bit too strong here... we're describing legit cases on this page (like focusing) but this makes it sound like even the stuff below is fragile
</button> | ||
</> | ||
); | ||
return <><MyInput ref={inputRef} /></> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this <>
fragment isn't doing anything here
const MyInput = forwardRef((props, ref) => { | ||
return <input {...props} ref={ref} />; | ||
}); | ||
function MyInput({ref}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: spacing
|
||
const MyInput = forwardRef((props, ref) => { | ||
const MyInput = ({ ref }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should just be a function declaration now, no need for arrow
function MyInput({ ref }) {
// ...
}
|
||
const MyInput = forwardRef(({ value, onChange }, ref) => { | ||
```js {1,6} | ||
const MyInput = ({ value, onChange, ref}) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should remain a function declaration, no need to change it to an arrow
export default function MyInput({ value, onChange, ref }) {
// ...
}
also don't miss the space after ref
, it should be ref })
rather than ref})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good if nits are addressed. please have a look at #7332 which was aiming to do the same — maybe something can be synthesized if you like any of those edits, these PRs seem similar in goals
Update the "useRef" and "Manipulating the DOM with Refs" pages to no longer use forwardRef since its no longer needed in React 19