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

[Feat] - Detect unused class component method #2239

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@pawelnvk
Copy link
Contributor

commented Apr 13, 2019

Display errors in case class component contains unused methods.

Fixes #2166.

@pawelnvk pawelnvk force-pushed the pawelnvk:unused-component-methods branch from a537eeb to 1aaa6c9 Apr 13, 2019

@ljharb
Copy link
Collaborator

left a comment

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

// Rule Definition
// ------------------------------------------------------------------------------

const internalMethods = [

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 16, 2019

Collaborator

what about unused static methods?

This comment has been minimized.

Copy link
@pawelnvk

pawelnvk May 7, 2019

Author Contributor

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..

This comment has been minimized.

Copy link
@ljharb

ljharb May 7, 2019

Collaborator

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?

This comment has been minimized.

Copy link
@pawelnvk

pawelnvk May 8, 2019

Author Contributor

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

Show resolved Hide resolved lib/rules/no-unused-class-component-methods.js
const isMethod = node.type === 'MethodDefinition';
const isArrowFunction = (
node.type === 'ClassProperty' &&
node.value.type === 'ArrowFunctionExpression'

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 16, 2019

Collaborator

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

@ljharb ljharb added the new rule label Apr 16, 2019

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

This comment has been minimized.

Copy link
@golopot

golopot Apr 19, 2019

Contributor

What about use by object destructuring?

return;
}

methods = methods.filter(method =>

This comment has been minimized.

Copy link
@golopot

golopot Apr 19, 2019

Contributor

this.foo = bar will make foo falsely considered used.

@pawelnvk

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

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

This comment has been minimized.

Copy link

commented May 3, 2019

Would this also account for functional components?

@golopot

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

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

@ha404

This comment has been minimized.

Copy link

commented May 3, 2019

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

const SomeComponent = () => ...

SomeComponent.propTypes = ...
SomeComponent.anythingYouWant = ...
filterAllMethods(property) &&
internalMethods.indexOf(astUtil.getPropertyName(property)) === -1
));
const getThisExpressions = subnode => {

This comment has been minimized.

Copy link
@ljharb

ljharb May 11, 2019

Collaborator

maybe this function would be useful moved out to a util, and unit-tested?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.