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

Add a --fix option for prefer-arrow-callback #7002

Closed
not-an-aardvark opened this issue Aug 28, 2016 · 7 comments
Closed

Add a --fix option for prefer-arrow-callback #7002

not-an-aardvark opened this issue Aug 28, 2016 · 7 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Aug 28, 2016

I'm using Node v6.4.0 and ESLint v3.4.0.

It would be useful to have an autofix option for the prefer-arrow-callback rule. The fixer would attempt to convert regular callback functions to arrow functions.

  • If the function does not use this, super, arguments, or new.target, and it is not explicitly bound, it can be replaced naively:
/* eslint prefer-arrow-callback: ["error"] */

[1, 2, 3].map(function(a, b, c) {
  return a * 2;
});

// gets fixed to:

[1, 2, 3].map((a, b, c) => {
  return a * 2;
});
  • If the function is explicitly bound with .bind(this), and it does not use super, arguments, or new.target, the binding can be removed and the function can be replaced:
/* eslint prefer-arrow-callback: ["error"] */

[1, 2, 3].map(function(a, b, c) {
  return a * 2;
}.bind(this));

// gets fixed to:

[1, 2, 3].map((a, b, c) => {
  return a * 2;
});

// ---

[1, 2, 3].map(function(a, b, c) {
  return a * this.foo;
}.bind(this));

// gets fixed to:

[1, 2, 3].map((a, b, c) => {
  return a * this.foo;
});
  • If the option {allowUnboundThis: false} is used, and the function includes this without being explicitly bound, the problem should not get fixed, since there is no way to be sure of the correct this value:
/* eslint prefer-arrow-callback: ["error", {allowUnboundThis: false}] */
[1, 2, 3].map(function(a, b, c) {
  return a * this.foo;
});

// (Reports an error, but is not autofixed)
  • If the function uses super, arguments, or new.target, the rule does not report any problems, so nothing gets fixed.
/* eslint prefer-arrow-callback: ["error"] */

[1, 2, 3].map(function(a, b, c) {
  return arguments[0] * 5;
});

// (no errors are reported, so no fixes are performed)
  • To avoid SyntaxErrors, the fixer should not fix anything unless ES6 parsing is enabled.

Other things to consider:

  • If the {allowNamedFunctions: false} option is used and a function has a name, should the function still be changed when autofixing? (This doesn't break recursive functions, since those are ignored by prefer-arrow-callback anyway.) I think the fixer should still fix these functions; if people want to allow named functions, then they should set {allowNamedFunctions: true}.

I would be willing to add this if the issue is accepted.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Aug 28, 2016
@platinumazure platinumazure added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Aug 28, 2016
@platinumazure
Copy link
Member

platinumazure commented Aug 28, 2016

I can see how this would be useful, but this would be a massive fix (in terms of characters of source code affected per fix). However, the fix plan seems pretty sensible. I'll endorse (:+1:).

EDIT: Per comment below, this only applies when dealing with a call to .bind.

@not-an-aardvark
Copy link
Member Author

but this would be a massive fix (in terms of characters of source code affected per fix)

Note that this is only a massive fix if .bind(this) is used; otherwise, the fix would just replace function (a, b, c) with (a, b, c) => and leave the rest of the function untouched.

@mysticatea
Copy link
Member

👍

1 similar comment
@ilyavolodin
Copy link
Member

👍

@mysticatea
Copy link
Member

I will champion this.
We needs one more 👍 from the team.

@mysticatea mysticatea self-assigned this Aug 29, 2016
@platinumazure platinumazure added accepted There is consensus among the team that this change meets the criteria for inclusion enhancement This change enhances an existing feature of ESLint and removed enhancement This change enhances an existing feature of ESLint accepted There is consensus among the team that this change meets the criteria for inclusion labels Aug 29, 2016
@platinumazure
Copy link
Member

Not fair @mysticatea, you endorsed and championed and I double-counted you. 😜 One more 👍 from the team as @mysticatea said.

@vitorbal
Copy link
Member

I'll 👍 this!

@platinumazure platinumazure added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Aug 29, 2016
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

6 participants