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

[Bug]: The forbidDefaultForRequired option in the react/require-default-props rule does not work as expected #3720

Open
2 tasks done
vicasas opened this issue Mar 24, 2024 · 6 comments
Labels

Comments

@vicasas
Copy link

vicasas commented Mar 24, 2024

Is there an existing issue for this?

  • I have searched the existing issues and my issue is unique
  • My issue appears in the command-line and not only in the text editor

Description Overview

The forbidDefaultForRequired option in the react/require-default-props rule does not work as expected

Using the react/require-default-props rule together with the forbidDefaultForRequired option set to true does not throw an error or warning when a prop that is required is assigned a default value.

Eslint rule

{
  "rules": {
    "react/require-default-props": ["error", {
      "forbidDefaultForRequired": true,
      "functions": "defaultArguments"
    }],
  }
}

Example code:

function MyStatelessComponent({ foo = 'foo', bar }) {
  return <div>{foo}{bar}</div>;
}

MyStatelessComponent.propTypes = {
  foo: PropTypes.string.isRequired,
  bar: PropTypes.string
};
type MyStatelessComponentProps = {
  foo: string;
  bar?: string
}

function MyStatelessComponent({ foo = 'foo', bar }: MyStatelessComponentProps) {
  return <div>{foo}{bar}</div>;
}

MyStatelessComponent.propTypes = {
  foo: PropTypes.string.isRequired,
  bar: PropTypes.string
};

In the previous examples the foo prop is required, but it is being assigned a default value, with the forbidDefaultForRequired option set totrue, without throwing errors or warnings.

Expected Behavior

The expected behavior is that if a prop is mandatory and a default value is assigned, it should throw an error or warning depending on the rule configuration.

eslint-plugin-react version

7.32.2

eslint version

8

node version

18

@vicasas vicasas added the bug label Mar 24, 2024
@ljharb
Copy link
Member

ljharb commented Mar 24, 2024

The rule forbids default props, not default function parameters. In other words, the .defaultProps object attached to the component.

@vicasas
Copy link
Author

vicasas commented Apr 17, 2024

@ljharb I understand that the current focus is solely on .defaultProps. However, is there any current or future support for performing a different type of validation?

The proposed validation would cover two aspects:

  1. If there are required props, ensure they do not have a default value.
  2. For non-required props, ensure they have a default value.

All of the above should always be validated with the function parameters.

@ljharb
Copy link
Member

ljharb commented Apr 17, 2024

The first one seems like a great enhancement.

The second one, however, i don’t agree with and wouldn’t want to encourage people to do. Despite the react team’s position, defaultProps are much better than default arguments, because they’re introspectable from the outside (and they work on class components).

@vicasas
Copy link
Author

vicasas commented Apr 28, 2024

@ljharb I understand your position regarding point 2, can we revisit it in the future for more feedback on this? I still think it would be nice to provide some support for this, maybe not encourage its use, but have it available as a utility.

Regarding point 1, I think it's great that it can be taken. Would this be validated on default arguments?

@vicasas
Copy link
Author

vicasas commented May 1, 2024

@ljharb Regarding point 1, could we re-evaluate it? Facebook already made changes in React Dom (v18.3.0) to start throwing warnings when .defaultProps is used in functional components, but not for class ones.

See more: facebook/react@5894232

@ljharb
Copy link
Member

ljharb commented May 1, 2024

Yes, with the react team’s very unwise and unfortunate decision in 18.3/19, we might as well add both things.

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

No branches or pull requests

2 participants