Opened 18 months ago

Closed 4 months ago

Last modified 4 months ago

#6411 closed change (rejected)

Allow sparse arrays in eslint-config-eyeo

Reported by: mjethani Assignee:
Priority: Unknown Milestone:
Module: Automation Keywords:
Cc: hfiguiere, kzar, fhd, greiner, sebastian, agiammarchi Blocked By:
Blocking: #6425 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by 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.

Change History (20)

comment:1 Changed 18 months ago by mjethani

  • Description modified (diff)

comment:2 Changed 18 months ago by kzar

  • Cc fhd greiner sebastian added

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?

comment:3 follow-up: Changed 18 months ago by 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?

comment:4 Changed 18 months ago by 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).

comment:5 Changed 18 months ago by mjethani

  • Cc agiammarchi added

comment:6 in reply to: ↑ 3 Changed 18 months ago by 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.

comment:7 follow-up: Changed 18 months ago by 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:

Last edited 18 months ago by agiammarchi (previous) (diff)

comment:8 in reply to: ↑ 7 ; follow-up: Changed 18 months ago by 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?

comment:9 in reply to: ↑ 8 Changed 18 months ago by 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.

comment:10 follow-up: Changed 18 months ago by 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)

Last edited 18 months ago by agiammarchi (previous) (diff)

comment:11 Changed 18 months ago by 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?

comment:12 in reply to: ↑ 10 Changed 18 months ago by 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.

comment:13 Changed 18 months ago by 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.

comment:14 Changed 18 months ago by kzar

  • Blocked By 6425 added

comment:15 Changed 18 months ago by kzar

  • Blocked By 6425 removed

comment:16 Changed 18 months ago by kzar

  • Blocking 6425 added

comment:17 Changed 4 months ago by hfiguiere

  • Component changed from Sitescripts to Automation

comment:18 follow-up: Changed 4 months ago by 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.

comment:19 in reply to: ↑ 18 Changed 4 months ago by 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.

Last edited 4 months ago by mjethani (previous) (diff)

comment:20 Changed 4 months ago by sebastian

  • Resolution set to rejected
  • Status changed from new to closed

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.

Last edited 4 months ago by sebastian (previous) (diff)
Note: See TracTickets for help on using tickets.