Allow sparse arrays in eslint-config-eyeo

[mjethani]

Background

Code like this should be common in modern JavaScript:

let [, pattern, flags] = regExp.exec(text) || [, textToRegExp(text)];

However eslint-config-eyeo does not allow sparse arrays. It gives the following error:

error  Unexpected comma in middle of array  no-sparse-arrays

See code review ​29613805 (comment #30) for context.

What to change

Modify our ESLint config to allow sparse arrays, at least when using this kind of pattern.

[kzar]

Thanks for opening the issue. For changes to our coding style rules / ESLint rules it's important to get everyone onboard so I've copied some more people in.

What do you all think to this suggestion?

[kzar]

Oh another thing with these types of suggestions it's important to check browser support. Did you have a look to see if Chrome 49, Firefox 51, Edge etc support sparse arrays?

[greiner]

Generally, I don't see any issues with relaxing our linting rules as long as there aren't any rules in our coding styles that say otherwise.

Regarding browser support, I think it's important but it may need to be ensured through other means since every project has different requirements (e.g. #5462).

[mjethani]

Replying to kzar:

Oh another thing with these types of suggestions it's important to check browser support. Did you have a look to see if Chrome 49, Firefox 51, Edge etc support sparse arrays?

Sparse arrays per se are old, and ​destructuring assigments are supported on all of the above platforms.

[agiammarchi]

FWIW I've double checked both Chrome 49 and Edge 15. This is also not really about sparse arrays, rather about destructuring, as @mjethani mentioned, but it's actually only in Chrome 49 that it works, and in Edge 15 without a flag.

I think there's no risk whatsoever in relaxing the linter here, although dropping that rule will not affect destructuring only, it will actually affect sparse arrays. I guess without this rule code reviews and common sense would be more suitable than a linter.

:thumb-up:

[kzar]

Replying to agiammarchi:

I guess without this rule code reviews and common sense would be more suitable than a linter.

While we sometimes do have to rely on common sense, where possible it's good to specify stuff in the ESLint configuration and our ​coding style rules. Not long before Manish and Hubert joined we wasted quite a lot of time discussing these things over and over in codereviews - it's better to get that discussion out of the way once.

So what should our coding style rule be here, that we should use Array destructuring where possible? Should we incorporate other types of destructuring into that rule?

...but it's actually only in Chrome 49 that it works, and in Edge 15 without a flag.

I don't quite understand, do you mean that Array destructuring is not supported in Firefox 51?

This is also not really about sparse arrays, rather about destructuring, as @mjethani mentioned...

Fair enough, but in that case could you guys update the issue description to explain that?

[mjethani]

Replying to kzar:

Replying to agiammarchi:

This is also not really about sparse arrays, rather about destructuring, as @mjethani mentioned...

Fair enough, but in that case could you guys update the issue description to explain that?

The problem with the config though is precisely that sparse array literals are not allowed.

i.e. in the following statement:

let [, pattern, flags] = regExp.exec(text) || [, textToRegExp(text)];

The error is not on the left hand side but rather on the right hand side. Our ESLint config is OK with destructing assignments, it just doesn't like sparse arrays.

[agiammarchi]

Our ESLint config is OK with destructing assignments, it just doesn't like sparse arrays.

so, if I understand correctly, ESLint is OK with sparse destructuring [, a, b] = but id doesn't like the right part ?

Because in that case I'd personally would write that as such:

let [, pattern, flags] = regExp.exec(text) || [null, textToRegExp(text)];

so we can keep the warning on sparse arrays for all other valid cases.

do you mean that Array destructuring is not supported in Firefox 51?

no, I meant that it was worth checking destructuring because Chrome 49 apparently was the first version shipping it without any flag and same goes for Edge 15. Firefox 51 is more than fine with arrays (but for rest objects is not, it starts from 55)

[kzar]

no, I meant that it was worth checking destructuring because Chrome 49 apparently was the first version shipping it without any flag and same goes for Edge 15.

Gotya OK, well I just tested this on Chrome 49 and it worked:

(function ()
{
  let [, b, c] = [1, 2, 3];
  console.log(b, c);
}());

=> 2 3

anything else I should check do you think?

[mjethani]

Replying to agiammarchi:

[snip]

Because in that case I'd personally would write that as such:

let [, pattern, flags] = regExp.exec(text) || [null, textToRegExp(text)];

Yes, that is exactly what we're doing for now, but the question is why the restriction on sparse arrays exists in the first place. I'm using sparse arrays ​all over the place in Chromium for example.

do you mean that Array destructuring is not supported in Firefox 51?

no, I meant that it was worth checking destructuring because Chrome 49 apparently was the first version shipping it without any flag and same goes for Edge 15. Firefox 51 is more than fine with arrays (but for rest objects is not, it starts from 55)

Rest in object is not standard so we shouldn't use it.

[agiammarchi]

the question is why the restriction on sparse arrays exists in the first place

I guess I'm not the right person to answer that, sorry: quoting @kzar

we wasted quite a lot of time discussing these things over and over in codereviews

but if it's a very common pattern these days, and FWIW, I'm +1 for sparse arrays.

Rest in object is not standard so we shouldn't use it.

It's in latest specification draft (it's a finished stage 4 proposal so it'll be in next ES release) but good point, specially if we don't transpile.

[sebastian]

Quoting from the ​documentation of the no-sparse-array rule:

var colors = [ "red",, "blue" ];

In this example, the colors array has a length of 3. But did the developer intend for there to be an empty spot in the middle of the array? Or is it a typo?

I can see the issue there.

[mjethani]

Replying to sebastian:

Quoting from the ​documentation of the no-sparse-array rule:

var colors = [ "red",, "blue" ];

In this example, the colors array has a length of 3. But did the developer intend for there to be an empty spot in the middle of the array? Or is it a typo?

I can see the issue there.

A lot of problems that ESLint tries to solve are for personal projects. In this example of [ "red",, "blue" ], if it were in Adblock Plus, it would be caught in code review either by the author or the reviewer. It is highly unlikely (still possible, though I have never seen an instance) that such code would make it into the repository by mistake.

On the other hand, it is easy enough to just make it [ "red", null, "blue" ] to please ESLint.

Since I filed this ticket, I have not run into any more examples of no-sparse-array causing any inconvenience. Therefore I would vote for closing this ticket now, if no one has any objections.

[sebastian]

Sure, a second pair of eyes during code review make those mistakes less likely, but a rule that can be enforced automatically is still more reliable, and also makes the job of the reviewer easier. Anyway, since you (who reported this issue) is no longer bothering about this limitation, I'm closing this issue now.