[meta] Consider enabling more of the eslint:recommended rules

NEW
Unassigned

Status

7 months ago
3 months ago

People

(Reporter: standard8, Unassigned)

Tracking

({meta})

Trunk

Firefox Tracking Flags

(Not tracked)

Details

User Story

Should enable
=============

* for-direction: https://eslint.org/docs/rules/for-direction

* getter-return: https://eslint.org/docs/rules/getter-return
** There's quite a few places where we're not returning anything from getters or doing things like `if (...) return...;` and implicitly returning undefined in the else case.

* no-compare-neg-zero: https://eslint.org/docs/rules/no-compare-neg-zero

* no-global-assign: https://eslint.org/docs/rules/no-global-assign

* constructor-super: https://eslint.org/docs/rules/constructor-super

* no-new-symbol: https://eslint.org/docs/rules/no-new-symbol

* no-this-before-super: https://eslint.org/docs/rules/no-this-before-super

* require-yield: https://eslint.org/docs/rules/require-yield

Never enable
============

* no-inner-declarations: https://eslint.org/docs/rules/no-inner-declarations
** Superseded now we're in ES6.

Possibly enable
===============

* no-console: https://eslint.org/docs/rules/no-console
** We use console logging in lots of places, but may hurt perf.

* no-constant-condition: https://eslint.org/docs/rules/no-constant-condition
** We mainly hit this in tests, and there's `break` or `return` to exit the loop.

* no-case-declarations: https://eslint.org/docs/rules/no-case-declarations
** This is done in lots of places, I don't think it causes script errors, but should we tidy this up?

* no-fallthrough: https://eslint.org/docs/rules/no-fallthrough
** There's a add-a-comment way to allow the fallthrough explicitly, though not sure if we do want to change this or not.

* no-useless-escape: https://eslint.org/docs/rules/no-useless-escape
** This seems to be picking up lots of useless escape instances. I've not worked out yet if these are valid or not.

No need to enable
=================

* no-unused-labels: https://eslint.org/docs/rules/no-unused-labels
** We already don't allow labels, so we don't need this.
(Reporter)

Description

7 months ago
ESLint has a set of rules which are in the "eslint:recommended" configuration. You can see them by looking for the tick marks on the rule list:

https://eslint.org/docs/rules/

We do enable a lot of these, but there are some which aren't yet enabled. The user story contains the ones that aren't currently enabled and what I think we should do with them.

Comments on the lists welcome.
(Commenting on User Story)
> * no-inner-declarations: https://eslint.org/docs/rules/no-inner-declarations
> ** I'm not sure it is worth doing this, as our javascript seems to support
> it.

This seems to have been superseded by block-scoped-var.

Comment 2

7 months ago
(Commenting on User Story)
> Never enable
> ============
> 
> * no-console: https://eslint.org/docs/rules/no-console
> ** We use console logging in lots of places.

I wonder if we need to re-evaluate this given things like bug 1486885 comment 7. We might need more details there about why/how that improvement relates to the removal of console.error things, but if it really does, perhaps we actually do want this rule, with specific exceptions where we're confident there's no perf risk (e.g. tests).
(Reporter)

Comment 3

7 months ago
(In reply to Dave Townsend [:mossop] (he/him) from comment #1)
> (Commenting on User Story)
> > * no-inner-declarations: https://eslint.org/docs/rules/no-inner-declarations
>
> This seems to have been superseded by block-scoped-var.

Good point, moved it to the never enable.

(In reply to :Gijs (he/him) from comment #2)
> (Commenting on User Story)
> > * no-console: https://eslint.org/docs/rules/no-console
> > ** We use console logging in lots of places.
> 
> I wonder if we need to re-evaluate this given things like bug 1486885
> comment 7. We might need more details there about why/how that improvement
> relates to the removal of console.error things, but if it really does,
> perhaps we actually do want this rule, with specific exceptions where we're
> confident there's no perf risk (e.g. tests).

That's a good point. I think there might be some sort of review of logging generally that we need - there's multiple ways of doing it, and we aren't consistent (e.g. direct logging, Log.jsm etc).
User Story: (updated)
(Reporter)

Updated

7 months ago
Depends on: 1488445
Keywords: meta
You need to log in before you can comment on or make changes to this bug.