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

this.props.children and `react/prop-types` rule #7

Closed
remitbri opened this issue Mar 2, 2015 · 21 comments
Assignees
Labels
bug

Comments

@remitbri
Copy link

@remitbri remitbri commented Mar 2, 2015

Using this.props.children triggers the rule ("'children' is missing in props validation").

Shouldn't it be an exception?

@yannickcr

This comment has been minimized.

Copy link
Owner

@yannickcr yannickcr commented Mar 2, 2015

Right, we should add an exception for this.props.children

@yannickcr yannickcr added the bug label Mar 2, 2015
@yannickcr yannickcr self-assigned this Mar 2, 2015
@yannickcr yannickcr closed this in 5df82bd Mar 2, 2015
@AsaAyers

This comment has been minimized.

Copy link

@AsaAyers AsaAyers commented Mar 24, 2015

Why should children be an exception? I think you should document when a component does accept or require children.

@yannickcr

This comment has been minimized.

Copy link
Owner

@yannickcr yannickcr commented Mar 24, 2015

You're right, but I rarely seen some propTypes for this.props.children so it dot not seems to be a common practice to me.

Maybe we can introduce an option to prop-types to re-include children in the props list to check?

@AsaAyers

This comment has been minimized.

Copy link

@AsaAyers AsaAyers commented Mar 24, 2015

It seems to me like most components don't need this.props.children, so to me that makes it extra important to document children in the propTypes.

I think the option idea is fine. I'm interested in hearing why anyone would want to run this against all other prop types, but never against children. It seems not well thought out to me, but I kind of hope I'm just missing something.

@sebastienbarre

This comment has been minimized.

Copy link

@sebastienbarre sebastienbarre commented Apr 7, 2015

All good points. [UPDATED]
I think @AsaAyers is onto something, considering the Single Child section in the React doc:

With React.PropTypes.element you can specify that only a single child can be passed to a component as children.

  propTypes: {
    children: React.PropTypes.element.isRequired
  },

In other cases when this.props.children might be used (or not), this below could probably do the trick:

  propTypes: {
    children: React.PropTypes.oneOfType([
      React.PropTypes.arrayOf(React.PropTypes.node),
      React.PropTypes.node
    ])
  },
  getDefaultProps: function() {
    return {
      children: null // or [] I guess
    };
  },

@remitbri

This comment has been minimized.

Copy link
Author

@remitbri remitbri commented Apr 7, 2015

I ended up with React.PropTypes.node to solve my issues…

@AsaAyers

This comment has been minimized.

Copy link

@AsaAyers AsaAyers commented Apr 7, 2015

It looks like I have a combination of element, node, a few strings, and an arrayOf(...shape(...)).

@sebastienbarre

This comment has been minimized.

Copy link

@sebastienbarre sebastienbarre commented Apr 7, 2015

@remitbri thanks, I updated my comment above accordingly. What's your default prop value? null?

@AsaAyers

This comment has been minimized.

Copy link

@AsaAyers AsaAyers commented Apr 7, 2015

React.PropTypes.any is also an option. If I really had a component that could support 0, 1, or more children I'd probably just stick with .any to document that children is used.

@remitbri

This comment has been minimized.

Copy link
Author

@remitbri remitbri commented Apr 7, 2015

Hmmm, my component is a wrapper, which could stand on its own, so I didn't set a default prop value. Bad practice? I guess if I had to set one then it should be null

@yannickcr

This comment has been minimized.

Copy link
Owner

@yannickcr yannickcr commented Apr 7, 2015

Since 2.0.0 children is no longer ignored for props validation.

If you still want to ignore it you can use the ignore option.

@matthewwithanm

This comment has been minimized.

Copy link
Contributor

@matthewwithanm matthewwithanm commented May 29, 2015

@AsaAyers Won't node handle those cases too?

@AsaAyers

This comment has been minimized.

Copy link

@AsaAyers AsaAyers commented May 29, 2015

yes, I think node is a better option than any, I just hadn't used it much at the time I commented.

@matthewwithanm

This comment has been minimized.

Copy link
Contributor

@matthewwithanm matthewwithanm commented May 29, 2015

👍 Thanks, just making sure.

@reggi

This comment has been minimized.

Copy link

@reggi reggi commented Sep 8, 2015

Just to clarify, children should be defined as type node, like this:

propTypes: {
  children: React.PropTypes.node
}
shorttompkins added a commit to shorttompkins/react_sandbox that referenced this issue Sep 23, 2015
The this.props.children warning details can be found here:
yannickcr/eslint-plugin-react#7 (comment)
@resistdesign

This comment has been minimized.

Copy link

@resistdesign resistdesign commented Nov 29, 2015

I know this is closed, but wouldn't children be inherited from Component? Or is OOP not a thing anymore?

@AsaAyers

This comment has been minimized.

Copy link

@AsaAyers AsaAyers commented Nov 29, 2015

Not all Components have children. In a couple of my projects I have an <Avatar user={userObject} />

@joaomilho

This comment has been minimized.

Copy link

@joaomilho joaomilho commented Jan 25, 2016

@resistdesign actually yeah, OOP is not the main thing anymore, at least in React's world.

@lencioni

This comment has been minimized.

Copy link
Collaborator

@lencioni lencioni commented Sep 16, 2016

Why do you need oneOfType here? The node propType is defined as "Anything that can be rendered: numbers, strings, elements or an array (or fragment) containing these types." https://facebook.github.io/react/docs/reusable-components.html

You should use children: PropTypes.node.

@jdanilov

This comment has been minimized.

Copy link

@jdanilov jdanilov commented Feb 18, 2018

Justin case you're wondering how to do that in Flow, use React.Node as described here:
https://flow.org/en/docs/react/types/#toc-react-node

type Props = {
  children: React.Node,
};
@Denly

This comment has been minimized.

Copy link

@Denly Denly commented Mar 15, 2018

PropTypes.node can be an array, so no need the React.PropTypes.arrayOf(React.PropTypes.node) @sebastienbarre

// Anything that can be rendered: numbers, strings, elements or an array
  // (or fragment) containing these types.
  optionalNode: PropTypes.node,
 
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.