-
Notifications
You must be signed in to change notification settings - Fork 47.7k
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
ref
callback attribute not behaving as expected
#6249
Comments
This is intended (discussed elsewhere) but rather unintuitive behavior. Every time you render: <Value ref={(e) => { if (e) { console.log("ref", e); }}} /> You are generating a new function and supplying it as the ref-callback. React has no way of knowing that it's (for all intents and purposes) identical to the previous one so React treats the new ref callback as different from the previous one and initializes it with the current reference. PS. Blame JavaScript :P |
Oh, I see. So that's simple to work around, then, thanks! Maybe the example in the docs should be updated, because it's now also using an inline callback to focus an element, which would have some weird effect on every render. I'm closing the issue though, since my issue is resolved. |
I am confused, what's an easy work-around for this issue? Even if I do something like that Since the functions have to be bound to access
and then doing This kind of mess makes I've made a jsbin of the issue here: https://jsbin.com/poboqo/1/edit?html,js,output |
@vitalybe I'm guessing this is why many React examples that use ES6 classes use trailing/leading underscores for method names, and bind them in the constructor to |
With ES6 class properties and arrow functions, you can do this: class MyComponent extends React.Component {
callback = () => {
console.log(this)
}
render () {
return <div ref={this.callback} />
}
} Here the constructor () {
this.callback = () => {
console.log(this)
}
} |
@vitalybe What is your use case for refs? If you just set an instance field it should make absolutely no difference whether it is reattached on every render or not. |
@goto-bus-stop Thanks for the snippet, that indeed looks better. @gaearon The problem was that I was causing, by proxy, a We've mentioned several solutions to this issue on the thread, I think that the main point, however is that its current behaviour is rather unexpected. |
We should probably throw early if you setState from a ref callback. I don't think this is meant to be a supported pattern. |
@syranide Whoa. 🍻 to you! So I should probably refactor all my code that looks like: <div ref={node => node && this.node = node}> |
@ffxsam On the contrary, we give you |
Oh wait, I misunderstood your comment. Yes, you need to refactor from that pattern to a simple: <div ref={node => this.node = node}> |
<div ref={node => this.node = node}> This doesn't work well for me though. When I use this pattern, as the OP said, the ref callback fires off every time the component renders. I should mention, that div is the top-level BTW: render() {
return <div ref={node => this.node = node}> I don't know if that's the reason I was getting repeated ref callback invocations? |
Why is this a problem for you? This is the expected behavior. |
In case you’re worried about bottlenecks, it is extremely unlikely that setting a field is a performance bottleneck in your app. In the extreme case when it is, you can hoist it to be a bound class method (just like you do with event handlers), and you’ll avoid extra calls. |
I understand why it's getting called upon re-renders. Sort of.. I'll get back to that in a second. What you suggest ( What I don't understand is why the ref callback is called upon re-render. I get that it's a new function every time, but shouldn't the determining factor be whether the DOM node has already mounted or not? In other words, let's say I have this: render() {
return <div>
<img ref={() => blahblah('abc')} src="..." />
</div>
} Rather than the internal React logic saying (upon re-render), "Hey, this is a new function, I should call it" - shouldn't it instead say "I've already invoked a ref callback for img, so I won't do it again"? |
Could you just do it in
Imagine this case: <img ref={this.props.isAvatar ? this.handleAvatarRef : this.handleNormalRef} src="..." /> It’s a bit contrived but it illustrates the API makes it possible to pass different callbacks every time. If we didn’t clean up the old ref, it would be a terrible footgun the user actually intends to pass a different ref because we wouldn't respect it. So they would keep the old reference but not set a new one. This would make the initial mount behave inconsistently from update, which is against how other React APIs (e.g. props) work. I hope this makes sense. |
<img ref={this.props.isAvatar ? this.handleAvatarRef : this.handleNormalRef} src="..." /> Maybe I'm thinking about this wrong, but I would still expect this to fire only once. So if the component mounts and
I thought about that, but I need the actual DOM node (so I can run componentDidMount() {
const node = this.refs.rootNode;
this.props.onMount({ rect: node.getBoundingClientRect() });
}
render() {
return <div ref="rootNode"> ... </div>
} To provide some context: I'm building a component that renders file icons and folders (each as a React component), and each |
That's not how React works generally. There is no other case where "initial props" are in any way special to "updated props". Usually React can update any prop but this would be a case where the first value "gets stuck".
Why use string refs there? componentDidMount() {
this.props.onMount({ rect: this.rootNode.getBoundingClientRect() });
}
render() {
return <div ref={node => this.rootNode = node}> ... </div>
} |
Oh. Ok, yeah, that makes a lot of sense. 😁 So Thanks for clarifying all this, Dan! |
Yes.
No, this is not necessary. |
Er, no - I know this. I know React! 😆 Sorry, I replied in haste. |
I am seeing something strange about this. This is what I am understanding: If a component updates it will simply call https://jsfiddle.net/dflores009/rwpfrge0/ Every time the Controlled |
Straight from the docs:
There are only two references to Either the documentation needs to be changed to clarify this is expected behavior or something needs to be fixed here. I hope we can all agree the latter is the case, because this would be very weird expected behavior! |
Or just <div ref={node => { this.node = node}}> to avoid add an unnecessary Also thanks for clearing that up, been looking in the documentation about that, also thought |
The docs were updated, just not published to website yet. See last section. |
Have you had a chance to read this thread? Yes, the docs were incomplete, but I believe it has been explained why this is happening. The explanation is both in #6249 (comment) and our last conversation with @ffxsam. I hope this helps. |
You can supply the "bound" callback method instead so that the javascript does not inefficiently recreate the callback method and call it on every render. |
It seems the
ref
callback isn't behaving as I expected it to, based on the docs.The docs say that
ref
is called when the component is mounted (and all the other times it's called, it's called withnull
as a parameter). However, when I put aconsole.log
incomponentDidMount
, and one in theref
callback, the one in theref
callback is called on everyrender()
, whereas thecomponentDidMount
is only called once. The component i'm reffing isn't even changing props.Am I misunderstanding something? I'm using React 0.14.
The text was updated successfully, but these errors were encountered: