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

no-useless-spread should report when passing spreading parameters to new Set #2412

Open
zanminkian opened this issue Jul 27, 2024 · 7 comments

Comments

@zanminkian
Copy link
Contributor

zanminkian commented Jul 27, 2024

1️⃣ Explain here what's wrong

function getNames() {
  return ['foo', 'bar', 'foo'];
}

const uniqueNames = [...new Set(...getNames())]; // I mistakenly spreading the `getNames()`
console.log(uniqueNames) // Print [ 'f', 'o' ]. But what I expect is ['foo', 'bar']

no-useless-spread should report error on the code new Set(...getNames()). But actually not.

Because new Set() accept one or zero parameter, we don't need to spread the parameters.

Example

const foo = ['abc'];
const set = new Set(...foo); // BAD!
const set = new Set(foo[0]); // OK

2️⃣ Specify which rule is buggy here and in the title
no-useless-spread

3️⃣ More information
TypeScript should report error when compiling the code above. Unfortunately, this problem havn't been fixed yet. Set microsoft/TypeScript#59390 and microsoft/TypeScript#48575

@fisker

This comment was marked as outdated.

@fisker
Copy link
Collaborator

fisker commented Jul 27, 2024

Because new Set() accept one or zero parameter, we don't need to spread the parameters.

This doesn't mean it can't spread the parameters

const foo = [[1,2]]

new Set(...foo)

@zanminkian
Copy link
Contributor Author

zanminkian commented Jul 27, 2024

This doesn't mean it can't spread the parameters

const foo = [[1,2]]

new Set(...foo)

It can, but it's unnecessary.

const foo = [[1,2]];
new Set(foo[0]);

Only the first item will be used in new Set(...foo). So Just use new Set(foo[0]).

Passing spreading parameters to new Set is a bad code style.

  • It's confused.
  • Not type safe enough (due to TypeScript bug).

@fregante
Copy link
Collaborator

fregante commented Jul 31, 2024

There's a slight difference I think. no-useless-spread suggests that you "don't need it", while here it leads to a different result altogether, probably not what you wanted in the first place.

However I think it should still report it because the spread only works correctly when foo.length <= 1.

const foo = []; // Empty
new Set(...foo) == new Set() == new Set(undefined); // Ok, works just as well
const foo = [[1]]; // One
new Set(...foo) == new Set(foo[0]); // Ok, works just as well
const foo = [[1], [2]]; // Two
new Set(...foo) == new Set(foo[0], foo[1]); // ❌ Item 2 lost

The only caution would be to not autofix it in this case.

@zanminkian
Copy link
Contributor Author

const foo = [[1], [2]]; // Two
new Set(...foo) == new Set(foo[0], foo[1]); // ❌ Item 2 lost

The only caution would be to not autofix it in this case.

const foo = [[1], [2]]; // Two
new Set(...foo) == new Set(foo[0], foo[1]) == new Set(foo[0]); // Ok, works just as well

Auto fixing is safe. Just confidently change all new Set(...foo) to new Set(foo[0]).

@fregante
Copy link
Collaborator

That only works if you know the type of foo. If it’s an iterator other than Array that breaks it.

@zanminkian
Copy link
Contributor Author

zanminkian commented Aug 1, 2024

My fault.

const map = new Map([['a','b']]);
new Set(...map); // Set(2) { 'a', 'b' }
new Set(map[0]); // Set(0) {}

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