Make prefer-stateless-function PureComponent aware #781

teameh · 2016-08-21T11:39:57Z

prefer-stateless-function should not return a warning when the user uses PureComponent to opt in for free optimisations with shouldComponentUpdate and is using props or context.

Current false positive:

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

I first only added the same isPureComponent check as require-optimization does. But realized that components that are extending form PureComponent but don't use this.props or this.context should be written as a pure function as well. So I had to change the existing checks a bit to check that too.

Yeah I know, I could have first reacted to the discussion at #777, but I thought it would be cool to find out how all the 'rules' in eslint work by trying to fix this issue. Hope you agree about the false positive. I think this case is different from #777 because that case it not using props or context at all.

Please correct any formatting fails, first contribution here.

lencioni · 2016-08-21T15:35:18Z

lib/rules/prefer-stateless-function.js

+      }
+
+      return false;
+    };


I wonder if this function should go in lib/util/Components.js

lencioni · 2016-08-21T15:41:44Z

This seems like a good change to me. I think it could be DRYed up a little with some of the code borrowed from other rules.

The only question I have is--should this be the behavior for everyone, or should it be an option? I'm inclined to think it can be the behavior for everyone for the sake of simplicity, but I'd like to hear opinions on that.

teameh · 2016-08-21T16:10:33Z

Thanks for the quick response.

Yeah I changed as little as possible. Just let me know if you want me to move some of it to the utils. That makes sense.

ljharb · 2016-08-21T16:35:01Z

docs/rules/prefer-stateless-function.md

 * 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 5 elements are found, the rule will warn you to write this component as a pure function.


Let's just make this say "these elements" to avoid having to update the count in the future.

ljharb · 2016-08-21T16:36:52Z

I'm -1 on this, per #777.

teameh · 2016-08-21T16:55:03Z

on #777

You should be preferring a stateless function there, since there's no state. "PureComponent" usage doesn't affect this rule by design.

But if the user chose to use PureComponent instead of Component and uses props or context, doesn't that indicate that the user want's the component to only update if the props or context changes? And that's not possible with a stateless component.

And if this is not desired then there should also be a warning when the user implemented shouldComponentUpdate or used the PureRenderMixin right? And both are not triggering a warning now.

ljharb · 2016-08-21T16:59:33Z

If the user wants to convey their intentions to ignore this rule that prefers components without state be SFCs, they can do that with an eslint override.

While I agree that implementing shouldComponentUpdate means that the component can't be blindly made into an SFC, I think that if a React.createClass implements PureRenderMixin, but in all other respects should be an SFC - that it should be a warning.

lencioni · 2016-08-21T18:42:21Z

Yeah I was originally thinking that using PureComponent was a strong indication that the developer prefers this over an SFC, but taking a step back I don't think that's accurate. I agree that this should be handled on a case by case basis with an eslint override and not by weakening this rule.

teameh · 2016-08-21T21:02:42Z

Okay, fair enough.

But currently it's not possible to globally override this rule in .eslintrc so that it doesn't trigger a warning when PureComponent is used right?

ljharb · 2016-08-22T04:08:53Z

@tiemevanveen that's true - making an option to prefer-stateless-function like "ignorePureComponents" might be worth doing, however?

teameh · 2016-08-22T13:59:37Z

Added the option. Also moved isPureComponent(node) to the utils in Components.js.

teameh · 2016-08-22T14:01:13Z

lib/util/Components.js

     * Check if the node is returning JSX
     *
-     * @param {ASTNode} ASTnode The AST node being checked
+     * @param {ASTNode} node The AST node being checked


all the other methods say 'node'..

This token should match the argument name, so you should update line 222 at the same time.

Check. And that's not possible because node is reassigned afterwards. Changed back tot original value. Thanks.

teameh · 2016-08-22T16:00:44Z

You're right, thanks.

Fixed and added to last commit.

Also removed the === true form 'no-multi-comp' where I stole the example from in the first place ;)

yannickcr · 2016-08-22T19:45:39Z

docs/rules/prefer-stateless-function.md

+
+### `ignorePureComponent`
+
+When `true` the rule will ignore Components extending from `React.PureComponent` that use `this.props` or `this.context`.


Can you indicate the default value here please?

Done (added to commit).

forgot to push.. done now

lencioni · 2016-08-23T04:06:46Z

docs/rules/prefer-stateless-function.md

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


nit: please make these jsx instead of js, for best syntax highlighting

yannickcr · 2016-08-23T21:01:00Z

Merged, thanks!

teameh · 2016-08-23T21:08:03Z

Thanks!

swarthy · 2016-08-29T06:06:30Z

docs/rules/prefer-stateless-function.md


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


ignorePureComponents not ignorePureComponent

Thanks, fixed in 9f76459

lencioni reviewed Aug 21, 2016
View reviewed changes

lencioni added enhancement rule labels Aug 21, 2016

ljharb reviewed Aug 21, 2016
View reviewed changes

teameh force-pushed the master branch from 74236ca to 0ff3404 Compare August 21, 2016 16:57

Make prefer-stateless-function PureComponent aware

6eabed5

teameh force-pushed the master branch from 0ff3404 to 6eabed5 Compare August 21, 2016 17:04

teameh reviewed Aug 22, 2016
View reviewed changes

teameh force-pushed the master branch from 6f6f90a to 1a59015 Compare August 22, 2016 15:58

yannickcr reviewed Aug 22, 2016
View reviewed changes

teameh force-pushed the master branch from 1a59015 to 3d75ff6 Compare August 22, 2016 19:56

lencioni reviewed Aug 23, 2016
View reviewed changes

teameh force-pushed the master branch from 3d75ff6 to f6f66c7 Compare August 23, 2016 10:35

add ignorePureComponents option to prefer-stateless-function

1482f74

teameh force-pushed the master branch from f6f66c7 to 1482f74 Compare August 23, 2016 10:38

yannickcr merged commit 7024678 into jsx-eslint:master Aug 23, 2016

swarthy reviewed Aug 29, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make prefer-stateless-function PureComponent aware #781

Make prefer-stateless-function PureComponent aware #781

teameh commented Aug 21, 2016

lencioni Aug 21, 2016

ljharb Aug 21, 2016

lencioni commented Aug 21, 2016

teameh commented Aug 21, 2016

ljharb Aug 21, 2016

ljharb commented Aug 21, 2016

teameh commented Aug 21, 2016

ljharb commented Aug 21, 2016

lencioni commented Aug 21, 2016 •

edited by ljharb

Loading

teameh commented Aug 21, 2016

ljharb commented Aug 22, 2016

teameh commented Aug 22, 2016

teameh Aug 22, 2016

lencioni Aug 23, 2016

teameh Aug 23, 2016

teameh commented Aug 22, 2016

yannickcr Aug 22, 2016

teameh Aug 22, 2016

teameh Aug 23, 2016

lencioni Aug 23, 2016

teameh Aug 23, 2016

yannickcr commented Aug 23, 2016

teameh commented Aug 23, 2016

swarthy Aug 29, 2016

ljharb Aug 29, 2016


		### `ignorePureComponent`

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


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

		```js

Make prefer-stateless-function PureComponent aware #781

Make prefer-stateless-function PureComponent aware #781

Conversation

teameh commented Aug 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lencioni commented Aug 21, 2016

teameh commented Aug 21, 2016

Choose a reason for hiding this comment

ljharb commented Aug 21, 2016

teameh commented Aug 21, 2016

ljharb commented Aug 21, 2016

lencioni commented Aug 21, 2016 • edited by ljharb Loading

teameh commented Aug 21, 2016

ljharb commented Aug 22, 2016

teameh commented Aug 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teameh commented Aug 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannickcr commented Aug 23, 2016

teameh commented Aug 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lencioni commented Aug 21, 2016 •

edited by ljharb

Loading