1617959 - Unify eslint parser throughout the tree potentially allowing new js features

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, task, P3)

Description

[Ed Lee :Mardak]

Currently devtools and newtab configure eslint to use "parser": "babel-eslint" and these directories can use newer js features, e.g., static class members, nullish operator (??), optional chaining (?.).

The babel-eslint@10.0.3 is already included at the top level package.json, so maybe we can configure eslint to use babel-eslint for all of m-c so developers can use newer features?

Looks like it's not entirely trivial of just setting the parser as it seems to result in:

  5:5  error  'EXPORTED_SYMBOLS' is assigned a value but never used.  no-unused-vars (eslint)

And there might be performance differences.

Alternatively, newtab directory currently parses fine without babel-eslint, so that could be removed. (Looks like newtab unnecessarily specifies version 2018 and ecmaFeatures jsx isn't necessary anymore either??)

Comment 1

[Ed Lee :Mardak]

A quick check of removing babel-eslint from devtools/client/debugger/.eslintrc.js seems to show ./mach lint -l eslint devtools/client/debugg passing fine. It seems to also pass fine when removing version: 2016 and jsx: true.

Comment 2

[Jason Laster [:jlast]]

Thanks Ed.

I'd love to be able to use these new features.

Comment 3

[Dave Townsend [:mossop]]

I ran a quick test comparing running eslint over the tree with and without the babel-eslint parser. The results:

Normal parser:

✖ 0 problems (0 errors, 6 warnings)

real	9m13.862s
user	12m10.686s
sys	0m55.550s

Babel parser:

✖ 15849 problems (15849 errors, 6 warnings)

real	10m44.262s
user	13m29.739s
sys	0m57.180s

As far as I can see all of the errors are no-unused-vars so it looks like using the babel parser breaks some of our plugin's functions that marks certain variables (EXPORTED_SYMBOLS for example) as used.

Adding a minute to the run time is around 8% and so not insignificant.

Comment 4

[Jason Laster [:jlast]]

what is the default eslint parser?

now that ?? and ?. are further along, i wouldnt be surprised if they are coming to an updated version of eslint.

Comment 5

[Dave Townsend [:mossop]]

(In reply to Jason Laster [:jlast] from comment #4)

what is the default eslint parser?

now that ?? and ?. are further along, i wouldnt be surprised if they are coming to an updated version of eslint.

They use Acorn and want to add support but are blocked on estree standardizing AST definitions, see https://github.com/eslint/eslint/issues/12642.

Comment 6

[Mark Banner (:standard8)]

(In reply to Ed Lee :Mardak from comment #0)

Looks like it's not entirely trivial of just setting the parser as it seems to result in:

  5:5  error  'EXPORTED_SYMBOLS' is assigned a value but never used.  no-unused-vars (eslint)

That's interesting, we have a rule to mark these as used: https://searchfox.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/mark-exported-symbols-as-used.js

Maybe something in Babel's parser is changing these so they're not in the global scope (and hence the rule isn't running)?

Looking at ESLint's history, I'm currently thinking that they are likely one to two months out from a release that includes the new features we want. So I think it might be worth moving forward with this, if the work isn't too involved.

I did spot that we'll need to upgrade Prettier as well (bug 1620556). I've already got patches up for a smaller intermediate upgrade.

I suspect its a trivial patch, but can anyone post the patch to try this out? (and maybe have a look at what that rule is doing).

Comment 7

[Dave Townsend [:mossop]]

Attachment: Bug 1617959: Switch to the babel-eslint parser and turn on support for optional chaining and nullish coalescing operator syntaxes. r=Standard8

The babel-eslint parser defaults to assuming all scripts are modules. Although
we're moving in that direction it seems reasonable to set the default as regular
scripts for now. All the places that were previously overriding the parser are
already specifying the sourceType in their eslint configs.

I chose to put the babel config in a file that babel itself won't look to avoid
the risk of this config impacting the use of babel elsewhere in the tree.

Comment 8

[Dave Townsend [:mossop]]

The issue is just that eslint's default parser defaults to a sourceType of script while babel-eslint defaults to module. This throws off some global scope checking (I didn't dig in too much but I think there is no "Program" in the AST for modules, just a "Module", and the scope object of the global is of type "module" and has a parent scope which is the global scope).

Since for now the majority of our scripts are not modules and files that are modules are already declared as such in deeper eslint configs we can just default to script at the top-level.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40bfe22bad4c
Switch to the babel-eslint parser and turn on support for optional chaining and nullish coalescing operator syntaxes. r=Standard8

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/40bfe22bad4c