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

require-default-props natively #1184

Closed
NullDivision opened this issue May 10, 2017 · 9 comments
Closed

require-default-props natively #1184

NullDivision opened this issue May 10, 2017 · 9 comments

Comments

@NullDivision
Copy link

Given the following example:

const Container = ({ name = 'Lucy' }: { name?: any }) => <div><span>{name}</span></div>;

Understandably the above code does not use the defaultProps assignment but I'm wondering if this should be added as an acceptable case considering that under ES6 destructuring rules this should be a perfectly valid case of property assignment.

@ljharb
Copy link
Member

ljharb commented May 10, 2017

It should not; defaultProps are not the same as ES6 destructuring (although both only apply to undefined). In addition, defaultProps provides information to React, which handles the defaulting; by using ES6 defaulting, you're depriving React of the ability to optimize based on it.

@NullDivision
Copy link
Author

NullDivision commented May 10, 2017

I understand what you're saying but from the React source I remember only a couple of things happening to defaultProps:

  • check for getDefaultProps (not the case for arrow function and I doubt anyone would have a reason to coerce it)
  • check for defaultProps property assigned to constructor
  • check for any illegal/reserved properties (eg. key) in the defaultProps object

Advantages of destructuring:

  • no reason for coercion
  • doesn't require React to loop through defaultProps on every update
  • no side-effects
  • respects JS/ES6 standards implementation

Maybe I'm missing what you mean by optimize. AFAIK React doesn't do component optimization on pure functions.

It could be an option for the rule like:

"react/require-default-props": [2, { allowImplicit: true }]

@ljharb
Copy link
Member

ljharb commented May 10, 2017

#666 may be relevant here.

@NullDivision
Copy link
Author

The issue is indeed related in a manner as it is the exact opposite of what I want. Also the rule proposal is aptly named prefer-default-props, not require. The scope of require-default-props should not be to enforce the use of the defaultProps property (note: in its current state the rule clashes with several other rule libraries as is my case with immutable/no-mutation)

The scope of the rule should be to assign a default value in order to prevent undefined property values. Am I correct in this assumption?

@ljharb
Copy link
Member

ljharb commented May 10, 2017

My interpretation of the original intent of the rule is to require a defaultProps entry for each non-required propType, not to prevent undefined property values (especially since undefined is a valid defaultProps value).

@NullDivision
Copy link
Author

I see your point. Still wondering if there's a way to circumvent the use of default props, especially if developers use Flow instead of propTypes.

@ljharb
Copy link
Member

ljharb commented May 10, 2017

Flow is not a complete replacement for PropTypes, as there's many things custom propType validators can do that Flow can't. I don't see the issue in using both Flow and defaultProps.

@mrchief
Copy link

mrchief commented Oct 15, 2019

@ljharb for functional components, seems like the preferred way is to use ES6 defaults. See this facebook/flow#7467

@mrchief
Copy link

mrchief commented Oct 15, 2019

@ljharb disregard my earlier comment. I'm just catching up on this #1009. That thread seems painful to repeat here. :)

@ljharb ljharb closed this as completed Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants