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

defaultProps rule to be deprecated on function components #2396

Closed
amypellegrini opened this issue Aug 29, 2019 · 31 comments · Fixed by #3249
Closed

defaultProps rule to be deprecated on function components #2396

amypellegrini opened this issue Aug 29, 2019 · 31 comments · Fixed by #3249

Comments

@amypellegrini
Copy link

defaultProps on function components is getting deprecated by React (see: facebook/react#16210)

For this rule to follow suit, it should only be triggered when missing from class components.

The official recommendation in future versions of React is to use ES6 default values for function components.

@ljharb
Copy link
Member

ljharb commented Aug 30, 2019

That won't change by default; there can be an option to ignore function components, however.

@KevinGhadyani-minted
Copy link

KevinGhadyani-minted commented Oct 7, 2019

Is there a way to check the deconstructed values for default props instead?

@ljharb
Copy link
Member

ljharb commented Oct 7, 2019

@KevinGhadyani-minted I'm not sure what you're asking.

@KevinGhadyani-minted
Copy link

KevinGhadyani-minted commented Oct 7, 2019

The change isn't to remove defaultProps from function components, but to use the official ES6 standard for defaulting props using object deconstruction and default assignment.

Where we once had:

const TestComponent.defaultProps = {
  size: 'medium',
  type: 'primary',
}

We now have:

const TestComponent = ({
  size = 'medium',
  type = 'primary',
}) => (
  // ...
)

The goal isn't to remove defaultProps, but change how they're defined on function components.

I would expect the defaultProps rules to function the same regardless of if I use defaultProps or object deconstruction and default assignment.

@ljharb
Copy link
Member

ljharb commented Oct 7, 2019

That's something that should be configurable via an option, as I said above.

@KevinGhadyani-minted
Copy link

I think I'm confused then. In your comment, you said there can be an option to ignore it for function components.

To me, this means there's development that would occur to disable it for function components, but this development has not occurred. It doesn't say there'd be an alternative for getting it to identify ES2015 default assignment instead.

@ljharb
Copy link
Member

ljharb commented Oct 8, 2019

Yes, it means someone would have to do the development work to add an option.

It makes sense to me to modify require-default-props so that the default is "defaultProps" for everything, but that there's an option for function components to instead require "default arguments", or, to ignore function components.

@aaronmccall
Copy link

@ljharb, I'm considering working on the addition you suggested above, is there a good place to start beyond looking at the current rule implementation?

@ljharb
Copy link
Member

ljharb commented May 13, 2020

@aaronmccall That, and its tests, are pretty much it :-)

@nknaveen007
Copy link

if you are using a state ,then use the default values like below,
const BlogPostForm=({onSubmit,initialValues=()=>{initialValues.title='',initialValues.content=''}})=>{ //default values set in a function

const [title,setTitle]=useState(initialValues.title)
const [content,setContent]=useState(initialValues.content)

@chbdetta
Copy link

Can we also have a way to enforce that function components can't have defaultProps?

@ljharb
Copy link
Member

ljharb commented May 24, 2021

@chbdetta that's included in #2396 (comment)

@flying-sheep

This comment has been minimized.

@ljharb

This comment has been minimized.

@flying-sheep
Copy link

flying-sheep commented Nov 16, 2021

The second variant is no less correct, it just doesn’t give some of the types names.

Regarding our comments being hidden: Is this not the right place for this discussion? Whatever is the case, I very much think that my eslintrc.yaml might be helpful to people so in hopes this comment won’t be hidden without explanation I’ll reproduce it here:

rules:
  react/require-default-props:
  - error
  - forbidDefaultForRequired
overrides:
- files: ['*.ts', '*.tsx']
  rules:
    react/require-default-props: off # use default arguments in typescript

@ljharb
Copy link
Member

ljharb commented Nov 16, 2021

@flying-sheep discussion about just disabling the rule, even with overrides, won’t likely help anyone; people already know they can do that.

Either way, defaultProps should absolutely be used in TS still, and the type info does not contain the same information - because 100% of it is stripped out at runtime, which is when the defaultProps information can be needed.

@amypellegrini
Copy link
Author

@flying-sheep discussion about just disabling the rule, even with overrides, won’t likely help anyone; people already know they can do that.

Either way, defaultProps should absolutely be used in TS still, and the type info does not contain the same information - because 100% of it is stripped out at runtime, which is when the defaultProps information can be needed.

Would that very depending on the compilation target? I would expect default values assigned in ES6 to still work at runtime (be that built by TS or not), right?

@ljharb
Copy link
Member

ljharb commented Nov 16, 2021

@amypellegrini they work, but are not introspectable, so at runtime tooling can’t read information about what’s defaulted except with .defaultProps.

@flying-sheep
Copy link

Why treat components in a special way? If you use JS, your code base could benefit from runtime type checking for all kinds of functions, not just components.

Once you have fully migrated a code base to TypeScript, all types are proven correct at compile time and runtime checking isn’t necessary.

@ljharb
Copy link
Member

ljharb commented Nov 16, 2021

all types are proven correct at compile time and runtime checking isn’t necessary.

This is objectively false, since every system includes runtime user data (from the network, filesystem, user interaction, etc), as well as a majority of third-party code that was never typechecked, and this false belief is actually the worst damage caused by the use of a type system.

@ljharb
Copy link
Member

ljharb commented Nov 16, 2021

Even if TS was a sound type system - which it is not - it would NEVER be safe to omit runtime checks or to believe that types are "proven" correct. TS, for example, thinks that this JS snippet which has thrown an exception since 2009 - Object.create(null).toString() - is perfectly type-safe, so it's incredibly easy to disprove your claim.

@flying-sheep
Copy link

Well, seems like none of my systems ever put user data through edge cases like the one you pointed out, but point taken!

@ljharb
Copy link
Member

ljharb commented Feb 19, 2022

#2396 (comment) is the plan of record here.

@uloco
Copy link

uloco commented Dec 5, 2022

If this gets depreacted, is there still a way to set the defaultProps globally for example the react-native <Text> element?

@ljharb
Copy link
Member

ljharb commented Dec 5, 2022

@uloco im not familiar with what you’re describing, but either way, that’s a question to ask react native, and only if concrete plans to depreciate it ever materialize.

@avin-kavish
Copy link

Any way to disable the error that react throws in development about this?

@pavinduLakshan
Copy link

#2396 (comment) is the plan of record here.

Does anyone have an idea whether this improvement is available now?

Thanks in Advance!

@ljharb
Copy link
Member

ljharb commented Jun 4, 2024

@pavinduLakshan at the top, it says "fixed by #3249", and if you go there, you can see my comment #3249 (comment) which says that it's in v7.30.0.

@pavinduLakshan
Copy link

@pavinduLakshan at the top, it says "fixed by #3249", and if you go there, you can see my comment #3249 (comment) which says that it's in v7.30.0.

Thanks. missed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.