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

[docs] Add note about refs on stateless components #8916

Merged
merged 7 commits into from
Feb 6, 2017

Conversation

krinoid
Copy link

@krinoid krinoid commented Feb 2, 2017

Today at work we were trying to debug some code that used stateless components. We found out, that it's not possible to use refs on the stateless components as described in the docs. We founded this issue: #4936
and given that it wasn't obvious behaviour, we decided to add this info to the Caveats section in the doc page about ref.

Please let me know what you think about this. Thanks!

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@krinoid krinoid changed the title Add note about refs on stateless components [docs] Add note about refs on stateless components Feb 2, 2017
@@ -106,3 +106,23 @@ Your first inclination may be to use refs to "make things happen" in your app. I
### Caveats

If the `ref` callback is defined as an inline function, it will get called twice during updates, first with `null` and then again with the DOM element. This is because a new instance of the function is created with each render, so React needs to clear the old ref and set up the new one. You can avoid this by defining the `ref` callback as a bound method on the class, but note that it shouldn't matter in most cases.

Also, setting `ref` attribute on a custom component which is implemented as a stateless function won't work as intened. For example, given this code:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe shorten to just say You can set a ref to a functional component:

}
```

`this.helloNode` will be set to `null`. This is a deliberate decision, detailed discussion can be found in [this issue on GitHub](https://github.com/facebook/react/issues/4936). While it would be possible (currently) to make `ref` on stateless components work the same as with component classes, in the future versions of React it would be hard or even impossible to implement. Citing Sebastian Markbåge: "it is easier to go from restrictive -> loose than the other way around".
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is too much detail. We can just say "The ref will always be called with null. If you need to a ref to a component, it must be a class."

@gaearon
Copy link
Collaborator

gaearon commented Feb 2, 2017

FWIW, we have a new warning for this but it hasn't been released yet.

@krinoid
Copy link
Author

krinoid commented Feb 2, 2017

@gaearon So, given the planned warning should I make suggested changes or just close this PR? (thanks for the feedback, btw)

@gaearon
Copy link
Collaborator

gaearon commented Feb 2, 2017

I'm not sure when we'll get to cutting another release so I'm fine with adding this to docs for now.

@gaearon
Copy link
Collaborator

gaearon commented Feb 2, 2017

Also, to be fair, there is already

You may not use the ref attribute on functional components because they don't have instances.

in the docs. Maybe we can just expand this section?

@krinoid
Copy link
Author

krinoid commented Feb 2, 2017

Right, missed that line! What about that: I'll simplify the caveats section and add note to the

You may not use the ref attribute on functional components because they don't have instances.

line pointing to the caveat section?

@krinoid
Copy link
Author

krinoid commented Feb 2, 2017

Or I can just ditch this PR if you prefer, no hard feelings :D

@gaearon
Copy link
Collaborator

gaearon commented Feb 2, 2017

Maybe you could simplify the caveats section but make the warning in the main section more prominent (e.g. separate paragraph and a tiny example).

@krinoid
Copy link
Author

krinoid commented Feb 2, 2017

I've expanded the line in the doc you referenced and moved the example there, caveats section is left as it was before. Ok or not?

**You may not use the `ref` attribute on functional components** because they don't have instances. In the below code the ref will always be called with `null`:

```
function StatelessComponent() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's call it FunctionalComponent. That's the important part, not whether it uses state or not.
Maybe even MyFunctionalComponent to make it clear FunctionalComponent is not some special name.

@@ -73,7 +73,23 @@ class AutoFocusTextInput extends React.Component {
}
```

You may not use the `ref` attribute on functional components because they don't have instances. You can, however, use the `ref` attribute inside the functional component:
**You may not use the `ref` attribute on functional components** because they don't have instances. In the below code the ref will always be called with `null`:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just say "This won't work:". The fact that it's called with null is implementation detail, we might not call it at all in the future, and we'll definitely start warning about this. So we don't people to rely on "calling null" as the expected behavior.


class ParentComponent extends React.Component {
render() {
return <StatelessComponent ref={node => this.helloNode = node} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good to add an inline comment here:

// This will not work!

Otherwise people will just read over this and remember this example.

}
```

If you need to a ref to a component, it must be a class (additional discussion can be found in [this issue on GitHub](https://github.com/facebook/react/issues/4936)).
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's necessary to link to GH here as if it was a bug. People who are interested will find it anyway (you did).

@krinoid
Copy link
Author

krinoid commented Feb 2, 2017

Simplified the section. Good or not?

@krinoid
Copy link
Author

krinoid commented Feb 3, 2017

I'm not sure why GitHub shows that changes are requested after I've implemented them. If somebody can review the current version and let me know whether it's ok, it would be great. Thanks!

@krinoid
Copy link
Author

krinoid commented Feb 6, 2017

@gaearon: when you have time, please let me know whether current version is ok. Thank you!

@gaearon
Copy link
Collaborator

gaearon commented Feb 6, 2017

Thanks! I took the liberty of slightly editing the whole page to better fit this into the flow.

gaearon pushed a commit that referenced this pull request Feb 6, 2017
* Add note about refs on stateless components

* Move info about refs in the stateless components to the main section

* Simplification of the part about refs and functional components

* Tweaks

* Move sections around

* Oops

* Rearrange more sections

(cherry picked from commit d42c285)
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.

3 participants