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

[New Rule] prefer-function-component #3040

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ Enable the rules that you would like to use.
| | | [react/no-will-update-set-state](docs/rules/no-will-update-set-state.md) | Prevent usage of setState in componentWillUpdate |
| | | [react/prefer-es6-class](docs/rules/prefer-es6-class.md) | Enforce ES5 or ES6 class for React Components |
| | | [react/prefer-exact-props](docs/rules/prefer-exact-props.md) | Prefer exact proptype definitions |
| | | [react/prefer-function-component](docs/rules/prefer-function-component.md) | Prefer function components over class components |
| | 🔧 | [react/prefer-read-only-props](docs/rules/prefer-read-only-props.md) | Require read-only props. |
| | | [react/prefer-stateless-function](docs/rules/prefer-stateless-function.md) | Enforce stateless components to be written as a pure function |
| ✔ | | [react/prop-types](docs/rules/prop-types.md) | Prevent missing props validation in a React component definition |
Expand Down
89 changes: 89 additions & 0 deletions docs/rules/prefer-function-component.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
# Prefer function components over class components (react/prefer-function-component)

This rule prevents the use of class components.

Since the addition of hooks, it has been possible to write stateful React components
using only functions. Mixing both class and function components in a code base adds unnecessary hurdles for sharing reusable logic.

By default, class components that use `componentDidCatch` are enabled because there is currently no hook alternative for React. This option is configurable via `allowComponentDidCatch`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not clear on why this is an option. A class component that's an error boundary can never be a functional component (pending changes from React itself), so there'd never be any value in this rule warning on it. I think that this option shouldn't be an option, it should just be hardcoded to true.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many codebases I've seen don't implement error boundaries from scratch in their code base, instead they import library solutions that use error boundaries internally. I made this configurable (though with the default value of true so most clients can ignore it) for clients who want all class component usage to be flagged. This also provides a migration path should React release functional component error boundaries.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, Preact does offer functional error boundaries via a hook, so this would be valuable for those users.


## Rule Details

This rule will flag any React class components that don't use `componentDidCatch`.

Examples of **incorrect** code for this rule:

```jsx
import { Component } from 'react';

class Foo extends Component {
render() {
return <div>{this.props.foo}</div>;
}
}
```

Examples of **correct** code for this rule:

```jsx
const Foo = function (props) {
return <div>{props.foo}</div>;
};
```

```jsx
const Foo = ({ foo }) => <div>{foo}</div>;
```

## Rule Options

```js
...
"prefer-function-component": [<enabled>, { "allowComponentDidCatch": <allowComponentDidCatch> }]
...
```

- `enabled`: for enabling the rule. 0=off, 1=warn, 2=error. Defaults to 0.
- `allowComponentDidCatch`: optional boolean set to `true` if you would like to ignore class components using `componentDidCatch` (default to `true`).

### `allowComponentDidCatch`

When `true` (the default) the rule will ignore components that use `componentDidCatch`

Examples of **correct** code for this rule:

```jsx
import { Component } from 'react';

class Foo extends Component {
componentDidCatch(error, errorInfo) {
logErrorToMyService(error, errorInfo);
}

render() {
return <div>{this.props.foo}</div>;
}
}
```

When `false` the rule will also flag components that use `componentDidCatch`

Examples of **incorrect** code for this rule:

```jsx
import { Component } from 'react';

class Foo extends Component {
componentDidCatch(error, errorInfo) {
logErrorToMyService(error, errorInfo);
}

render() {
return <div>{this.props.foo}</div>;
}
}
```

### Related rules

- [prefer-stateless-function](./prefer-stateless-function)
1 change: 1 addition & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ const allRules = {
'no-will-update-set-state': require('./lib/rules/no-will-update-set-state'),
'prefer-es6-class': require('./lib/rules/prefer-es6-class'),
'prefer-exact-props': require('./lib/rules/prefer-exact-props'),
'prefer-function-component': require('./lib/rules/prefer-function-component'),
'prefer-read-only-props': require('./lib/rules/prefer-read-only-props'),
'prefer-stateless-function': require('./lib/rules/prefer-stateless-function'),
'prop-types': require('./lib/rules/prop-types'),
Expand Down
86 changes: 86 additions & 0 deletions lib/rules/prefer-function-component.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/**
* @fileoverview Enforce function components over class components
* @author Tate Thurston
*/

'use strict';

const docsUrl = require('../util/docsUrl');
const Components = require('../util/Components');
const ast = require('../util/ast');

const COMPONENT_SHOULD_BE_FUNCTION = 'componentShouldBeFunction';
const ALLOW_COMPONENT_DID_CATCH = 'allowComponentDidCatch';
const COMPONENT_DID_CATCH = 'componentDidCatch';

module.exports = {
meta: {
docs: {
description: 'Enforce components are written as function components',
category: 'Stylistic Issues',
recommended: false,
suggestion: false,
url: docsUrl('prefer-function-component')
},
fixable: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason this rule isn't fixable?

Any component with a constructor body or class methods, of course, couldn't be fixable, but the example in the readme could.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially fixable seemed odd to me on first thought, but the same logic as used by prefer-stateless-function could be used here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of rules are partially fixable; obv it's ideal when things are fully fixable tho.

Actually wait, I'd forgotten about prefer-stateless-function. How is this rule different from that one? https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/prefer-stateless-function.md

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #2860 explains the difference well:

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see, so this is a stronger thing than that, where prefer-stateless-function pushes SFCs when possible, this rule would ban class components entirely in favor of hook-using SFCs. Gotcha.

type: 'problem',
messages: {
[COMPONENT_SHOULD_BE_FUNCTION]:
'Class component should be written as a function'
},
schema: [
{
type: 'object',
properties: {
[ALLOW_COMPONENT_DID_CATCH]: {
default: true,
type: 'boolean'
}
},
additionalProperties: false
}
]
},

create: Components.detect((context, components, utils) => {
const allowComponentDidCatchOption = context.options[0] && context.options[0].allowComponentDidCatch;
const allowComponentDidCatch = allowComponentDidCatchOption !== false;

function shouldPreferFunction(node) {
if (!allowComponentDidCatch) {
return true;
}

const properties = ast
.getComponentProperties(node)
.map(ast.getPropertyName);
return !properties.includes(COMPONENT_DID_CATCH);
}

const detect = (guard) => (node) => {
if (guard(node) && shouldPreferFunction(node)) {
components.set(node, {
[COMPONENT_SHOULD_BE_FUNCTION]: true
});
}
};

return {
ObjectExpression: detect(utils.isES5Component),
ClassDeclaration: detect(utils.isES6Component),
ClassExpression: detect(utils.isES6Component),

'Program:exit'() {
const list = components.list();
Object.values(list).forEach((component) => {
if (component[COMPONENT_SHOULD_BE_FUNCTION]) {
context.report({
node: component.node,
messageId: COMPONENT_SHOULD_BE_FUNCTION
});
}
});
}
};
})
};
Loading