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

Add warning for ref to a stateless component #4943

Merged
merged 5 commits into from
Oct 6, 2015
Merged

Add warning for ref to a stateless component #4943

merged 5 commits into from
Oct 6, 2015

Conversation

bspaulding
Copy link
Contributor

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.

@@ -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();
Copy link
Contributor

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).

Copy link
Contributor Author

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.'
Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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)

@bspaulding
Copy link
Contributor Author

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.

@zpao
Copy link
Member

zpao commented Sep 25, 2015

Yea, don't worry about any unrelated lint issues.

ref,
componentName,
this.getName()
);
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@sebmarkbage
Copy link
Collaborator

We could possibly bring this in tomorrow for 0.14 if we feel like it.

@jimfb
Copy link
Contributor

jimfb commented Oct 6, 2015

👍

@sebmarkbage sebmarkbage modified the milestones: 0.15, 0.14 Oct 6, 2015
sebmarkbage added a commit that referenced this pull request Oct 6, 2015
Composite component throws on attaching ref to stateless component #4939
@sebmarkbage sebmarkbage merged commit 6d3a11e into facebook:master Oct 6, 2015
@bspaulding bspaulding deleted the throw-stateless-ref branch October 6, 2015 21:41
@gaearon
Copy link
Collaborator

gaearon commented Oct 8, 2015

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.
(The use case, I think, is calling imperative methods, or accessing underlying instance in tests.)
We don't use this ref ourselves—we only provide access to it because people asked for that access.

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.
Can we keep supporting this for stateful components, but avoid warnings on stateless ones?

@bspaulding
Copy link
Contributor Author

@sophiebits sophiebits changed the title Composite component throws on attaching ref to stateless component #4939 Add warning for ref to a stateless component Jan 21, 2016
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.

6 participants