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

[New] Add no-arrow-function-lifecycle rule #1980

Merged
merged 1 commit into from Oct 1, 2021
Merged

Conversation

ngtan
Copy link
Contributor

@ngtan ngtan commented Sep 13, 2018

Overview

I would like to add one more rule no-arrow-function-lifecycle which checks that it is not necessary to use arrow function for lifecycle methods and it will affect a bit performance.

Movitation

I did code review some projects and found that our teammates use arrow function for lifecycle methods constantly. Beside that, I also found a ticket related to this issue. See react/issues/10810 for more details.

Consider the below code

class Hello extends React.Component {
  render = () => {
    return <div />;
  }
}

Do you need to use arrow function here? I believe that, you will have the same idea that it shouldn't.

I'm eagerly looking forward to seeing your comments.

lib/rules/no-arrow-function-lifecycle.js Outdated Show resolved Hide resolved
lib/rules/no-arrow-function-lifecycle.js Outdated Show resolved Hide resolved
docs/rules/no-arrow-function-lifecycle.md Outdated Show resolved Hide resolved
@ngtan
Copy link
Contributor Author

ngtan commented Sep 13, 2018

@ljharb Thanks for your reviews. I think I have addressed your comments already. Could you check it out again? If you have any concerns, just leave your comment.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This rule seems like it should be autofixable. Can it be?

lib/rules/no-arrow-function-lifecycle.js Outdated Show resolved Hide resolved
lib/rules/no-arrow-function-lifecycle.js Outdated Show resolved Hide resolved
@ngtan
Copy link
Contributor Author

ngtan commented Sep 14, 2018

@ljharb I really keen on your idea but, in my point of view it should be fixed by creator to make sure that they won't repeat it in next time.

Beside that, I also figure out some common cases:

class Hello extends React.Component {
  render = () => {
    return (<div />);
  }
}
class Hello extends React.Component {
  render = () => (<div />);
}
class Hello extends React.Component {
  componentWillReceiveProps = nextProps => {}
  shouldComponentUpdate = (nextProps = { blabla }, nextState) => (returned)
  render() {
    return (<div />);
  }
}

To be honest, I didn't find out a good way to replace them with instance method yet. Anyway, I believe I will have a one to help it automatically fixed later after it was merged.

@ljharb
Copy link
Member

ljharb commented Sep 14, 2018

$methodName($argList) { return $methodBody; }?

@ngtan
Copy link
Contributor Author

ngtan commented Sep 14, 2018

$methodName($argList) { return $methodBody; }?

I also thought about it. But, have idea for now.

@alexzherdev
Copy link
Contributor

The autofix would be so easy if we had AST-driven fixers 🙁 (although I'm guessing there's valid reasons we don't?)

@ngtan
Copy link
Contributor Author

ngtan commented Sep 17, 2018

The autofix would be so easy if we had AST-driven fixers 🙁 (although I'm guessing there's valid reasons we don't?)

Honestly, I'm working on this.

@golopot
Copy link
Contributor

golopot commented May 5, 2019

I think no-class-fields-lifecycle is a better name. The rule should also report on non-arrow-function like: render = function() {…}.

@dylmye
Copy link

dylmye commented Oct 7, 2019

Any update on this? Thanks

@niki-timofe
Copy link

@wheeler
Copy link

wheeler commented Nov 4, 2019

I'm going to add to the motivation for this rule. In my experience the render = () => { ... } pattern breaks react-hot-loader (it will render version n-1 of the component where n is the latest). I would LOVE to have this rule (because I was about to try writing it myself before finding this).

@ngtan // @ljharb This has been idle for a while - but I don't see any outstanding change requests. I'm interested in helping move this forward. Do we need to write a fixer or could that come in a subsequent patch?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This shouldn't be just confined to arrow functions; a non-arrow function in a class field should error too.

In other words, let's rename this rule to lifecycle-methods, and have it enforce that anything with a lifecycle method name is an instance method.

@ljharb
Copy link
Member

ljharb commented Aug 9, 2021

@ngtan any interest in finishing this PR?

@ljharb ljharb marked this pull request as draft August 9, 2021 20:59
@ngtan
Copy link
Contributor Author

ngtan commented Aug 11, 2021

Hi, It has been a long time. I will make time this week to finish the PR.

@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2021

Codecov Report

Merging #1980 (425860f) into master (241c38b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1980   +/-   ##
=======================================
  Coverage   97.40%   97.41%           
=======================================
  Files         111      113    +2     
  Lines        7444     7463   +19     
  Branches     2723     2725    +2     
=======================================
+ Hits         7251     7270   +19     
  Misses        193      193           
Impacted Files Coverage Δ
index.js 100.00% <ø> (ø)
lib/rules/no-arrow-function-lifecycle.js 100.00% <100.00%> (ø)
lib/util/lifecycleMethods.js 100.00% <100.00%> (ø)

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 241c38b...425860f. Read the comment docs.

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/rules/no-typos.js Outdated Show resolved Hide resolved
@ngtan ngtan marked this pull request as ready for review September 19, 2021 12:45
@ngtan ngtan requested a review from ljharb September 19, 2021 12:48
@ljharb ljharb changed the title [NEW]: Add no-arrow-function-lifecycle rule [New] Add no-arrow-function-lifecycle rule Sep 19, 2021
lib/util/lifecycleMethods.js Outdated Show resolved Hide resolved
@ljharb ljharb force-pushed the master branch 2 times, most recently from 2668b77 to a9f0b95 Compare October 1, 2021 04:00
@ljharb ljharb merged commit a9f0b95 into jsx-eslint:master Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

9 participants