Investigate and minimize eslint config differences between fluent.js and m-c
Categories
(Core :: Internationalization, defect, P5)
Tracking
()
Tracking | Status | |
---|---|---|
firefox68 | --- | fixed |
People
(Reporter: jaws, Assigned: berning5, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
Fluent.js uses a different eslint config than mozilla-central, this leads to some changes that need to be made to Fluent when it is imported to mozilla-central.
This bug should:
- Find the differences between the two configurations
2a. If the fluent.js configuration is lacking in rules, enable the missing ones
2b. If the m-c configuration is lacking in rules, see what it would take to enable them in m-c, enabling if it is minimal work - If any differences remain, disable linting on the m-c side since this code is linted in the github repo.
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Comment 1•6 years ago
|
||
I have a patch in bug 1539192 which updates Fluent.jsm and FluentSyntax.jsm to their latest versions. The patch removes a few eslint overrides which aren't required any longer. In fact, Fluent.jsm doesn't need any special eslint config, and FluentSyntax.jsm now uses the following one:
/* eslint object-shorthand: "off",
comma-dangle: "off",
no-labels: "off" */
The first two rules are an artifact of our build system which involves using Rollup to concatenate multiple source files into a single file. Here's the pattern I'm using in the entry point from which FluentSyntax.jsm is built:
import {FluentParser} from "../../fluent-syntax/src/parser";
import {FluentSerializer} from "../../fluent-syntax/src/serializer";
import * as ast from "../../fluent-syntax/src/ast";
import * as visitor from "../../fluent-syntax/src/visitor";
this.EXPORTED_SYMBOLS = [
...Object.keys({
FluentParser,
FluentSerializer,
}),
...Object.keys(ast),
...Object.keys(visitor),
];
Rollup translates import * as visitor ...
into const visitor = ({Visitor: Visitor, Transformer: Transformer});
, which violates object-shorthand and comma-dangle.
We could work around it by importing all exported symbols manually by name. It's easy to do for src/visitor which only has 2 exports. I'm concerned about src/ast, however, which has ca. 30 exports and manually importing all of them could lead to omissions in the future when more AST nodes are added. OTOH, the AST is fairly stable at this point, so maybe that's OK.
The "no-labels" rule is currently still needed because the parsing code can get quite complex in some places. I'd prefer to not change it at this time.
Comment 2•6 years ago
|
||
Staś, are you effectively saying there's nothing left to do here?
Comment 3•6 years ago
|
||
I'm not sure if I'm the right person to make the call here, but thanks for asking. I'll try to summarize my overly detailed previous comment, and perhaps we can try to find the right path forward together?
-
There's nothing left to do for intl/l10n/Fluent.jsm.
-
intl/l10n/FluentSyntax.jsm currently needs 3 eslint overrides:
object-shorthand
andcomma-dangle
because of how its built by Rollup in the fluent.js repo, andno-labels
because of the code using break in a switch inside of a while loop.
Removing the first two overrides is possible, although it creates a maintenance burden: any additions to the Fluent AST class list would need to be manually reflected (most likely by me) in the JSM exports. It's not a hard task, but something that is easily overlooked or easy to make a stupid mistake in.
Removing the no-labels
override is also possible, but it requires a refactoring of the parser code. I'm open to doing it, but I'd prefer to postpone it until after Fluent 1.0 is published and the dust settles.
Is there harm in keeping those three overrides inside of the intl/l10n/FluentSyntax.jsm file?
Comment 4•6 years ago
|
||
I forgot to mention the two other files in the intl/l10n repo: Localization.jsm and DOMLocalization.jsm. Zibi, how much manual work is involved right now in maintaining them and copying them from the fluent.js repo? Do they conflict with mozilla-central's default eslint rules in any way?
Reporter | ||
Comment 5•6 years ago
|
||
In comment 0, option #2 says that we could also just disable linting of this code on the m-c side since it is vendored in and is linted on the Github side.
Comment 6•6 years ago
|
||
Good point. If this option is on the table, I think it would the the easiest solution right now and also the most convenient from my point of view.
Reporter | ||
Comment 7•6 years ago
|
||
Okay let's go with that. Avery, can you write the patch to disable linting in the /intl/l10n/ directory?
Comment 9•6 years ago
|
||
Updated•6 years ago
|
Comment 10•6 years ago
|
||
Comment 11•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Description
•