Opened 12 months ago

Last modified 12 months ago

#6411 new change

Allow sparse arrays in eslint-config-eyeo

Reported by: mjethani Assignee:
Priority: Unknown Milestone:
Module: Sitescripts 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 (16)

comment:1 Changed 12 months ago by mjethani

  • Description modified (diff)

comment:2 Changed 12 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 12 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 12 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 12 months ago by mjethani

  • Cc agiammarchi added

comment:6 in reply to: ↑ 3 Changed 12 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 12 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 12 months ago by agiammarchi (previous) (diff)

comment:8 in reply to: ↑ 7 ; follow-up: Changed 12 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 12 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 12 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 12 months ago by agiammarchi (previous) (diff)

comment:11 Changed 12 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 12 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 12 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 12 months ago by kzar

  • Blocked By 6425 added

comment:15 Changed 12 months ago by kzar

  • Blocked By 6425 removed

comment:16 Changed 12 months ago by kzar

  • Blocking 6425 added
Note: See TracTickets for help on using tickets.