Open Bug 1487642 Opened 6 years ago Updated 2 months ago

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

Categories

(Developer Infrastructure :: Lint and Formatting, enhancement)

Product:
Developer Infrastructure
Component:
Lint and Formatting
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: standard8, Unassigned)

References

Details

(Keywords: meta)

User Story

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

* 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-console: https://eslint.org/docs/rules/no-console
** Yes, avoid unnecessary console logs, and used dedicated loggers where needed.
* no-constant-condition: https://eslint.org/docs/rules/no-constant-condition
* no-case-declarations: https://eslint.org/docs/rules/no-case-declarations

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

* no-inner-declarations: https://eslint.org/docs/rules/no-inner-declarations
** Superseded now we're in ES6.
* 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.

Already Done
============
* for-direction: https://eslint.org/docs/rules/for-direction
* no-compare-neg-zero: https://eslint.org/docs/rules/no-compare-neg-zero
* no-fallthrough: https://eslint.org/docs/rules/no-fallthrough
* no-global-assign: https://eslint.org/docs/rules/no-global-assign
* no-new-symbol: https://eslint.org/docs/rules/no-new-symbol
* no-this-before-super: https://eslint.org/docs/rules/no-this-before-super
* no-unused-labels: https://eslint.org/docs/rules/no-unused-labels
* constructor-super: https://eslint.org/docs/rules/constructor-super
* require-yield: https://eslint.org/docs/rules/require-yield
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.
(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).
(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)
Depends on: 1488445
Product: Firefox Build System → Developer Infrastructure
Severity: normal → S3
User Story: (updated)
Depends on: 1880535
Depends on: 1881262
User Story: (updated)

I've just updated the user story with the status of the remaining rules having discussed with :Mossop and :Gijs, I'll be filing bugs/patches for them.

Depends on: 1881265
Depends on: 1881266
You need to log in before you can comment on or make changes to this bug.