-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[docs] Remove unnecessary findDOMNode usage #14836
Conversation
No bundle size changes comparing a5b8011...67a300e |
@@ -31,10 +30,10 @@ function NativeSelects() { | |||
name: 'hai', | |||
}); | |||
|
|||
const inputLabelRef = React.useRef(null); |
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.
We should perhaps agree a pattern here. I find having Ref
in the name adds clarity. when viewing the variable in isolation. Admittedly .current
is a big giveaway, but when it's an actual component ref, I think it has merit. Similarly, having State
in the name of state hooks...
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 far I have seen *Ref
used for ref objects and for return values from findDOMNode
. I don't mind adding *Ref
to return values from createRef
. But it doesn't make much sense to do something like ref={ref => (containerRef = React.findDOMNode(ref))}
.
I just think that it doesn't add much value if I append to every variable that gets some (create|use)Ref
return value the *Ref
suffix. At that point it only adds noise but no information.
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.
I have been suffixing all the ref variables with Ref
. So no surprises here for me. I would vote for keeping it. But I don't have a strong opinion on the subject.
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.
I would vote for keeping it.
Keep what? *Ref
for (create|use)Ref
or *Ref
for anything that is a reference to some object?
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.
I would vote for the former...
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 for (create|use)Ref
@eps1lon Yes. My motivation is that it's an object we are mutating, it's not immutable. It's an important distinction.
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.
My motivation is that it's an object we are mutating, it's not immutable. It's an important distinction.
That's not quite correct though. Every write to an instance field is mutating an object (this
). I guess we can settle on "every var that can be handled by react via ref
should have the Ref
suffix"
// valid
<Component ref={ref => (this.someField = ref)} />
<Component ref={this.someRef} />
// invalid
<Component ref{ref => (this.notARef = ref)} />
<Component ref{ref => (this.containerRef = ReactDOM.findDOMNode(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.
Ok, if you feel it's better without the prefix we can remove it. I was optimizing for class components. In the hook realm, doing:
const containerRef = React.useRef();
allow us to make a distinction with the over variables. I'm seduced by this pattern. We can't confound a ref variable, that wraps a mutable variable, with an immutable variable that might comes, e.g. from a prop or useState.
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.
Well in that context it makes sense. I was mostly thinking about usage in this
. For example
// containerRef is not mutable the same way as `React.createRef` would be mutable.
// The value can be changed but the reference not. Using the `Ref` suffix here is misleading
const { containerRef } = this;
<Component ref={ref => (this.containerRef = ReactDOM.findDOMNode(ref))} />
bcb0468
to
7f77419
Compare
7f77419
to
67a300e
Compare
Using some goodies from #13676 and #14536.
docs/
is free offindDOMNode
.