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

Rule proposal: No this argument in array methods when using arrow functions #974

Closed
sindresorhus opened this issue Dec 31, 2020 · 16 comments · Fixed by #1357
Closed

Rule proposal: No this argument in array methods when using arrow functions #974

sindresorhus opened this issue Dec 31, 2020 · 16 comments · Fixed by #1357

Comments

@sindresorhus
Copy link
Owner

Prevent specifying the this argument to array methods when using an arrow function, since it has no effect.

Applies to the following instance methods:

map
filter
some
find
forEach
findIndex
every

And also Array.from.

Only Array.from can be safely auto-fixed. The others should use suggestions.

Fail

array.filter(x => x === 1, this);

Pass

array.filter(x => x === 1);
@fisker
Copy link
Collaborator

fisker commented Dec 31, 2020

I've never seen anyone use this.

@sindresorhus
Copy link
Owner Author

I have seen it in the wild many times before, but I don't remember any specific place now. It's most likely to happen when you migrate from a function to arrow function.

@fisker
Copy link
Collaborator

fisker commented Dec 31, 2020

What do you think forbid use of thisArgument.

I don't think it's useful.

@sindresorhus
Copy link
Owner Author

What do you think forbid use of thisArgument.

Do you mean to forbid thisArgument in general? If so, what other places can it be found?

@fisker
Copy link
Collaborator

fisker commented Dec 31, 2020

Forbid

2nd argument in

map
filter
some
find
forEach
findIndex
every

call,

It's never useful,

why would someone use

foo.map(function() {}, bar)

instead of

foo.map(function() {}.bind(bar))

or

foo.map(() => use(this))

@sindresorhus
Copy link
Owner Author

I agree.

@fisker
Copy link
Collaborator

fisker commented Jan 22, 2021

no-this-argument-for-array-callback?

@sindresorhus
Copy link
Owner Author

How about no-array-method-this-argument?

@fisker
Copy link
Collaborator

fisker commented Jan 28, 2021

A more generic solution, enfore array method arguments length, takes a number or range.

{
	slice: [1],      // At least one argument, forbid `.slice()` to copy array
	reduce: 2,       // Enfore inital value for `.reduce()`
	map: 1,          // For the example in the topic, forbid use this argument
	concat: [0, 5],  // Allow concat at most 5 arrays (just an example)
}

This rule may not only work for array, we can also make it work for any method.

For example:

// eslint-xxx {"is": 2, "parserInt": 2}

Object.is(foo) // Fail, missing the second value to compare
parseInt(foo)  // Fail, missing the radix

@sindresorhus
Copy link
Owner Author

A more generic solution, enfore array method arguments length, takes a number or range.

I don't really see the use-case for this. I wouldn't use any of those.

parseInt(foo) // Fail, missing the radix

There's already a built-in rule for this.

@fisker
Copy link
Collaborator

fisker commented Jan 31, 2021

I don't really see the use-case for this.

Have you ever use string.split("|", 5)? The second argument is unknown to many people, but I've seen people use, if I have this rule, I'll forbid to use it.

Another case, I don't like array.join(), I prefer array.join(',').

@sindresorhus
Copy link
Owner Author

Ok. Yeah, I can see how that could be useful. I'm just concerned that the general solution will have many more possible false-positives. .map is a general method name and could come in many forms. It's a lot less likely array.map(x => foo(x), this); will be a false-positive than notArray.map(x => foo(x), bar);, because most non-array .map type methods would not accept this.

@sindresorhus
Copy link
Owner Author

Your proposal would have been ideal if we had type information, but as I commented earlier, I fear a generic solution would have a lot of false-positives. The rule proposed here is much more focused. I think they both could exist.

@sindresorhus
Copy link
Owner Author

@fisker Can you open a separate proposal for your idea about a generic arguments rule?

@sindresorhus
Copy link
Owner Author

This is now accepted.

@pcorpet
Copy link

pcorpet commented Jun 29, 2021

This triggers some false positive with React.Children, what is your recommendation for this? Should I try to fix the code enforcing the rule? Should I disable the rule each time I use React.Children?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants