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

A case for prefer-flat-map #311

Closed
tinovyatkin opened this issue Jun 2, 2019 · 10 comments · Fixed by #323
Closed

A case for prefer-flat-map #311

tinovyatkin opened this issue Jun 2, 2019 · 10 comments · Fixed by #323
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted

Comments

@tinovyatkin
Copy link

tinovyatkin commented Jun 2, 2019

Issuehunt badges

I think it will be way greater rule if it will be able to identify legacy (before .flat got supported) code and suggest to refactor to flatMap. I believe one of the most common scenario was use of [].concat with array spread:

[].concat(...someArray.map(fn))
// is the same
someArray.flatMap(fn)

IssueHunt Summary

[
<
i
m
g

s
r
c

'
h
t
t
p
s
:
/
/
a
v
a
t
a
r
s
1
.
g
i
t
h
u
b
u
s
e
r
c
o
n
t
e
n
t
.
c
o
m
/
u
/
7
4
2
6
0
?
v

4
'

a
l
t

'
m
r
h
e
n
'

w
i
d
t
h

2
4

h
e
i
g
h
t

2
4

m
r
h
e
n
]
(
h
t
t
p
s
:
/
/
i
s
s
u
e
h
u
n
t
.
i
o
/
u
/
m
r
h
e
n
)

h
a
s

b
e
e
n

r
e
w
a
r
d
e
d
.

Backers (Total: $40.00)

Submitted pull Requests


Tips


IssueHunt has been backed by the following sponsors. Become a sponsor

@sindresorhus
Copy link
Owner

Good idea!

@fisker
Copy link
Collaborator

fisker commented Jun 4, 2019

should we consider Symbol.isConcatSpreadable?

@sindresorhus
Copy link
Owner

@fisker Consider it how? Can you elaborate?

@fisker
Copy link
Collaborator

fisker commented Jun 4, 2019

@sindresorhus

this demo seems not working on my chrome, but works on firefox

check this https://jsbin.com/farogop/edit?js,console

though this maybe not very common case,
I'm just saying, It's possible not the same as flatMap, and we can't guarantee new Objects not ConcatSpreadable never come in future

@MrHen
Copy link
Contributor

MrHen commented Jun 4, 2019

@fisker I'm comfortable using // eslint-disable-next-line prefer-flat-map to ignore cases like that.

@sindresorhus
Copy link
Owner

sindresorhus commented Jun 4, 2019

@fisker That's too much of an obscure use-case to handle.

@fisker
Copy link
Collaborator

fisker commented Jun 5, 2019

Imaging this fn is from a third-party library, which is isConcatSpreadable false

[].concat(...someArray.map(fn))

When you wrote and tested, It works as expected, you may not think about adding // eslint-disable-next-line prefer-flat-map.
When you format your code and publish, It breaks. You have to debug your program to locate the problem.

If a linter can't be safe, what's the point of using it.

I insist not to fix this code, at least an option, people should know this may cause problems

@sindresorhus
Copy link
Owner

@fisker This seems very unlikely, but sure, we can omit fixing this case.

If a linter can't be safe, what's the point of using it.

Then you should probably not use JavaScript. Most lint rules are not 100% safe. That is just impossible for such a loose language as JS.

@issuehunt-oss issuehunt-oss bot added the 💵 Funded on Issuehunt This issue has been funded on Issuehunt label Jun 11, 2019
@IssueHuntBot
Copy link

@issuehunt has funded $40.00 to this issue.


@issuehunt-oss
Copy link

issuehunt-oss bot commented Jun 24, 2019

@sindresorhus has rewarded $36.00 to @MrHen. See it on IssueHunt

  • 💰 Total deposit: $40.00
  • 🎉 Repository reward(0%): $0.00
  • 🔧 Service fee(10%): $4.00

@issuehunt-oss issuehunt-oss bot added 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt and removed 💵 Funded on Issuehunt This issue has been funded on Issuehunt labels Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🎁 Rewarded on Issuehunt This issue has been rewarded on Issuehunt help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants