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 1 commit
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
33 changes: 29 additions & 4 deletions docs/rules/prefer-stateless-function.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ If none of these elements are found, the rule will warn you to write this compon

The following pattern is considered a warning:

```js
```jsx
var Hello = React.createClass({
render: function() {
return <div>Hello {this.props.name}</div>;
Expand All @@ -26,15 +26,15 @@ var Hello = React.createClass({

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 a warning in React <15.0.0:

```js
```jsx
class Foo extends React.Component {
render() {
if (!this.props.foo) {
Expand All @@ -45,13 +45,38 @@ class Foo extends React.Component {
}
```

The following pattern is not considered a warning:

## 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
34 changes: 14 additions & 20 deletions lib/rules/prefer-stateless-function.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
'use strict';

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

// ------------------------------------------------------------------------------
Expand All @@ -21,13 +20,22 @@ 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 pragma = pragmaUtil.getFromContext(context);
var pureComponentRegExp = new RegExp('^(' + pragma + '\\.)?PureComponent$');
var configuration = context.options[0] || {};
var ignorePureComponents = configuration.ignorePureComponents || false;

var sourceCode = context.getSourceCode();

Expand Down Expand Up @@ -68,19 +76,6 @@ module.exports = {
}
}

/**
* 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 whether a given array of statements is a single call of `super`.
* @see ESLint no-useless-constructor rule
Expand Down Expand Up @@ -282,10 +277,9 @@ module.exports = {

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

// Mark `this` destructuring as a usage of `this`
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
26 changes: 24 additions & 2 deletions tests/lib/rules/prefer-stateless-function.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ ruleTester.run('prefer-stateless-function', rule, {
' }',
'}'
].join('\n'),
parserOptions: parserOptions
parserOptions: parserOptions,
options: [{
ignorePureComponents: true
}]
}, {
// Extends from PureComponent and uses context
code: [
Expand All @@ -57,7 +60,10 @@ ruleTester.run('prefer-stateless-function', rule, {
' }',
'}'
].join('\n'),
parserOptions: parserOptions
parserOptions: parserOptions,
options: [{
ignorePureComponents: true
}]
}, {
// Has a lifecyle method
code: [
Expand Down Expand Up @@ -289,6 +295,22 @@ ruleTester.run('prefer-stateless-function', rule, {
'}'
].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'
}]
Expand Down