Closed Bug 1527075 Opened 6 months ago Closed 4 months ago

Investigate and minimize eslint config differences between fluent.js and m-c

Categories

(Core :: Internationalization, defect, P5)

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: jaws, Assigned: berning5, Mentored)

References

(Blocks 3 open bugs)

Details

Attachments

(1 file)

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:

  1. 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
  2. If any differences remain, disable linting on the m-c side since this code is linted in the github repo.
Priority: -- → P5
Version: 57 Branch → Trunk
Assignee: nobody → berning5
Status: NEW → ASSIGNED

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.

Staś, are you effectively saying there's nothing left to do here?

Flags: needinfo?(stas)

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 and comma-dangle because of how its built by Rollup in the fluent.js repo, and
    • no-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?

Flags: needinfo?(stas)

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?

Flags: needinfo?(gandalf)

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.

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.

Okay let's go with that. Avery, can you write the patch to disable linting in the /intl/l10n/ directory?

Flags: needinfo?(berning5)

Yes, I can do that.

Flags: needinfo?(berning5)
Attachment #9058303 - Attachment description: Bug 1527075 - Investigate and minimize eslint config differences between fluent.js and m-c → Bug 1527075 - stop linting intl/l10n/ in m-c given it's linted in github
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f068f655d8df
stop linting intl/l10n/ in m-c given it's linted in github r=Gijs,jaws
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Flags: needinfo?(gandalf)
You need to log in before you can comment on or make changes to this bug.