New rule proposal: react/prefer-functional-component

cyyynthia commented 3 years ago

With the addition of hooks, it is possible now to write stateful React applications using only functions. However, unless I'm mistaken, there are not ways currently to enforce the use of functional components over class components.

The prefer-stateless-function rule is not enough for this case, since it allows class components provided they have a state or additional methods (like handlers defined as a class methods). It is not possible to forbid the use of state altogether (you can disable setState, but defining state is still allowed, silencing prefer-stateless-function), and filtering class methods using eslint rules without interfering with other code is complicated.

The only exception that should be taken into account are components making use of componentDidCatch, as there are currently no hook alternative for React (although there is a hook if you're using Preact, so it might be worth making this a configurable field).

Depending on how this rule gets implemented, it could even deprecate the prefer-stateless-function rule, in favor of a more generic rule covering more cases. In this case, the configuration for this new rule could follow the following schema:

"react/prefer-functional-component": [ "error", "always | allow-stateful | never" ]

Where always enforces the use of functional components everywhere, allow-stateful behaves as the current prefer-stateless-function rule, and never enforces the use of class components.

ljharb commented 3 years ago

It seems like you could ensure this by configuring existing linter rules to ban an import of Component from 'react', as well as a property of React.Component, although you're right that this wouldn't handle error boundaries.

The reality, though, is that I don't see the benefit of this rule. Just because you can implement something in an SFC doesn't mean you should, just like just because you can implement something in a class component doesn't mean you should.

What is the motivation here to ban class components almost entirely, beyond subjective preference?

cyyynthia commented 3 years ago

Most of the reasons I have in mind follows React's own motivations for introducing hooks (https://reactjs.org/docs/hooks-intro.html#motivation):

Hooks enforce making reusable code, rather than packing everything in a single class component that's heavier (therefore harder to understand), and harder to reuse. You can quickly end up making components that are either wrappers that'd better fit as a hook (and reduce component tree complexity), or start writing subcomponents as class methods (e.g. this.renderHeader, this.renderBody, this.renderFooter), which isn't super reusable and will become an issue in the event of a refactor.

There is also a negative footprint in final bundle sizes when using class components compared to functions, which can for very large apps add up and start being a few kilobytes of additional code to transfer (not a whole lot yes, but it's still preferable to save metered connections and weak networks a few kilobytes!).

ljharb commented 3 years ago

There's still the case of ErrorBoundaries, which must be implemented as a class component, and there's still some lifecycle hook patterns that are messier and more error-prone to write as hooks. I remain unconvinced that this is better done as a lint rule rather than in code review.

tatethurston commented 3 years ago

There's still the case of ErrorBoundaries, which must be implemented as a class component, and there's still some lifecycle hook patterns that are messier and more error-prone to write as hooks. I remain unconvinced that this is better done as a lint rule rather than in code review.

This sounds more like an argument against the inclusion of this rule in the recommended set rather than against its' existence at all.

ljharb commented 3 years ago

@tatethurston it's certainly that as well :-)

A rule that has caveats that you have to "just know" in order to apply the proper overrides shouldn't exist at all. If the proposed rule can avoid false positives, it's worth having.

tatethurston commented 3 years ago

I published eslint-plugin-react-prefer-function-component to accomplish this.

If there is interest down the road to pull this into eslint-plugin-react, I'm happy to PR the work.

ljharb commented 3 years ago

If you can rework it to depend on eslint-plugin-react internals (like component detection, eslint settings, etc) then its tests would be a very compelling argument to upstream it.

tatethurston commented 3 years ago

@ljharb Sure, happy to. I opened https://github.com/tatethurston/eslint-plugin-react-prefer-function-component/pull/1 for the lint rule above if you're interested in taking a look.

RAYDENFilipp commented 3 years ago

@tatethurston Great Work! I really wish it makes into eslint-plugin-react as soon as possible

tmoschou commented 3 years ago

Adding my two-cents. In response to https://github.com/yannickcr/eslint-plugin-react/issues/2860#issuecomment-752319046:

There's still the case of ErrorBoundaries, which must be implemented as a class component, and there's still some lifecycle hook patterns that are messier and more error-prone to write as hooks

Wouldn't you use an eslint ignore statement for these exceptional cases?

I remain unconvinced that this is better done as a lint rule rather than in code review.

I respectfully disagree, code style should not be left to code review if it can be enforced automatically. It can be easily missed at code review, developers go on leave, move on to other things, etc. Further, a rule enforced automatically frees others developers time in code review. Isn't that the whole point of a linter?

It is worth noting that React themselves have written the following sections:

Classes confuse both people and machines

In addition to making code reuse and code organization more difficult, we’ve found that classes can be a large barrier to learning React. ... You have to remember to bind the event handlers. ..., the code is very verbose. People can understand props, state, and top-down data flow perfectly well but still struggle with classes. ... However, we found that class components can encourage unintentional patterns that make these optimizations fall back to a slower path. Classes present issues for today’s tools, too. ...
Should I use Hooks, classes, or a mix of both?

... When you’re ready, we’d encourage you to start trying Hooks in new components you write. ...
Do Hooks cover all use cases for classes?

Our goal is for Hooks to cover all use cases for classes as soon as possible. There are no Hook equivalents to the uncommon getSnapshotBeforeUpdate, getDerivedStateFromError and componentDidCatch lifecycles yet, but we plan to add them soon. ...

nurse-the-code commented 1 year ago

I am not sure why this can't be added as an eslint rule and turned off by default (if people really object to it that much or if it is not appropriate for most people's projects).

Having this rule as an option could allow teams to make contributors consciously choose to use class components for the minority of cases where they are appropriate rather then just using them inappropriately "because that's the way we've always done it." Additionally, some projects may not have any legitimate need for class components and it would be great to be able to enforce this as an automated coding standard.

If anyone needs a temporary solutions, I found this eslint plugin.

tatethurston commented 1 year ago

I am not sure why this can't be added as an eslint rule and turned off by default (if people really object to it that much or if it is not appropriate for most people's projects).

Having this rule as an option could allow teams to make contributors consciously choose to use class components for the minority of cases where they are appropriate rather then just using them inappropriately "because that's the way we've always done it." Additionally, some projects may not have any legitimate need for class components and it would be great to be able to enforce this as an automated coding standard.

If anyone needs a temporary solutions, I found this eslint plugin.

https://github.com/jsx-eslint/eslint-plugin-react/issues/2860#issuecomment-819613519