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

connect doesn't apply ref to stateless component #143

Closed

Conversation

bspaulding
Copy link

Took a stab at #141

Probably would be better to create the render function beforehand, instead of hitting that conditional every render. Just getting this out there for discussion.

Worth noting that the detection isn't perfect, but solves the case mentioned here, and keeps the current test for wrappedInstance ref passing. We'd have to instantiate the component to know for sure, but this might be good enough.

Pretty much stole the check from ReactCompositeComponent

if (isStateless) {
return <WrappedComponent {...this.nextState} />;
}

Copy link
Author

Choose a reason for hiding this comment

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

This should probably not be here. If we decide to go ahead with this, we should probably create render above and apply it to this class.

@bspaulding bspaulding force-pushed the stateless-wrapped-component-ref branch from 3c70fdf to f4f2d1f Compare October 8, 2015 18:57
@gaearon
Copy link
Contributor

gaearon commented Oct 8, 2015

Wouldn't this give false positives for module pattern of non-Component-inheriting classes which are still valid in 0.14?

@bspaulding
Copy link
Author

Yes, indeed. I'm not sure at this point how you'd figure that out before mounting the component. I'll keep hacking on it.

return <span className={this.state.value} />;
}
};
}
Copy link
Author

Choose a reason for hiding this comment

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

I assume this is the case you're talking about?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, interestingly, this turns out to not be a false positive. At least, React thinks this is stateless, and shows the warning.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I got confused. This spec is bad, wrapping it in a function below.

@gaearon
Copy link
Contributor

gaearon commented Oct 15, 2015

Thanks for the PR.
In the end I decided on a simpler approach: refs are now opt-in at the connect() call site.

Closed via 2d3d0be.

@gaearon gaearon closed this Oct 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants