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

array-callback-return should check forEach #12551

Closed
SheetJSDev opened this issue Nov 10, 2019 · 8 comments
Closed

array-callback-return should check forEach #12551

SheetJSDev opened this issue Nov 10, 2019 · 8 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

@SheetJSDev
Copy link

What rule do you want to change?
array-callback-return

Does this change cause the rule to produce more or fewer warnings?
more

How will the change be implemented? (New option, new default behavior, etc.)?
new default behavior

Please provide some example code that this change will affect:

[
    "abc",
    "def"
].forEach((word) => {

    "use strict";

    return word;

});

What does the rule currently do for this code?
Pass

What will the rule do after it's changed?
Warn that the forEach callback potentially returns a value

Are you willing to submit a pull request to implement this change?
Yes

@SheetJSDev SheetJSDev added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Nov 10, 2019
@g-plane
Copy link
Member

g-plane commented Nov 10, 2019

Thanks for this issue.

Can you provide more information or context to tell us the intention of this change?

@g-plane g-plane added 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 Nov 10, 2019
@SheetJSDev
Copy link
Author

From the documentation for the rule

Array has several methods for filtering, mapping, and folding. If we forget to write return statement in a callback of those, it’s probably a mistake. If you don’t want to use a return or don’t need the returned results, consider using .forEach instead.

It is probably a mistake for callbacks of the forEach method to return values (since forEach does not use the result). If you want to use a return or need the returned result, consider using .map instead.

forEach / map confusion is not uncommon. The rule checks one direction (map used when forEach was more appropriate) but not the other direction (forEach used when map is more appropriate)

@g-plane
Copy link
Member

g-plane commented Nov 10, 2019

However, this rule warns if you forget to return something when using those methods which accept callback, while you want to propose about forEach. It shouldn't be a behavior of this rule.

@SheetJSDev
Copy link
Author

There are two questions you can ask:

  1. should ESLint warn if a forEach callback returns a value?

since array-callback-return currently checks callbacks for other array functions like map, it would be consistent for ESLint to check the callbacks for forEach. Array#forEach is a commonly used method.

  1. should the forEach check be part of array-callback-return or a new rule?

The name array-callback-return and the quoted language from the documentation suggests the general intent of the rule is to detect common mistakes in return values from callbacks of Array methods. As forEach is an Array method, it makes the most sense to change the rule rather than introducing a new rule.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Nov 10, 2019

The name of the rule suggests it should handle both cases - it’s just only doing half its job right now. An option to check the inverse case on forEach seems ideal.

@kaicataldo
Copy link
Member

I’ll champion this. While this would be a change of scope, I agree that this is in the spirit of the rule and is useful. I don’t think we should create another rule for this.

That being said, I think this has to go behind an option that is turned off by default and that we should make it the default behavior in a future major release.

@kaicataldo kaicataldo self-assigned this Nov 10, 2019
@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 Nov 10, 2019
@platinumazure
Copy link
Member

This is now accepted.

gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Dec 5, 2019
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Dec 5, 2019
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Dec 5, 2019
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Dec 5, 2019
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Dec 5, 2019
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Dec 5, 2019
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
@mdjermanovic
Copy link
Member

PR #12646

gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Dec 9, 2019
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Dec 20, 2019
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Jan 1, 2020
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Jan 1, 2020
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Jan 2, 2020
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Jan 2, 2020
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Jan 2, 2020
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Jan 2, 2020
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Jan 15, 2020
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Jan 28, 2020
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Jan 28, 2020
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs added a commit to gabrieldrs/eslint that referenced this issue Feb 2, 2020
array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
montmanu pushed a commit to montmanu/eslint that referenced this issue Mar 4, 2020
… (eslint#12646)

array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error.
This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true.

Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Aug 4, 2020
@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 Aug 4, 2020
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