1616608 - comm/eslintrc.js causes broken eslint behavior, e.g. --fix removes }

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Kai Engert (:KaiE:)]

Attachment: test.js

I'll attach sample JS code, which contains an expression
try { if () { action }} catch () {}

eslint --fix has a bug, and changes it to
try { if () { action } catch () {}

and reports
error Parsing error: Unexpected token catch (eslint)

Note that I've reduced the sample as much as possible, so please ignore that the code doesn't make sense.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: test2.js

Here's an additional test scenario, which also removes a closing }. This time it isn't with try/catch, but with if/for.

Comment 2

[Kai Engert (:KaiE:)]

I talked to Mark (Standard8). He noticed that the false behavior is likely caused by rules in comm/.eslintrc.js

When placing the test file outside comm, inside top mozilla, the problem cannot be reproduced.

Quoting him:
"There is a prettier config plugin that turns these off but I think the overrides there are turning them on and conflicting.
...
here's the list of rules that conflict with prettier, basically comm should just remove all references to them: https://github.com/prettier/eslint-config-prettier/blob/master/index.js "

Comment 3

[Kai Engert (:KaiE:)]

Removing the brace-style rule fixes the problem, for both attachments.

Could we remove it from eslintrc.js ?

Comment 4

[Mark Banner (:standard8)]

Basically all of this lot should be removed from the .eslintrc.js:

https://searchfox.org/comm-central/rev/6e3eab6ebb0852188e87a2a7251a986c696242e6/.eslintrc.js#37-60

Otherwise there's potential conflicts with prettier.

This also conflicts: https://searchfox.org/comm-central/rev/6e3eab6ebb0852188e87a2a7251a986c696242e6/mailnews/extensions/newsblog/.eslintrc.js#28

Whilst I'm here, I'll just note that calendar/.eslintrc.js also looks like it needs updating on multiple fronts - especially as extending from toolkit/.eslintrc.js isn't really sensible today (note: eslint inherits rules from the levels above).

It should definitely have the rules in eslint-prettier-config's index.js removed.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1616608-conflicting-rules-1.diff

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1616608-remove-calendar-rules-1.diff

This removes calendar/.eslintrc.js and fixes the errors (done with mach eslint --fix) so there's probably no need for a detailed inspection.

I've set the global config for the complexity rule to 80 (less strict than the default) because there are many calendar functions with more than the default. (Let's be honest, we mostly ignore this rule anyway.)

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9127742 [details] [diff] [review]
1616608-conflicting-rules-1.diff

Review of attachment 9127742 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, and thanks Mark for the input!

I'm not sure, but maybe some of these were leftovers since IIRC there were situations especially with braces (lack of such) that prettier wouldn't make right, only keep correct once they were there. So a double pass was needed, first eslint and then prettier.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9127743 [details] [diff] [review]
1616608-remove-calendar-rules-1.diff

Review of attachment 9127743 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 9

[Magnus Melin [:mkmelin]]

Quite nice to get all the no-else after returns handled.

Comment 10

[Paul Morris [:pmorris]]

Comment on attachment 9127743 [details] [diff] [review]
1616608-remove-calendar-rules-1.diff

Review of attachment 9127743 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Nice to have more standardization of the styles across the code base.  (Just as an aside, it's unfortunate that the new JS syntax for defining functions as object properties is identical to the syntax for calling functions.  It's more concise, but makes it harder to locate where a function is defined when searching the code base.)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7c243455ebe5
Remove ESLint rules that conflict with Prettier. r=mkmelin
https://hg.mozilla.org/comm-central/rev/f61f27a35d89
Remove separate ESLint rules in calendar. r=mkmelin,pmorris,fallen