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

prefer-array-flat awkward autofix on .concat().concat() #2470

Open
geoffswift opened this issue Oct 5, 2024 · 5 comments
Open

prefer-array-flat awkward autofix on .concat().concat() #2470

geoffswift opened this issue Oct 5, 2024 · 5 comments

Comments

@geoffswift
Copy link

Some people seem to like building up an Array, by starting with an empty list and calling concat() with extra items.

A trivial minimal reproduction would look like this:

const a = [].concat([1]).concat([2]);

When you apply the autofix, it comes out like this, which is clearly nonsense.

const a = [[1]].flat().concat([2])

It seems the intent of the rule is to stop me using some arcane technique to flatten arrays, but the developers intent here is just to build up a flat list of entries rather than to flatten an Array

@geoffswift geoffswift added the bug label Oct 5, 2024
@github-actions github-actions bot changed the title prefer-array-flat overzealous with concat case prefer-array-flat overzealous with concat case Oct 5, 2024
@fregante fregante changed the title prefer-array-flat overzealous with concat case prefer-array-flat awkward autofix on .concat().concat() Oct 5, 2024
@fregante
Copy link
Collaborator

fregante commented Oct 5, 2024

I wouldn't classify this as a bug. Garbage in garbage out. The code should be rewritten as [...[1], ...[2]], assuming that those two arrays are actually not literals, so [...arr1, ...arr2]

@geoffswift
Copy link
Author

Can we make the autofix rewrite the code like that? As it stands the autofix seems to make the code worse.

@fregante
Copy link
Collaborator

fregante commented Oct 6, 2024

I think that's already the case thanks to https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-spread.md

I don’t think [].concat() should be part of prefer-array-flat and thus should be removed.

The only reason to have it is because .concat accepts both arrays and non-arrays: array.concat(maybeArray) can't be autofixed to [...array, ...maybeArray]. But that's not a reason to use [maybeArray].flat(), which just looks weird in any case.

@fisker
Copy link
Collaborator

fisker commented Oct 8, 2024

I don’t think [].concat() should be part of prefer-array-flat and thus should be removed.

It definitely should be in prefer-array-flat. It's common (even recommended) to use [].concat() to flat array. https://stackoverflow.com/questions/10865025/merge-flatten-an-array-of-arrays

#975

@fregante
Copy link
Collaborator

fregante commented Oct 9, 2024

It's common (even recommended) to use [].concat() to flat array.

Right, but that doesn't mean that it's the best way. The example on that SO page is actually already caught by prefer-spread and it's autofixed:

- [].concat(["$6"], ["$12"], ["$25"], ["$25"], ["$18"], ["$22"], ["$10"]);
+ ["$6", "$12", "$25", "$25", "$18", "$22", "$10"];

The first example uses [].concat.apply([], arrays) which is correctly auto-fixed to arrays.flat()

The problem here is specifically with the plain usage as shown by OP, with again both rules applying to it, and this is what XO currently spits out with --fix:

- const a = [].concat([1]).concat([2])
+ const a = [...[[1]].flat(), 2]

Here's the output after prefer-array-flat is disabled:

- const a = [].concat([1]).concat([2])
+ const a = [1, 2]

It definitely should be in prefer-array-flat

For .concat.apply definitely, for [].concat(x).concat(y) definitely not.

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

No branches or pull requests

3 participants