Unify eslint parser throughout the tree potentially allowing new js features
Categories
(Developer Infrastructure :: Lint and Formatting, task, P3)
Tracking
(firefox76 fixed)
Tracking | Status | |
---|---|---|
firefox76 | --- | fixed |
People
(Reporter: Mardak, Assigned: mossop)
References
Details
Attachments
(1 file)
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??)
Reporter | ||
Comment 1•4 years ago
|
||
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•4 years ago
|
||
Thanks Ed.
I'd love to be able to use these new features.
Assignee | ||
Comment 3•4 years ago
|
||
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•4 years ago
|
||
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.
Assignee | ||
Comment 5•4 years ago
|
||
(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.
Updated•4 years ago
|
Comment 6•4 years ago
|
||
(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).
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 7•4 years ago
|
||
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.
Assignee | ||
Comment 8•4 years ago
|
||
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.
Pushed by elee@mozilla.com: 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•4 years ago
|
||
bugherder |
Updated•1 year ago
|
Description
•