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

Make prefer-stateless-function PureComponent aware #781

Merged
merged 2 commits into from
Aug 23, 2016
Merged
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
51 changes: 44 additions & 7 deletions docs/rules/prefer-stateless-function.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,32 +8,33 @@ This rule will check your class based React components for

* methods/properties other than `displayName`, `propTypes`, `render` and useless constructor (same detection as ESLint [no-useless-constructor rule](http://eslint.org/docs/rules/no-useless-constructor))
* instance property other than `this.props` and `this.context`
* extension of `React.PureComponent` ()
* presence of `ref` attribute in JSX
* `render` method that return anything but JSX: `undefined`, `null`, etc. (only in React <15.0.0, see [shared settings](https://github.com/yannickcr/eslint-plugin-react/blob/master/README.md#configuration) for React version configuration)

If none of these 4 elements are found, the rule will warn you to write this component as a pure function.
If none of these elements are found, the rule will warn you to write this component as a pure function.

The following pattern is considered warnings:
The following pattern is considered a warning:

```js
```jsx
var Hello = React.createClass({
render: function() {
return <div>Hello {this.props.name}</div>;
}
});
```

The following pattern is not considered warnings:
The following pattern is not considered a warning:

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

The following pattern is not considered warning in React <15.0.0:
The following pattern is not considered a warning in React <15.0.0:

```js
```jsx
class Foo extends React.Component {
render() {
if (!this.props.foo) {
Expand All @@ -43,3 +44,39 @@ class Foo extends React.Component {
}
}
```


## Rule Options

```js
...
"prefer-stateless-function": [<enabled>, { "ignorePureComponent": <ignorePureComponent> }]
Copy link

Choose a reason for hiding this comment

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

ignorePureComponents not ignorePureComponent

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, fixed in 9f76459

...
```

* `enabled`: for enabling the rule. 0=off, 1=warn, 2=error. Defaults to 0.
* `ignorePureComponent`: optional boolean set to `true` to ignore components extending from `React.PureComponent` (default to `false`).
Copy link

Choose a reason for hiding this comment

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

ignorePureComponents

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, fixed in 9f76459


### `ignorePureComponent`
Copy link

Choose a reason for hiding this comment

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

ignorePureComponents

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, fixed in 9f76459


When `true` the rule will ignore Components extending from `React.PureComponent` that use `this.props` or `this.context`.
Copy link
Member

Choose a reason for hiding this comment

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

Can you indicate the default value here please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (added to commit).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

forgot to push.. done now


The following patterns is considered okay and does not cause warnings:

```jsx
class Foo extends React.PureComponent {
Copy link
Member

Choose a reason for hiding this comment

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

I still think this pattern should be considered a warning - being a PureComponent does not mean this is better as a stateful component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Being a PureComponent does not make it a stateful component right? It just makes it benefit from shouldComponentUpdate

Copy link
Member

Choose a reason for hiding this comment

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

Being a PureComponent does not make it stateless either - the two things are orthogonal. I'm saying prefer-stateless-function is far more important than prefer-optimization.

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

The following pattern is considered a warning because it's not using props or context:

```jsx
class Foo extends React.PureComponent {
render() {
return <div>Bar</div>;
}
}
```
2 changes: 1 addition & 1 deletion lib/rules/no-multi-comp.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ module.exports = {
* @returns {Boolean} True if the component is ignored, false if not.
*/
function isIgnored(component) {
return ignoreStateless === true && /Function/.test(component.node.type);
return ignoreStateless && /Function/.test(component.node.type);
}

// --------------------------------------------------------------------------
Expand Down
51 changes: 48 additions & 3 deletions lib/rules/prefer-stateless-function.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,23 @@ module.exports = {
category: 'Stylistic Issues',
recommended: false
},
schema: []
schema: [{
type: 'object',
properties: {
ignorePureComponents: {
default: false,
type: 'boolean'
}
},
additionalProperties: false
}]
},

create: Components.detect(function(context, components, utils) {

var configuration = context.options[0] || {};
var ignorePureComponents = configuration.ignorePureComponents || false;

var sourceCode = context.getSourceCode();

// --------------------------------------------------------------------------
Expand Down Expand Up @@ -213,6 +225,16 @@ module.exports = {
});
}

/**
* Mark component as pure as declared
* @param {ASTNode} node The AST node being checked.
*/
var markSCUAsDeclared = function (node) {
components.set(node, {
hasSCU: true
});
};

/**
* Mark a setState as used
* @param {ASTNode} node The AST node being checked.
Expand All @@ -223,6 +245,16 @@ module.exports = {
});
}

/**
* Mark a props or context as used
* @param {ASTNode} node The AST node being checked.
*/
function markPropsOrContextAsUsed(node) {
components.set(node, {
usePropsOrContext: true
});
}

/**
* Mark a ref as used
* @param {ASTNode} node The AST node being checked.
Expand All @@ -244,6 +276,12 @@ module.exports = {
}

return {
ClassDeclaration: function (node) {
if (ignorePureComponents && utils.isPureComponent(node)) {
markSCUAsDeclared(node);
}
},

// Mark `this` destructuring as a usage of `this`
VariableDeclarator: function(node) {
// Ignore destructuring on other than `this`
Expand All @@ -256,6 +294,7 @@ module.exports = {
return name !== 'props' && name !== 'context';
});
if (!useThis) {
markPropsOrContextAsUsed(node);
return;
}
markThisAsUsed(node);
Expand All @@ -264,11 +303,13 @@ module.exports = {
// Mark `this` usage
MemberExpression: function(node) {
// Ignore calls to `this.props` and `this.context`
if (
node.object.type !== 'ThisExpression' ||
if (node.object.type !== 'ThisExpression') {
return;
} else if (
(node.property.name || node.property.value) === 'props' ||
(node.property.name || node.property.value) === 'context'
) {
markPropsOrContextAsUsed(node);
return;
}
markThisAsUsed(node);
Expand Down Expand Up @@ -322,6 +363,10 @@ module.exports = {
continue;
}

if (list[component].hasSCU && list[component].usePropsOrContext) {
continue;
}

context.report({
node: list[component].node,
message: 'Component should be written as a pure function'
Expand Down
22 changes: 2 additions & 20 deletions lib/rules/require-optimization.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
'use strict';

var Components = require('../util/Components');
var pragmaUtil = require('../util/pragma');

module.exports = {
meta: {
Expand All @@ -29,15 +28,11 @@ module.exports = {
}]
},

create: Components.detect(function (context, components) {
create: Components.detect(function (context, components, utils) {
var MISSING_MESSAGE = 'Component is not optimized. Please add a shouldComponentUpdate method.';
var configuration = context.options[0] || {};
var allowDecorators = configuration.allowDecorators || [];

var pragma = pragmaUtil.getFromContext(context);
var pureComponentRegExp = new RegExp('^(' + pragma + '\\.)?PureComponent$');
var sourceCode = context.getSourceCode();

/**
* Checks to see if our component is decorated by PureRenderMixin via reactMixin
* @param {ASTNode} node The AST node being checked.
Expand Down Expand Up @@ -89,19 +84,6 @@ module.exports = {
return false;
};

/**
* Checks to see if our component extends React.PureComponent
* @param {ASTNode} node The AST node being checked.
* @returns {Boolean} True if node extends React.PureComponent, false if not.
*/
var isPureComponent = function (node) {
if (node.superClass) {
return pureComponentRegExp.test(sourceCode.getText(node.superClass));
}

return false;
};

/**
* Checks if we are declaring a shouldComponentUpdate method
* @param {ASTNode} node The AST node being checked.
Expand Down Expand Up @@ -186,7 +168,7 @@ module.exports = {
},

ClassDeclaration: function (node) {
if (!(hasPureRenderDecorator(node) || hasCustomDecorator(node) || isPureComponent(node))) {
if (!(hasPureRenderDecorator(node) || hasCustomDecorator(node) || utils.isPureComponent(node))) {
return;
}
markSCUAsDeclared(node);
Expand Down
13 changes: 13 additions & 0 deletions lib/util/Components.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,19 @@ function componentRule(rule, context) {
return relevantTags.length > 0;
},

/**
* Checks to see if our component extends React.PureComponent
*
* @param {ASTNode} node The AST node being checked.
* @returns {Boolean} True if node extends React.PureComponent, false if not
*/
isPureComponent: function (node) {
if (node.superClass) {
return new RegExp('^(' + pragma + '\\.)?PureComponent$').test(sourceCode.getText(node.superClass));
}
return false;
},

/**
* Check if the node is returning JSX
*
Expand Down
55 changes: 55 additions & 0 deletions tests/lib/rules/prefer-stateless-function.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,32 @@ ruleTester.run('prefer-stateless-function', rule, {
// Already a stateless (arrow) function
code: 'const Foo = ({foo}) => <div>{foo}</div>;',
parserOptions: parserOptions
}, {
// Extends from PureComponent and uses props
code: [
'class Foo extends React.PureComponent {',
' render() {',
' return <div>{this.props.foo}</div>;',
' }',
'}'
].join('\n'),
parserOptions: parserOptions,
options: [{
ignorePureComponents: true
}]
}, {
// Extends from PureComponent and uses context
code: [
'class Foo extends React.PureComponent {',
' render() {',
' return <div>{this.context.foo}</div>;',
' }',
'}'
].join('\n'),
parserOptions: parserOptions,
options: [{
ignorePureComponents: true
}]
}, {
// Has a lifecyle method
code: [
Expand Down Expand Up @@ -259,6 +285,35 @@ ruleTester.run('prefer-stateless-function', rule, {
errors: [{
message: 'Component should be written as a pure function'
}]
}, {
// Only extend PureComponent without use of props or context
code: [
'class Foo extends React.PureComponent {',
' render() {',
' return <div>foo</div>;',
' }',
'}'
].join('\n'),
parserOptions: parserOptions,
options: [{
ignorePureComponents: true
}],
errors: [{
message: 'Component should be written as a pure function'
}]
}, {
// Extends from PureComponent but no ignorePureComponents option
code: [
'class Foo extends React.PureComponent {',
' render() {',
' return <div>{this.props.foo}</div>;',
' }',
'}'
].join('\n'),
parserOptions: parserOptions,
errors: [{
message: 'Component should be written as a pure function'
}]
}, {
// Has only displayName (method) and render
code: [
Expand Down