facebook / prop-types

Runtime type checking for React props and similar objects
MIT License
4.48k stars 356 forks source link

Cannot represent spread ...props passed to a child #319

Closed jamesarosen closed 4 years ago

jamesarosen commented 4 years ago

Let's say there's a third-party library like this:

export default function Notice({ header, body, className }) {
  return (…)
}

Notice.defaultProps = {
  className: '',
}

Notice.propTypes = {
  header: PropTypes.string.isRequired,
  body: PropTypes.string.isRequired,
  className: PropTypes.string,
}

And I want to wrap it:

export default function MyNotice({ type, ...props }) {
  return <Notice {{...props, className: `my-notice--${type}`}} />
}

MyNotice.defaultProps = {
  type: 'info',
}

MyNotice.propTypes = {
  type: PropTypes.oneOf([ 'debug', 'info', 'warn', 'error' ]),
}

If I don't declare header, body, and className as props on MyNotice then I get warnings about unused props.

If I do declare them on MyNotice, then I get a warning from react/no-unknown-property. (I can disable that warning, but it seems like its existence suggests that the team thinks all props should be declared.)

I can try to merge in the child component's prop types:

MyNotice.propTypes = {
  ...Notice.propTypes,
  type: PropTypes.oneOf([ 'debug', 'info', 'warn', 'error' ]),
}

But that gets a warning from react/forbid-foreign-prop-types. And even if it didn't, many third-party libraries strip the prop-types from their exported builds.

I'd love to be able to do something like this:

MyNotice.propTypes = {
  type: PropTypes.oneOf([ 'debug', 'info', 'warn', 'error' ]),
  ...PropTypes.spread,
}

Related: #285

ljharb commented 4 years ago

forbid-foreign-prop-types ensures that you use an explicitly exported propTypes shape, and not the .propTypes property; any explicitly exported propTypes wouldn't be stripped by any build.

Since you're trying to deal with linter warnings, you should file this on eslint-plugin-react, not here - there's nothing this project can do or needs to do to address your question.

(However, this is also intentional; you should be either manually duplicating Notice's propTypes on MyNotice, or, explicitly exporting the propTypes object so you can spread that into MyNotice.propTypes and then disabling the relevant eslint warnings)

jamesarosen commented 4 years ago

you should be either manually duplicating Notice's propTypes on MyNotice

If I declare the props and pass them explicitly to the child then I won't get Notice's defaults.

If I declare the props but then pass them as ...props then I get a lint warning about unused props. I guess I can take that to the other project.

or, explicitly exporting the propTypes object so you can spread that into MyNotice.propTypes and then disabling the relevant eslint warnings

That assumes I have control over the third-party package. I don't think it's fair to assume that all React packages that use PropTypes export their types as standalone constants.

A completely non-scientific quick survey:

ljharb commented 4 years ago

I agree, but you could make a PR. Either way, these are linter warnings - if they don't make sense for an edge case, use an override comment and explain why you overrode it :-)