[New] add `no-unused-class-component-methods` #2239

pawelnvk · 2019-04-13T19:01:13Z

Display errors in case class component contains unused methods.

ljharb

Thanks, this is a great start. Let's add a lot more test cases tho :-)

ljharb · 2019-04-16T21:25:05Z

lib/rules/no-unused-class-component-methods.js

+// Rule Definition
+// ------------------------------------------------------------------------------
+
+const internalMethods = [


what about unused static methods?

Should we be concerned about static methods? They doesn't seem to be closely connected to component. I feel that static methods could be used outside of component and in that case it would be harder to determine if method is used..

In a react component, yes - it's not meant to be used as a class.

(An instance method could also be used outside of the component)

Perhaps maybe an option to control checking of static methods?

That might be a good idea. I'll try to tackle it after basic version.

lib/rules/no-unused-class-component-methods.js

ljharb · 2019-04-16T21:27:54Z

lib/rules/no-unused-class-component-methods.js

+      const isMethod = node.type === 'MethodDefinition';
+      const isArrowFunction = (
+        node.type === 'ClassProperty' &&
+        node.value.type === 'ArrowFunctionExpression'


what about async functions, async arrow functions, generators, and async generators?

golopot · 2019-04-19T17:20:43Z

tests/lib/rules/no-unused-class-component-methods.js

+        class Foo extends React.Component {
+          action() {}
+          componentDidMount() {
+            const action = this.action;


What about use by object destructuring?

ping on this one

cc @meowtec

pawelnvk · 2019-04-26T10:49:30Z

Thanks for feedback. Currently I'm taking some time off, but I'll definitely get back to it next week. I'm super excited about it!

ha404 · 2019-05-03T05:43:00Z

Would this also account for functional components?

golopot · 2019-05-03T05:45:58Z

@ha404 It shouldn't. no-unused-vars should suffice in the case of functional components.

ha404 · 2019-05-03T06:26:54Z

@golopot but you can add static properties to functional components too.

const SomeComponent = () => ...

SomeComponent.propTypes = ...
SomeComponent.anythingYouWant = ...

lib/rules/no-unused-class-component-methods.js

Kiwka · 2019-08-12T11:54:51Z

Is there anything I can do to help to get this to mergeable state?

ljharb · 2019-08-15T22:59:05Z

@Kiwka if you post a link here to a branch that addresses the review comments and test failures here (please do NOT create a new PR) then I can pull in your commits, and we can definitely get this in.

valtido · 2019-08-22T16:11:10Z

Looks good, can we look at those tests, please? desperado for this... :) (y)

meowtec · 2019-09-15T15:22:18Z

tests/lib/rules/no-unused-class-component-methods.js

+    {
+      code: `
+        class Foo extends React.Component {
+          property = {}


Why is it valid?

Could it also check unused property?

That's a good idea; the rule should check both properties and methods.

meowtec · 2019-09-20T07:10:07Z

Still in progress?

ljharb · 2019-10-08T21:09:38Z

ping @pawelnvk

valtido · 2019-10-09T11:07:24Z

lib/rules/no-unused-class-component-methods.js

+          case 'ClassProperty':
+          case 'JSXAttribute':
+          case 'MethodDefinition':
+            getThisExpressions(subnode.value);


Consider using the Object switch methodology rather than an actual switch, in my view, it's a lot faster than the switch.

See this link for a reference.
https://stackoverflow.com/questions/37730199/switch-vs-object-lookup-performance-since-jsperf-is-down

Faster or not, it's always cleaner :-)

meowtec · 2019-10-21T08:49:46Z

It seams that this PR has been abandoned.

I have (re)written this rule with another implementation and use it in our project which has over 100k lines of code.

see here https://github.com/meowtec/eslint-plugin-react-ext/blob/master/lib/rules/no-unused-class-property.js

ljharb · 2019-10-22T02:55:15Z

@meowtec if you'd be able to make those changes in a branch on a fork - not a PR - then I can look into pulling them into this PR.

meowtec · 2019-10-22T08:01:51Z

@ljharb My branch is at https://github.com/meowtec/eslint-plugin-react/tree/unused-component-methods-improve

What are different:

The new implementation inspired by no-unused-state.
Check both properties and methods.
Remove the support for the legacy ES5 style React.createClass. If necessary I will add it back.
Static methods are just ignored.
More test cases and bugfix.

@pawelnvk Please take a look because I am not sure if any important things was missing.

ref: #2239 (comment)

ljharb · 2019-11-29T06:00:05Z

Yes, please add createClass support back; we'll be supporting it for the foreseeable future.

ljharb

Let's add tests for state = {} and ensure that unused state is ignored.

ljharb · 2019-11-29T06:07:02Z

docs/rules/no-unused-class-component-methods.md

+}
+```
+
+The following patterns are **not** considered warnings:


let's add some unused static methods here, so it's clear.

cc @meowtec

cc @meowtec

should all static methods be list here?

Is there more than one? Either way we'd only need at least one as an example.

ljharb · 2019-11-29T06:07:49Z

tests/lib/rules/no-unused-class-component-methods.js

+        class Foo extends React.Component {
+          action() {}
+          componentDidMount() {
+            const action = this.action;


ping on this one

meowtec

Yes, please add createClass support back; we'll be supporting it for the foreseeable future.

https://github.com/meowtec/eslint-plugin-react/tree/unused-component-methods

add createReactClass back
ignore unused state

meowtec · 2019-09-16T03:20:23Z

lib/rules/no-unused-class-component-methods.js

+          return;
+        }
+
+        switch (subnode.type) {


These codes look verbose and many cases were lost, such as BinaryExpression arguments ArrayExpression, ObjectExpression, etc.

meowtec · 2019-09-16T03:24:54Z

lib/rules/no-unused-class-component-methods.js

+          case 'ClassProperty':
+          case 'JSXAttribute':
+          case 'MethodDefinition':
+            getThisExpressions(subnode.value);


subnode.value could be null so it will throw with error at the next switch (subnode.type).

eg:

<button disabled /> class Foo extends React.Component { a; }

Fixed and added some test cases: meowtec@dd88dc1

meowtec · 2019-09-16T06:25:49Z

lib/rules/no-unused-class-component-methods.js

+            getThisExpressions(subnode.init);
+            break;
+          case 'MemberExpression':
+            if (subnode.object.type !== 'ThisExpression') {


Should exclude the computed property:

class Foo extends React.Component { handleClick() {} render() { return <button onClick={this[handleClick]}>Text</button>; } }

jakeleventhal · 2020-02-24T01:50:22Z

Bumping

ljharb · 2021-08-09T21:03:33Z

@pawelnvk are you still interested in landing this PR? if so, it could use a rebase and there's some review comments to address :-)

jakeleventhal · 2021-09-14T13:42:38Z

@pawelnvk are you going to do this?
If not, I might do it @ljharb

ljharb · 2021-09-14T18:01:45Z

@jakeleventhal that'd be great! however, please do NOT open a new PR; please comment here with a link to your branch, and I'll pull in the changes.

Co-authored-by: meowtec <bertonzh@gmail.com> Co-authored-by: Jake Leventhal <jakeleventhal@me.com>

jakeleventhal · 2021-09-16T12:38:22Z

@ljharb https://github.com/jakeleventhal/eslint-plugin-react/tree/jakeleventhal/no-unused-class-component-methods

ljharb · 2021-09-16T22:43:59Z

@jakeleventhal thanks! Pulling it in now. I do note that it's still missing support for the static getDerivedStateFromProps method.

codecov-commenter · 2021-09-16T23:07:40Z

Codecov Report

Merging #2239 (7114353) into master (9c1aee5) will increase coverage by 0.00%.
The diff coverage is 97.84%.

@@           Coverage Diff           @@
##           master    #2239   +/-   ##
=======================================
  Coverage   97.43%   97.44%           
=======================================
  Files         111      112    +1     
  Lines        7529     7622   +93     
  Branches     2769     2798   +29     
=======================================
+ Hits         7336     7427   +91     
- Misses        193      195    +2

Impacted Files	Coverage Δ
index.js	`100.00% <ø> (ø)`
lib/rules/no-unused-class-component-methods.js	`97.84% <97.84%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c1aee5...7114353. Read the comment docs.

jakeleventhal · 2021-09-18T02:35:53Z

@ljharb im a bit unfamiliar with this repo. do you think you could just take this over since you're pulling in the change anyway? i would appreciate it

jakeleventhal · 2021-09-18T02:36:18Z

@ljharb also, does this cover hooks?

jakeleventhal · 2021-09-25T00:29:23Z

@ljharb bump

ljharb · 2021-09-26T05:38:39Z

Sure, I can try to add it.

No, this doesn't cover hooks; the rule only has to do with class components. SFCs don't have methods.

jakeleventhal · 2021-09-29T12:20:03Z

@ljharb okay, thank you. I will leave it to you to finish then i suppose

pawelnvk force-pushed the unused-component-methods branch from a537eeb to 1aaa6c9 Compare April 13, 2019 19:08

pawelnvk mentioned this pull request Apr 13, 2019

react/no-unused-class-comp-methods #2166

Closed

ljharb requested changes Apr 16, 2019

View reviewed changes

ljharb added the new rule label Apr 16, 2019

golopot reviewed Apr 19, 2019

View reviewed changes

ljharb reviewed May 11, 2019

View reviewed changes

lib/rules/no-unused-class-component-methods.js Outdated Show resolved Hide resolved

ljharb force-pushed the unused-component-methods branch from ffff596 to c4dd9d0 Compare August 31, 2019 05:40

This comment has been minimized.

Sign in to view

meowtec reviewed Sep 15, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

valtido reviewed Oct 9, 2019

View reviewed changes

ljharb force-pushed the unused-component-methods branch from c4dd9d0 to e84c99e Compare November 29, 2019 06:02

ljharb requested changes Nov 29, 2019

View reviewed changes

meowtec reviewed Dec 6, 2019

View reviewed changes

ljharb force-pushed the unused-component-methods branch from 32857ec to aff2f24 Compare December 8, 2019 07:32

ljharb marked this pull request as draft August 9, 2021 21:03

[New] add no-unused-class-component-methods

1371f71

Co-authored-by: meowtec <bertonzh@gmail.com> Co-authored-by: Jake Leventhal <jakeleventhal@me.com>

ljharb force-pushed the unused-component-methods branch 2 times, most recently from e51f8d7 to 7114353 Compare September 16, 2021 22:54

ljharb changed the title ~~[Feat] - Detect unused class component method~~ [New] add no-unused-class-component-methods Sep 16, 2021

ljharb assigned jakeleventhal Sep 18, 2021

ljharb force-pushed the unused-component-methods branch 2 times, most recently from 08a6f69 to 1371f71 Compare September 30, 2021 06:20

ljharb approved these changes Sep 30, 2021

View reviewed changes

ljharb marked this pull request as ready for review September 30, 2021 06:20

ljharb merged commit 1371f71 into jsx-eslint:master Sep 30, 2021

[New] add no-unused-class-component-methods #2239

[New] add no-unused-class-component-methods #2239

Conversation

pawelnvk commented Apr 13, 2019 • edited by ljharb Loading

ljharb left a 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

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

pawelnvk commented Apr 26, 2019

ha404 commented May 3, 2019

golopot commented May 3, 2019

ha404 commented May 3, 2019

Kiwka commented Aug 12, 2019

ljharb commented Aug 15, 2019

valtido commented Aug 22, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

meowtec commented Sep 20, 2019

ljharb commented Oct 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meowtec commented Oct 21, 2019

ljharb commented Oct 22, 2019

meowtec commented Oct 22, 2019 • edited Loading

ljharb commented Nov 29, 2019

ljharb left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meowtec left a 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

jakeleventhal commented Feb 24, 2020

ljharb commented Aug 9, 2021

jakeleventhal commented Sep 14, 2021

ljharb commented Sep 14, 2021

jakeleventhal commented Sep 16, 2021

ljharb commented Sep 16, 2021

codecov-commenter commented Sep 16, 2021 • edited Loading

Codecov Report

jakeleventhal commented Sep 18, 2021

jakeleventhal commented Sep 18, 2021

jakeleventhal commented Sep 25, 2021

ljharb commented Sep 26, 2021

jakeleventhal commented Sep 29, 2021

[New] add `no-unused-class-component-methods` #2239

[New] add `no-unused-class-component-methods` #2239

pawelnvk commented Apr 13, 2019 •

edited by ljharb

Loading

meowtec commented Oct 22, 2019 •

edited

Loading

codecov-commenter commented Sep 16, 2021 •

edited

Loading