Eslint-plugin-react: Rule request: require-default-props

[ljharb]

I'd like a rule that requires an explicit defaultProp to be present for every non-required prop (in propTypes or otherwise).

[lencioni]

It would be great if this rule could take into account references to objects in other modules. e.g.

// Component-shape.js
export default {
  foo: React.PropTypes.string,
  bar: React.PropTypes.string.isRequired,
}

// Component.js
import ComponentShape from './Component-shape.js';

const propTypes = ComponentShape;

export default function Component({ foo, bar }) {
  return <div>{foo}{bar}</div>;
}

Component.propTypes = propTypes;

should warn about foo but not bar, since bar is required.

[vitorbal]

I would like to work on this if no one is on it already!

[rik]

It would be great if that rule allowed an option for non-required booleans to be excluded. Because if they are not provided, it will be a falsy value.

[ljharb]

Falsy, but not false. Relying on falsiness can result in implicit coercions that can cause hard-to-find bugs. If an option for that were added, it'd need to be off by default, but I'm -1 on adding that option.

[rik]

I agree that it should be off by default but I still this as a nice option.

If it isn't provided, it's undefined. If it is provided, it has to be true or false or the propType will complain. So I think we're fine in terms of implicit coercions.

[ljharb]

The implicit coercion would be in the component code itself, where it expects a boolean but sometimes gets undefined.

[caesarsol]

@vitorbal are you actually working on this? would be a killer feature!

[vitorbal]

Sorry, was a bit bogged own with other tasks in my pipeline ⏳ I'm still interested in pursuing this though, will try to make some time for it these next couple of weeks.

[vitorbal]

Quick update: I've been slowly chipping away at this and covering all the different cases it would have to support. I will probably have a PR up as an RFC by next week.

[vitorbal]

Okay, I finally managed to wrap up most of this rule, I think. The only thing missing is support for Flowtype. @ljharb @yannickcr How important would you say is flowtype support? Could I tackle that as a separate PR, for example?

[ljharb]

I think it's critical that it not break on code that has flow syntax, at the very least, but I'm not sure how critical it is that the initial PR support flow. It'd probably be ideal to include it at the outset, though.

[vitorbal]

@ljharb fair enough, thanks for the feedback. In that case, I'll start working on adding Flow support as well.

[agpt]

@yannickcr How to configure this rule in eslintrc ? There is no rule options given in the documentations. Also how to use this rule with react/prop-types ? thanks

[vitorbal]

@agupta-q4 the rule does not have any extra configuration besides just being turned "on" or "off". Is there any specific configuration that you are looking for?

[agpt]

@vitorbal Thanks for the heads up ! :) yes you are right, I didn't understood it at first hand. actually whenever I try to add object type in propTypes definition, this rule says forbidden. so in react I added something like below to overcome this warning:

LoginForm.propTypes = {
  formState: PropTypes.shape({
    emailid: PropTypes.string.isRequired,
    password: PropTypes.string.isRequiredform
  }).isRequired
}

now it is working fine.

also do you have any idea what is rule options for following two newly added rules:

"react/require-default-props"
"react/no-array-index-key"

(I am newbie in react - redux environment)
Thanks.

[smokku]

This rule should accept a list of props that are excluded from the check.
i.e. it is perfectly fine to have children undefined while validating for node when defined. Writing defaultProps = { children: undefined } every time is redundant and annoying.

Similarly, we have a custom of merging given className using classnames every time: className={classNames(this.props.className, styles.self)} and having className undefined is just fine for classnames lib. Adding defaultProps = { className: undefined } to every single component is equally redundant and annoying.

[ljharb]

No, it's not perfectly fine - children needs a defaultProp too. It serves as documentation, and ensures that the author didn't make a mistake.

Separately, having a className prop is not a great idea at all anyways: https://brigade.engineering/don-t-pass-css-classes-between-components-e9f7ab192785 but if you have it, you need the defaultProp there too.

You're free not to use the rule, of course, if you don't like the rigor it provides.

[smokku]

I don't agree on misusing defaultProps as documentation. propTypes serves a better job at this, telling not only whether the property is being accepted, but what type(s) it should be and is it required. One could of course pull default value from defaultProps - falling back to undefined as the component does. And className argument is bogus - it assumes that you are dealing with people who do not know what they are doing.

Back on topic - if the rule accepted excludes, we would both be happy. I would exclude children, and you would be free to not use excludes and have all the rigor you want. _One size does not fit all._

[smokku]

Anyway, here's a fork with support for ignore option: https://github.com/smokku/eslint-plugin-react

"react/require-default-props": [
  "error",
  {
    "ignore": ["children"]
  }
]

If anyone is interested, just put "eslint-plugin-react": "git://github.com/smokku/eslint-plugin-react", to your package.json.

[ljharb]

If you'd like to open a separate issue requesting that feature, we can discuss it.

The benefit of having even an explicit undefined over an implicit one is the same as the universal "explicit > implicit" - developers working on that component can know, with certainty, what the value of the prop is.

(The className argument is not bogus; and in fact when developing you should assume that nobody knows what you're doing - always err on the side of safety)