Eslint-plugin-react: Rule `require-default-props` does not support default parameters in stateless function

[preco21]

Hello, I just got some error from require-default-props rule:

`react/require-default-props`:  propType "foo" is not required, but has no corresponding defaultProp declaration.
`react/require-default-props`:  propType "baz" is not required, but has no corresponding defaultProp declaration.

Here is the code:

function Header({
  foo = true,
  bar,
  baz = () => { /* noop */ },
}) {
  return ...;
}

Header.propTypes = {
  foo: PropTypes.bool,
  bar: PropTypes.bool.isRequired,
  baz: PropTypes.func,
};

I expected there is no error, but seems it causes the problem.

Any ideas?

[ljharb]

Default values are not the same as defaultProps. The rule correctly reports this as an error.

[preco21]

If so..

But I guess this kind of pattern is commonly used on stateless functions. So, this is reasonable to me.

In order to realize this pattern works, may I request another rule or option?

[ljharb]

That's why it's an important rule - it's bad to use default values instead of defaultProps.

I suppose we could make an option that allows default values but I think the existence of the option would be harmful.

[steida]

@ljharb Correct me if I am wrong, but this rule is unnecessary when we use flowtype.

[ljharb]

@steida i don't understand why it would be - defaultProps provide important information to the React runtime that default values can't possibly provide. Flowtype replaces propTypes - information that React doesn't actually use at runtime in production.

[steida]

Yeah, but if you have flowtyped everything, you need runtime checking only at application boundaries.

[ljharb]

@steida again, you're talking about propTypes. defaultProps are different, and control runtime behavior - not type-checking. Using default values prevents React from being able to make some kinds of optimizations.

[steida]

I don't want to use defaulProps https://twitter.com/estejs/status/812491172438560769

[steida]

As for optimizations, link pls?

[ljharb]

@steida There's no need for a link - by spec, default values are re-evaluated on every invocation of the function. defaultProps, on the other hand, are evaluated once and cached at the module level - across every invocation of the function. Even if React does nothing with regard to them (which I doubt, but haven't bothered to dig up evidence of), defaultProps are more performant than default values.

If you don't want to use defaultProps, disable this rule. If you want flow annotations to be equivalent, make sure that your Flow annotations transpile down to defaultProps, and then this rule can and should be adapted to support them - until that time, they're not the same, and defaultProps is staunchly recommended.

[bradennapier]

Hmm - yet the documentation for Flow / React specifically tell you to use default values this way and actually would not support propTypes.

// @ https://flow.org/en/docs/frameworks/react/
let MyComponent = ({ foo, bar = 2 }: { foo: number, bar?: number }) => {
  return <div>{foo + bar}</div>;
};

[ljharb]

The documentation for React suggests a number of bad practices amidst all the good ones - just because they make the framework doesn't mean they're always right :-)

[bradennapier]

truth -- I switched to using defaultProps already and working well. Would be cool to have a plugin to automatically convert to defaultProps in this situations -- although I know there could be times default parameters are the desired effect (when it executes a function, etc).

[AlexanderTserkovniy]

Hi guys,

In my opinion there should be at least an option to tell that require-default-props should pass if there is default parameter is set.

const Button = ({
  children = 'Cancel', intent = 'primary', className, size, loading = false, ...rest
}) => (
  <button
    {...rest}
    className={classnames(styles[intent], className, {
      [styles[`size-${size}`]]: size,
      [styles['loading-disabled']]: rest.disabled && loading,
    })}
  >
    {loading ? <Spinner height="100%" /> : children}
  </button>
);

intent in this case has default parameter (defaultProp). I do not want to repeat myself, if I write default parameter for some property in function arguments that should be considered as defaultProp.

Would you be so kind to add this option?

[ljharb]

That’s not included intentionally; react components (SFCs included) should simply not use default arguments at all.

[abalbanyan]

https://github.com/reactjs/rfcs/pull/107
defaultProps will eventually be deprecated on function components. Knowing this, it would be nice to have support for enforcing default values for optional props.

[ljharb]

Not "will" - that's not merged yet.

[nathggns]

@ljharb can you please expand on your opposition to default arguments. Considering the React team seems pretty set on deprecating them for function components, and for good reason, what is your opposition to them? Or at the very least, what is your opposition to supporting it as an option? It seems you're out of step with where the React team is going.

It seems to be that defaultProps are at the very least less performant, as they are resolved on every call to React.createElement?

Our team is moving to use default arguments instead of defaultProps for function components, but we use this rule. It would be good for it to support default arguments too.

[bvaughn]

defaultProps provide important information to the React runtime that default values can't possibly provide

Using default values prevents React from being able to make some kinds of optimizations.

I know this is an older thread, but I noticed it because it was referenced by a new RFC. Can you clarify what info and optimizations these comments were referring to?

To my knowledge, React does not use defaultProps for anything other than filing in default values for undefined props. May also be worth pointing out that defaultProps are not even supported for some newer types, e.g. forwardRef.

[ljharb]

@bvaughn at the time i made that comment, my understanding was that this was planned, but never materialized.

[ljharb]

@nathggns what is planned to be deprecated isn’t relevant until it actually is deprecated.

However, in particular, defaultProps can be read and reflected on by HOCs and runtime tools attempting to inspect components, where default arguments can not - so i hope the RFC doesn’t land (even if it’s very likely to do so).

[nathggns]

@nathggns what is planned to be deprecated isn’t relevant until it actually is deprecated.

I genuinely don't understand how you can believe this to be true. People are writing code now, and people can see what the React team is planning, and its pretty clear that defaultProps for function components is going away. Therefore people will want to write code now that doesn't use defaultProps. But they very quickly have to choose between changing the tooling, disabling it, or writing code they know is going to be deprecated in the near future. That's not good situation to be in.

It is also the case that if you know a feature is going to be deprecated in the future, that people making tooling that people depend on should be prepared for it. It makes no sense at all to wait literally until the release deprecating the feature before beginning to prepare options for avoiding the use of that feature.

TL;DR: people know that defaultProps is going to be deprecated. That genie is out of the bottle. The question now is whether people making tooling are going to prepare for that now or later, and I don't understand why you would pick later.

[ljharb]

I assume React, like it usually does, will do the responsible thing and release a codemod to convert defaultProps to default arguments (assuming this change happens) - and at the same time, this plugin will add an autofixable rule to enforce defaultProps placement on function components.

I'd pick later because nothing is certain until it's released - parts of hooks, for example, changed prior to release in ways that would have been complicated to transition alpha support to.

[nathggns]

I agree that nothing is certain until its released, but even if React decides not to deprecated defaultProps (which seems quite unlikely at this point), people are using default arguments now. People believe it is going to be deprecated and are therefore avoiding it. Is it your opinion that that is a mistake?

[FredyC]

@nathggns Sorry, but why do you see it as such a problem to simply disable this rule and do the default arguments right away? It's not mandatory to use rules that are most likely obsolete.

Edit: Sure, you don't have the rule to help you writing default arguments, but is it such a pain? Personally, I rather rely on TypeScript than some linter telling me what to do.

[nathggns]

@nathggns Sorry, but why do you see it as such a problem to simply disable this rule and do the default arguments right away? It's not mandatory to use rules that are most likely obsolete.

We are disabling the rule currently. We'd prefer for the rule to be able to check that non-required props have default arguments instead.

Not everyone uses or can use TypeScript. (Also TypeScript arguably is a super advanced linter anyway).

[ljharb]

Yes, I think a) deprecating defaultProps, and b) changing how you code based on unreleased "plans" are both mistakes.

[FredyC]

(Also TypeScript arguably is a super advanced linter anyway).

Sure, that's a different way to look at it, why not, many ways toward the goal :)

You might want to have a look at this which helps at least partially to cover default props in function components https://github.com/cvazac/eslint-plugin-react-perf

changing how you code based on unreleased "plans" are both mistakes.

This is where I don't agree. Or I mean you are generally right but in this case, it's actually a natural part of a language to have default values for props in arguments. The defaultProps is just React construct that was made long before we had this new luxury.

I've personally never liked defaultProps as they are too detached from the component and I was always been using default values in function components. Thank God (read Microsoft) for TypeScript.

[nathggns]

Yes, I think a) deprecating defaultProps, and b) changing how you code based on unreleased "plans" are both mistakes.

@ljharb I think I see your point now. My issue is that I agree with deprecating defaultProps and had started writing code using default arguments before we even knew defaultProps was going to be deprecated because using built in language features where possible makes sense to me.

I understand if you insist on not even adding optional support on this, but I may have to fork for the time being (which I would have liked to avoid) and hopefully merge in when defaultProps have been deprecated.

But I also disagree that these are "unreleased plans" – its on an official RFC and they haven't had a comment that's decided them to change their plans yet, and its been a while. These plans seem about as public as they can get. @gaearon seems pretty certain its going to be deprecated from his Twitter feed. This seems like a decision that's been made. https://twitter.com/dan_abramov/status/1133878326358171650

[FredyC]

but I may have to fork for the time being (which I would have liked to avoid) and hopefully merge in when defaultProps have been deprecated.

If you are actually going to write the rule to cover for that, why not to share it and make a PR out of it? I am sure @ljharb is reasonable enough for that. As long as it's opt-in rule for people who want to go that direction, there is no harm in having in the plugin, right? It's feature of the language, it's not about React deprecating anything.

[nathggns]

but I may have to fork for the time being (which I would have liked to avoid) and hopefully merge in when defaultProps have been deprecated.

If you are actually going to write the rule to cover for that, why not to share it and make a PR out of it? I am sure @ljharb is reasonable enough for that. As long as it's opt-in rule for people who want to go that direction, there is no harm in having in the plugin, right?

That's what I've been trying to judge through this issue, is whether a PR would be accepted. But considering this issue is closed, I thought it was worth getting the maintainers to accept there's an issue here before writing the code.

[FredyC]

From my understanding, there is no will to modify current require-default-props rule to check for default function values instead. That's totally ok, it's rule focused on something else. But having a different rule that goes in direction of what's supported in language cannot really harm anything and can be done even if the React team decides to withdraw deprecation. It will always work.

@ljharb do you see some flaw in that logic?

[ljharb]

I think it would be a mistake to enable such a new rule prior to release, just like I think it's a mistake to migrate away from defaultProps prior to that time - but your logic does hold, and I can't stop people from making every kind of mistake.

[FredyC]

I think it would be a mistake to enable such a new rule prior to release,

Can you elaborate? How is it tied to release of anything? It's not a feature of React but the language. It will always work. So it's only a matter of preference and that's what lint rules are about right? Each user can pick own preferences on what code style to enforce.

[ljharb]

Yes, and I think using a language feature that contradicts a framework's feature (while that remains the case) is a mistake.

[FredyC]

Yes, and I think using a language feature that contradicts a framework's feature (while that remains the case) is a mistake.

Perhaps that's one of the reasons why they are deprecating defaultProps 😆 You cannot forbid language feature so to avoid confusion it's easier to remove duplicate approach of declaring defaults. Makes sense imo.

[FredyC]

Coming to think of it, it's not only a matter of confusion, consider following. It's fairly easy to do such a mistake if someone is used to defaultProps.

const Comp = ({ value = 10 }) => {
  return <div>How many: {value}</div>
}
Comp.defaultProps = {
  value: 20
}

Ultimately, when defaultProps are deprecated officially, it might be a good idea to have a rule that warns about this pattern. Unless React will be able to provide a warning of course. But that's worry of the future days :)

[ljharb]

I hope I’ve been clear that as soon as a regular version of react is released that deprecates something, it becomes appropriate for a rule to guide people to better patterns - just, imo, not before.

[nathggns]

I hope I’ve been clear that as soon as a regular version of react is released that deprecates something, it becomes appropriate for a rule to guide people to better patterns - just, imo, not before.

@ljharb So just to be completely clear, would you at this time reject a PR that added a react/require-function-component-default-arguments (there's probably a better name) rule, without enabling it by default?

[ljharb]

Enabling any new rule by default is a semver-major change, that we try to avoid.

I wouldn’t reject a PR for some kind of react/props-defaults-style rule, I’d just want the docs to advise doing what react, and thus the react ecosystem, first-class supports - which at this time is defaultProps.

[bvaughn]

I’d just want the docs to advise doing what react, and thus the react ecosystem, first-class supports - which at this time is defaultProps.

I think this seems a bit misleading. Default parameters are a language feature so React and the ecosystem don't really need to "support" them. Maybe you're referring to the fact that if _both default parameters and defaultProps_ are used, defaultProps will "win"?

[FredyC]

the react ecosystem, first-class supports - which at this time is defaultProps.

No offense, but you might be slightly behind (perhaps even biased?) on what react ecosystem does. Have a look at any new library that uses function components. I tried a few I can recall and haven't seen a defaultProps that much. So I would dare to say that people are getting comfortable with that deprecation fairly quickly :)

https://unpkg.com/[email protected]/cjs/react-router-dom.js
https://unpkg.com/formik@next/dist/formik.umd.development.js _(single one, it's still WIP)_
https://unpkg.com/[email protected]/dist/react-select.cjs.dev.js

[bvaughn]

FYI https://github.com/facebook/react/pull/16210

[FredyC]

@nathggns I wonder will you be able to work on PR for the new rule? Considering the linked PR (now merged), it's confirmed that React 17 will drop defaultProps on functional components.

Also, shouldn't the current rule warn about deprecation? React will warn in runtime, but would be nice to have a warning in linter as well.

@ljharb What are your thoughts here after your involvement in the https://github.com/facebook/react/pull/16210?

[nathggns]

@FredyC I almost feel like there's two rules to come out of this

one to prevent use of defaultProps on function components, and one to ensure that every prop used that isn't required has a default value.

Should we make two new issues for this? I'm not sure if the rules have a naming convention or not.

[FredyC]

@nathggns Well, I am convinced that current require-default-props rule should be tweaked and have deprecation warning for functional components. A new rule to check for default argument values should be made. Otherwise, I think it's too soon to be preparing another enforcing rule until React 17 comes out.

[nathggns]

@FredyC

I am convinced that current require-default-props rule should be tweaked and have deprecation warning for functional components

This seems like an odd place to put such a deprecetation warning, would it not be better to have a no-sfc-default-props rule or something?

[FredyC]

People might have this rule enabled and it is forcing them to set defaultProps on functional components without them knowing it's a deprecated pattern. They would need to consciously be aware of the deprecation to enable a new rule unless it would be added to recommended preset. And even with that, there would be suddenly two contradicting rules.

[nathggns]

@FredyC That's a fair point. Maybe this rule should be deprecated in favour of two new rules – one disabling defaultProps for function components and one requiring them only for class components?

[FredyC]

That's what I was mostly suggesting :)

Well, I am convinced that current require-default-props rule should be tweaked and have deprecation warning for functional components

There is probably no reason for deprecating that rule for class components and disturb users with that.

Only new rule at this point should be about the proper use of default argument values and that's what we have been talking about for a while here ... https://github.com/yannickcr/eslint-plugin-react/issues/1009#issuecomment-510402936

[ljharb]

The first step is that require-default-props should add an option that can disable warning on defaultProps for SFCs. When the react version is 17+, for example, we can default this option to be on.

A different rule should exist that warns when defaultProps is used on SFCs; ideally doing nothing until the version of React that deprecates it (perhaps the next one, but not any of the current ones).

[FredyC]

The first step is that require-default-props should add an option that can disable warning on defaultProps for SFCs. When the react version is 17+, for example, we can default this option to be on.

That option should be enabled by default now because of defaultProps for FC (not SFC anymore) is deprecated now. Why do you want to be misleading users and forcing them to use defaultProps? If they are going to stick to React 16 for a while, they can consciously disable that option.

With React 17+ the rule should emit a hard error when defaultProps on FC is used.

[ljharb]

@FredyC it's not deprecated in a released version of React yet; it's just merged to master.

[FredyC]

And why does that matter? Obviously, the React team is already decided to deprecate it. By delaying it here you are only increasing the number of users who will have to refactor later because by default they will be forced to use defaultProps on FC. It should at least inform them about the existence of that option so they can decide.

Btw, don't forget that most users are probably using not ejected CRA apps. They will have no way to enable that option or disable the rule.

[ljharb]

I'm not delaying anything; I'm not going to add a breaking change for any reason - and for those using tools that rely on defaultProps - like storybook annotations, etc - silently no longer warning on these could break them.

This may not be your use case, but that doesn't mean I'm going to break it.

[FredyC]

What are you talking about? I am not suggesting to be forbidding to use defaultProps. They just shouldn't be required for FC anymore. If someone wants to use them, the rule could just WARN about future deprecation, but that's it. I don't even insist on that warning, but the requirement to use something that's about to be deprecated is just wrong.

[nathggns]

It seems like the happy medium is the following:

• Add an optional no-fc-default-props rule that warns on the use of defaultProps on functional components.
• Add the option to require-default-props to not warn when missing defaultProps on functional components.
• Add no-fc-default-props to recommended when the change deprecating defaultProps on functional components lands in a public release
• Make a new rule that is similar to require-default-props but for functional components & default arguments (require-fc-default-args? probably a better name for this, naming isn't my strong suit)

While some of us including @FredyC & I may like to go further than this, if @ljharb can agree to this bit of work being done now (by pull request or by a core maintainer) it might be worth starting work on what we can agree on rather than letting "perfect" (from our perspective) be the enemy of good.

@ljharb can you confirm that this is a path you'd be supportive of?

[FredyC]

@nathggns Just a tiny detail, please stop using acronym SFC.

You might have previously known these as “stateless components”. We’re now introducing the ability to use React state from these, so we prefer the name “function components”.

https://reactjs.org/docs/hooks-state.html#hooks-and-function-components

[nathggns]

@FredyC ah I know SFC isn't accurate anymore but "function component" is so wordy 😭 updated!

[FredyC]

Well, I got pretty used to FC acronym because it's available in TypeScript declarations :) I think it's getting very well known, there is no need to use a whole thing in rule names for sure.

[ljharb]

I’m going to continue using the term SFC as long as the wider community does; the react team’s naming preference doesn’t constrain anyone else.

@FredyC the issue is that for some use cases, merely omitting defaultProps on an SFC will break things, because tooling requires it be present. That tooling certainly will have to adapt to lacking this information as React releases the deprecation, but until that time, omitting the defaultProps on a single component will break people.

@nathggns that plan is exactly what i was suggesting; except that nothing can be added to recommended without a breaking change, so we won’t be doing that. I’d be comfortable enabling the “don’t warn on missing default props on SFCs” option in the recommended config, however, since that warns less, and anyone depending on airbnb or on every component having defaultProps will not be affected by that.

Let’s do each thing in a separate PR, and naming bikesheds can happen there.

[FredyC]

Oh man, I don't know. It seems like I am either explaining it badly or you are not reading what am I saying.

Let's be practical. If someone uses tooling that relies on defaultProps, they can keep using it. But has to be the user's choice. The rule shouldn't force everyone into doing something that's going to be deprecated. That's just unnecessary stalling.

I’m going to continue using the term SFC as long as the wider community does; the react to team’s naming preference doesn’t constrain anyone else.

So you are going to name rules with sfc and later change it to fc once the trend catches up? How is that a good approach? I don't know why would you be fighting against the official recommendation. It's obvious that React team cannot enforce that, but from a logical standpoint where we don't longer have stateless components, it's wrong.

I agree we should discuss further approaches in separate issues/PRs

[ljharb]

That user is making that choice by using the current rule in master. Silently taking away their choice by defaulting them to a permissive mode is unacceptable.

The hooks have state, but the hooks aren’t the component - imo they’re still forever stateless.

[FredyC]

That user is making that choice by using the current rule in master. Silently taking away their choice by defaulting them to a permissive mode is unacceptable.

There is a problem in that logic. What if you have class components which still needs defaultProps but you also have FC where it would be misleading to have defaultProps enforced. That's why I am suggesting to ease up on the rule for FC components so you still can use the rule for class ones without being constraint toward FC.

Ultimately, this whole rule should be deprecated and there should be separate ones for class and FC, but that's surely a question of some semi-distant future.

The hooks have state, but the hooks aren’t the component - imo they’re still forever stateless.

Uh, what? :) Hooks are not components, that's true, but hooks are used in components thus making them stateful and no longer stateless. Have you ever used hooks seriously? Cannot imagine what would you say something like that 😆

[j-f1]

The deprecation process has started: https://github.com/facebook/react/pull/16210. The warning isn’t actually enabled yet, though.

[ljharb]

@FredyC it's not misleading if you are using tools that break when an SFC lacks defaultProps, as some people are doing now.

[ljharb]

As for SFCs, they were SFCs - stateless - even if they closed over variables, giving them state - the components are not stateful, they just might close over state. In the case of hooks, the hook is doing the same thing.