jsx-eslint / eslint-plugin-react

React-specific linting rules for ESLint
MIT License
8.93k stars 2.77k forks source link

[Bug]: Prop-types rules ignored for lowercase functional components #3806

Open burnhamrobertp opened 3 weeks ago

burnhamrobertp commented 3 weeks ago

Is there an existing issue for this?

Description Overview

Functional components that start with a lower-case letter (const someComponent = ...) are ignored when checking the prop-types rule. This behavior is not observed with similarly named class components.

Several of our codebases lean heavily into React's HoC pattern, resulting in component files that look like:

const someComponent = ({ userId }) => {
 // some code
}

someComponent.propTypes = {
  userId: PropTypes.string.isRequired,
}

const SomeComponent = compose(
  withGlobalContext(['userId']),
  React.memo,
)(someComponent);

export default SomeComponent

This worked for years on a much older version of the plugin/eslint, but has since stopped reporting errors on new functional components that follow this pattern. In the above example, changing someComponent to Test (or anything that starts with a capital letter) fixes the propTypes linting.

Expected Behavior

prop-types rule is enforced even for lowercase functional components; even if it requires a rule configuration to opt into.

eslint-plugin-react version

v7.35.0

eslint version

v8.57.0

node version

v18.20.3

ljharb commented 3 weeks ago

A component, by definition, MUST have a capital letter as the beginning of its name, including with the HoC pattern. I think you just need to stick to that.

burnhamrobertp commented 3 weeks ago

The usage of a component MUST have a capital letter, but not the definition. Moreover, even if this were strictly true it wouldn't explain the discrepancy in the behavior of the plugin between class and functional components.

There is nothing in React that precludes (as bad a pattern as it may be)

const someComponent = () => {
  // ...
}

export default someComponent
import SomeComponent from './some-component';

...

<SomeComponent />
ljharb commented 3 weeks ago

You're entirely correct, but given the limits of static analysis, we've always enforced the universal practice that everyone follows. For classes, we have extends React.Component to help; SFCs have no such syntactic marker.

In my HOC experience, i'd name them SomeComponentInner and SomeComponent, for example.

burnhamrobertp commented 3 weeks ago

You're entirely correct, but given the limits of static analysis, we've always enforced the universal practice that everyone follows. For classes, we have extends React.Component to help; SFCs have no such syntactic marker.

In my HOC experience, i'd name them SomeComponentInner and SomeComponent, for example.

We did this for quite some time; its an awful convention. It also doesn't do anything to address the plugin being internally inconsistent in its behavior between class components and functional components based on casing (an arbitrary way to determine behavior, regardless of React's conventions). Of course, this entire exchange has also failed to provide any explanation as to why the plugin doesn't actually work in the way the documentation specifies it should.

In other words: if this behavior is intended, why is it necessary? Removing this restriction for the enforcement of prop-types warnings/errors during linting wouldn't negatively affect any other user.

ljharb commented 3 weeks ago

Fair point. It's necessary because without this distinction, we get too many false positives on non-component functions - making the entire plugin not useful.

burnhamrobertp commented 3 weeks ago

Would it be considered as an opt-in behavior available via a new option on the prop-types rule, or new value for an existing option?

ljharb commented 2 weeks ago

Sorry if I'm not being clear. Without this heuristic, you'll get react-related warnings on thousands of non-component functions in your codebase.