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

Make the GlobalStyleComponent call the base constructor with props. #2321

Conversation

Projects
None yet
2 participants
@jaydenseric
Copy link
Contributor

jaydenseric commented Jan 14, 2019

This PR ensures the GlobalStyleComponent created by createGlobalStyle calls the base constructor with props.

This is a React best practice for class components:

Class components should always call the base constructor with props.
https://reactjs.org/docs/state-and-lifecycle.html#adding-local-state-to-a-class

Not doing so can cause issues with this.props being undefined when rendered by libraries that are not very fault tolerant.

React's renderToString accounts for it here: https://github.com/facebook/react/blob/v16.7.0/packages/react-dom/src/server/ReactPartialRenderer.js#L532

Apollo's getDataFromTree used to account for it here: https://github.com/apollographql/react-apollo/blob/v2.2.4/src/getDataFromTree.ts#L80

Fixes jaydenseric/graphql-react#17 .

@jaydenseric

This comment has been minimized.

Copy link
Contributor Author

jaydenseric commented Jan 14, 2019

Flow types gave me hell, both in setting up VSCode to make this PR, and in trying to learn how to add types for props. Will the way I have done it allow people to use the component with no props at all?

@probablyup

This comment has been minimized.

Copy link
Contributor

probablyup commented Jan 15, 2019

Flow types gave me hell, both in setting up VSCode to make this PR, and in trying to learn how to add types for props. Will the way I have done it allow people to use the component with no props at all?

We don't export our flow types, so don't worry about that. They'll actually be removed from the lib in general probably for v5.

@probablyup
Copy link
Contributor

probablyup left a comment

LGTM, thanks!

@probablyup probablyup merged commit cd134dc into styled-components:master Jan 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jaydenseric jaydenseric deleted the jaydenseric:globalstylecomponent-super-props branch Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.