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

Added more info about refs in the documentation #8707

Merged
merged 6 commits into from
Jan 10, 2017
Merged

Conversation

gyfis
Copy link
Contributor

@gyfis gyfis commented Jan 7, 2017

I want to propose some changes to the Refs and the DOM documentation page.

I've also planned on adding an example for passing the refs up the component chain based on something I've needed to solve myself (e.g. you want to connect two dynamic components by line in React, so you need to both use refs and propagate them up the chain), and while it would be great to read up on this in the docs, it may be too specific for this section; I'd be happy to hear any recommendations.

I want to propose some changes to the Refs and the DOM documentation page. 
- Make it clear that string refs are legacy. It seems that this information got lost during the transition to new docs and only some part stayed the same, which was confusing when first reading the docs.
- Clarify and explain that during render, if the ref callback is provided, it will get called twice, first with `null` and then with the rendered DOM element. Discussed in facebook#4533 and first proposed docs change in PR facebook#8333.

I've also planned on adding an example for passing the refs up the component chain based on something I've needed to solve myself (e.g. you want to connect two dynamic components by line in React, so you need to both use refs and propagate them up the chain), and while it would be great to read up on this in the docs, it may be too specific for this section; I'd be happy to hear any recommendations.
@Andarist
Copy link
Contributor

Andarist commented Jan 7, 2017

I think that this is not true. Each render WILL cause the described behaviour, but only if you use inline function as a callback ref.

That is because with each render it will create a new function (so a new reference) and pass it to React. React then will notify old instance (old reference from previous render) with null and the new instance with the actual element.

So your statement is true about this:
<div ref={ ref => this.node = ref }></div>

But it is not for this:

// class property
onRef = ref => this.node = ref
// render
<div ref={ this.onRef }></div>

@gyfis
Copy link
Contributor Author

gyfis commented Jan 8, 2017

Thanks for the comments @Andarist, you are completely right - I didn't realize that when you define the function outside of the render, it won't fall into the render lifecycle. That's good to know; I'll try to update the text to reflect that it is the scope of the function that causes the null/component double fire, and that defining the function outside of render can help.

Just to give some context on how I did get to refs in my project ...
This is what I originally tried with inline refs:

{this.props.components.map(component => (
  <Component ref={(ref) => liftRefs("component" + component.id, ref)} />
))}

which caused the nulls to propagate to the liftRefs prop, and that's because this callback function is created directly in render, just like you described.

So after reading your comment and realizing that different scope may help, I tried to define the function in the class, like this:

liftRefsWrapper = (componentId) => (ref) => this.props.liftRefs("component" + componentId, ref);

...

{this.props.components.map(component => (
  <Component ref={this.liftRefsWrapper(component.id)} />
))}

but again, the nulls were getting passed - I guess that even though the outer function is defined outside of render, the inner is getting created dynamically and therefore is different at each render.

My final test was to create all the lifting functions before running render:

liftRefsFunctions = this.props.components.map(component => (ref) => this.props.liftRefs("component" + component.id, ref));

...

{this.props.components.map((component, index) => (
  <Component ref={this.liftRefsFunctions[index]} />
))}

The last approach did not propagate the nulls, as expected, because the functions were created before running the render function. However, because the components props can change, the array of functions would need to be updated after each prop change, and it would take some care to keep the function instances correct - and with this much effort, it may just be easier to handle the nulls being passed to the function, specially when one knows about it.

@Andarist
Copy link
Contributor

Andarist commented Jan 8, 2017

you could also try with

{this.props.components.map(component => (
  <Component ref={ref => ref && liftRefs("component" + component.id, ref)} />
))}

@gyfis
Copy link
Contributor Author

gyfis commented Jan 8, 2017

That would indeed work as well, although you'd lose the null callback on the unmount - I guess that depends on what the final requirements are.

@@ -50,9 +50,11 @@ class CustomTextInput extends React.Component {
}
```

React will call the `ref` callback with the DOM element when the component mounts, and call it with `null` when it unmounts.
React will call the `ref` callback with the DOM element when the component mounts, and call it with `null` when it unmounts. Additionally, if the `ref` callback is defined as an inline function, it will get called twice during each render pass, first with `null` and then again with the DOM element. This is because internally, a new instance of the function is created with each render, so React needs to reset the old instance and set-up the new one. This can be avoided by defining the ref callback as a property on the class, but note that it shouldn't matter in the average `ref` use case.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too much (mostly irrelevant) information for somebody who is just learning about refs for the first time.

I would prefer we add a "Caveats" section below for advanced users that mentions this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it would be a good fit here because it's a place where the ref callback calls are mentioned, and also, for me, the first time I was reading up about refs was also the time I'd appreciate this kind of information. But you're right it's a better fit for the "Caveats" section. I'll update the text later today. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The important part is that while some readers will be new just to callback refs, for a growing number of readers over time this will be the first introduction to the concept of refs at all. And as such, throwing too many details on them will only confuse them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you mean, thanks! I've moved the text talking about the double ref fire into a "Caveats" section at the bottom of the doc as advised, so hopefully new readers of React won't have to care about these specific details unless/until they really need it.


### Caveats

If the `ref` callback is defined as an inline function, it will get called twice during each render pass, first with `null` and then again with the DOM element, additionally to the mount/unmount call. This is because a new instance of the function is created with each render, so React needs to reset the old instance and set-up the new one. This side-effect of inline callback function can be avoided by defining the ref callback as a property on the class, but note that it shouldn't matter in the average `ref` use case.
Copy link
Collaborator

@gaearon gaearon Jan 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nits:

during each render pass => during updates
, additionally to the mount/unmount call (it's unnecessary)
so React needs to reset the old instance and set-up the new one => so React needs to clear the old ref and set up the new one
This side-effect of inline callback function can be avoided => You can avoid this
as a property on the class => as a bound method on the class because class properties are a proposal and not universally known
in the average ref use case => in most cases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestions! I'm ok with all but replacing the mentioned render with updates, maybe I don't understand but when saying it will get called twice during updates, what updates? The DOM update? Props/State update? I think it may confuse readers; when we specify during render directly, I think it's more technical, but more clear too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I've read the whole paragraph again, and updates is ok - it is mentioned afterwards that it's the render that creates the function. Thanks and sorry for the confusion!

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix nits above

@gaearon
Copy link
Collaborator

gaearon commented Jan 10, 2017

Made a few small tweaks. Thank you so much!

@gaearon gaearon merged commit 4a7e06b into facebook:master Jan 10, 2017
@gyfis
Copy link
Contributor Author

gyfis commented Jan 11, 2017

Thanks a lot @gaearon !

@gyfis gyfis deleted the patch-2 branch January 11, 2017 08:48
gaearon pushed a commit that referenced this pull request Jan 12, 2017
* Update refs-and-the-dom.md

I want to propose some changes to the Refs and the DOM documentation page.
- Make it clear that string refs are legacy. It seems that this information got lost during the transition to new docs and only some part stayed the same, which was confusing when first reading the docs.
- Clarify and explain that during render, if the ref callback is provided, it will get called twice, first with `null` and then with the rendered DOM element. Discussed in #4533 and first proposed docs change in PR #8333.

I've also planned on adding an example for passing the refs up the component chain based on something I've needed to solve myself (e.g. you want to connect two dynamic components by line in React, so you need to both use refs and propagate them up the chain), and while it would be great to read up on this in the docs, it may be too specific for this section; I'd be happy to hear any recommendations.

* Adds more specific information about the callback

* Moved the ref callback description to the Caveats section

* Fixed suggested nits

* Replace 'each render pass' with 'updates'

* Tweak the wording

(cherry picked from commit 4a7e06b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants