Advertise all underlying propTypes and typings in wrapper components

[levithomason]

See #1163 for the inspiration here.

Some components wrap others, like a Popup wrapping a Portal. However, we do not advertise the Portal props as propTypes nor typings of the Popup. This means there is no way for users to know what props are available.

Add propTypes

I propose we explicitly list all known props in propTypes and typings of the underlying component when a component wraps another component.

Add Examples

We should then also add doc site examples showing use of these props. Example, Popup mouse enter/leave delays are not shown but are available as it composes the Portal which has mouse enter/leave delays.

Task List

Here is a brain dump of wrapper components, there may be more. @jcarbo / @layershifter feel free to update this list at will:

• [ ] Popup -> Portal
• [ ] Modal -> Portal
• [ ] Confirm -> Modal
• [ ] Select -> Dropdown
• [ ] Radio -> Checkbox
• [ ] Table.Footer -> Table.Header
• [ ] Table.HeaderCell -> Table.Cell
• [ ] Form.Button -> FormField + Button
• [ ] Form.Checkbox -> FormField + Checkbox
• [ ] Form.Dropdown -> FormField + Dropdown
• [ ] Form.Input -> FormField + Input
• [ ] Form.Radio -> FormField + Radio
• [ ] Form.Select -> FormField + Select
• [ ] Form.TextArea -> FormField + TextArea

[layershifter]

LGTM, I'll will make this updates in PRs that affects typings.

[levithomason]

Great, thanks! I've left Select / Radio unfinished on the list as I don't see all the props of their underlying components listed in their propTypes. Example, Dropdown takes a search prop so the Select propTypes should also list search.

[layershifter]

@levithomason I need there some clarification. Most of these pairs has a correct inheritance in typings, however, I need to make an additional check.

I'm also support an idea with docs, but there is problem with propTypes. I see two ways to deal with them.

TableHeaderCell.propTypes = {
  ...TableCell.propTypes,
  as: customPropTypes.as,
  children: PropTypes.node,,
}

TableHeaderCell.propTypes = {
  as: customPropTypes.as,
  children: PropTypes.node,
  collapsing: PropTypes.bool,
  icon: customPropTypes.itemShorthand,
}

I'm not sure that docs generation will support the first way. While second way will produce a tons of unnecessary work with handling of props:

function TableHeaderCell (props) {
  const { children, collapsing, icon } = props
  // ...
  return <TableCell collapsing={collapsing} icon={icon}>{children}</TableCell>
}

[levithomason]

Let me check on this, I saw another project that extended react-docgen to allow this exact usage as well. Perhaps that has been integrated in to react-docgen core by now.

[levithomason]

Indeed, we get some support for this. Adding your above snippet to the header cell, I get this output in the docgenInfo.json:

  "src/collections/Table/TableHeaderCell.js": {
    "methods": [],
    "props": {
      // all the TableHeaderCell props
    },
    "composes": [
      "./TableCell"
    ],
    "docBlock": {
      "description": "A table can have a header cell.",
      "tags": []
    }
  },

We can use the composes key to include all the props of the composed component.

One issue is that we need to update babel-plugin-transform-react-handled-props as it doesn't know how to handle the spread operator there:

Message:
    ./src/collections/Table/TableHeaderCell.js

Module build failed: TypeError: ./src/collections/Table/TableHeaderCell.js: Cannot read property 'name' of undefined
    at ./node_modules/babel-plugin-transform-react-handled-props/lib/visitors/propVisitor.js:26:25
    at arrayMap (./node_modules/lodash/lodash.js:660:23)
    at Function.map (./node_modules/lodash/lodash.js:9571:14)
    at getObjectKeys (./node_modules/babel-plugin-transform-react-handled-props/lib/visitors/propVisitor.js:25:27)
    at Store.AssignmentExpression (./node_modules/babel-plugin-transform-react-handled-props/lib/visitors/propVisitor.js:47:34)
    at NodePath._call (./node_modules/babel-core/node_modules/babel-traverse/lib/path/context.js:76:18)
    at NodePath.call (./node_modules/babel-core/node_modules/babel-traverse/lib/path/context.js:48:17)
    at NodePath.visit (./node_modules/babel-core/node_modules/babel-traverse/lib/path/context.js:105:12)
    at TraversalContext.visitQueue (./node_modules/babel-core/node_modules/babel-traverse/lib/context.js:150:16)
    at TraversalContext.visitSingle (./node_modules/babel-core/node_modules/babel-traverse/lib/context.js:108:19)
    at TraversalContext.visit (./node_modules/babel-core/node_modules/babel-traverse/lib/context.js:192:19)
    at Function.traverse.node (./node_modules/babel-core/node_modules/babel-traverse/lib/index.js:114:17)
    at NodePath.visit (./node_modules/babel-core/node_modules/babel-traverse/lib/path/context.js:115:19)
    at TraversalContext.visitQueue (./node_modules/babel-core/node_modules/babel-traverse/lib/context.js:150:16)
    at TraversalContext.visitMultiple (./node_modules/babel-core/node_modules/babel-traverse/lib/context.js:103:17)
    at TraversalContext.visit (./node_modules/babel-core/node_modules/babel-traverse/lib/context.js:190:19)

This sounds like a slippery slope as well. It may be pretty tough to properly traverse the propTypes of objects imported from elsewhere. My hunch is that if it were easy, react-docgen would have included that feature rather than listing composes instead.

Another (much smaller) issue is that the resolve is also a little too tricky for my IDE and I'm guessing others' tools as well. This means unless you're using TS, your tool probably won't benefit much from this kind of composition. This should be taken lightly as tools will catch up someday, but it is worth considering.

This is a lot to manage by hand and also creates nasty low visibility dependencies that we're going to break often. At some point, updating a Portal prop is bound to cause breakages in all the other propTypes that use it.

Let's think on this a bit more. We want to reduce maintenance overhead, reduce dependencies, and increase doc visibility.

[layershifter]

One issue is that we need to update babel-plugin-transform-react-handled-props as it doesn't know how to handle the spread operator there:

I'll make an update to plugin on this week, it will ignore spreads.

[levithomason]

Won't this break the handledProps array? It needs to parse them to know which are handled and which are ...rest, correct?

[layershifter]

Nope.

It needs to parse

It's a kind of black magic if we speak about external imports like in my snippet. So, it will be easier to ignore spreads at now at all. Especially, in our case, when this behavior is expected.

TableHeaderCell.propTypes = {
  ...TableCell.propTypes,
  as: customPropTypes.as,
  children: PropTypes.node,
}
// will produce
TableHeaderCell.handledProps = ['as', 'children']

[layershifter]

I've checked typings of all components in list. All of them now correctly extends parents, except Confirm (#1425). babel-plugin-transform-react-handled-props was also updated 😄

[levithomason]

Great, so we should be able to spread the inherited propTypes as well then. Once that is done, I can update ComponentProps to parse and display the composes props value as well.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 30 days if no further activity occurs. Thank you for your contributions.

[levithomason]

This really needs to be done. It bites again in #2579.

[stale[bot]]

There has been no activity in this thread for 90 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 90 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

[martindale]

This issue should remain open (and closing issues marked as stale seems anti-productive...)

[stale[bot]]

There has been no activity in this thread for 180 days. While we care about every issue and we’d love to see this fixed, the core team’s time is limited so we have to focus our attention on the issues that are most pressing. Therefore, we will likely not be able to get to this one.

However, PRs for this issue will of course be accepted and welcome!

If there is no more activity in the next 180 days, this issue will be closed automatically for housekeeping. To prevent this, simply leave a reply here. Thanks!

[stale[bot]]

This issue will be closed due to lack of activity for 12 months. If you’d like this to be reopened, just leave a comment; we do monitor them!

[Anuragtech02]

Hey @layershifter ! Is this issue still open? I'd love to work on this.

[maxwell-tchiabe]

Hey @layershifter ! Is this issue still open? I'd love to work on this.

[Ranzeb]

I am new to open-source contributions and I want to contribute. Can I handle this issue?

[martindale]

@Anuragtech02, @Ranzeb — don't ask, just do! It's the open source way.