-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Add warning for ref to a stateless component #4943
Conversation
@@ -808,8 +808,10 @@ var ReactCompositeComponentMixin = { | |||
attachRef: function(ref, component) { | |||
var inst = this.getPublicInstance(); | |||
invariant(inst != null, 'Stateless function components cannot have refs.'); | |||
var componentInstance = component.getPublicInstance(); |
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.
semantics, but let's call this publicInstance
or publicComponentInstance
instead of componentInstance
. The problem is that the component
variable passed in is technically also a componentInstance (just, it's the internal one).
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.
Dig. I definitely punted on what to name that. Will go with publicComponentInstance
.
|
||
expect(console.error.argsForCall.length).toBe(1); | ||
expect(console.error.argsForCall[0][0]).toContain( | ||
'Stateless function components cannot be given refs.' |
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.
Now that this is a warning, I wonder if a more helpful message would be nice. Even if it was just Stateless function components cannot be given refs. Attempts to access this ref will fail.
Thoughts?
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.
Yeah, would be good to give as much context as possible also.
Stateless function components cannot be given refs (see ChildComponentName created by OwnerComponentName). Attempts to access this ref will fail.
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.
Donezo. Also added the ref name into the error: (See ref "stateless" in StatelessComponent created by Parent)
A little confused about the lint failures. I see the same thing in master. I assume I should not go ahead and fix those? Though I'd be happy to. |
Yea, don't worry about any unrelated lint issues. |
ref, | ||
componentName, | ||
this.getName() | ||
); |
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 whole warning should be within a __DEV__
block so that it doesn't need to be evaluated in the production builds.
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.
Done.
We could possibly bring this in tomorrow for 0.14 if we feel like it. |
👍 |
Composite component throws on attaching ref to stateless component #4939
I have a question: it came up in reduxjs/react-redux#141. I have a HOC that creates a ref in case the user might want to use it. What should we do in HOC? Currently we do this: function connect(Stuff) {
return class ConnectedStuff extends Component {
getWrappedInstance() {
return this.refs.wrappedInstance;
}
render() {
return (
<Stuff ref='wrappedInstance' />
);
}
}
} This logs a warning with stateless components. |
Fixes #4939. I know the issue was specifically to add a warning, but I thought it would be more consistent to throw.
I'm happy to back it down to a warning if that is preferable.