1572047 - [Meta] Use Prettier for formatting JavaScript in Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Magnus Melin [:mkmelin]]

Bug 1561435 (and friends) re-formatted the JavaScript code in mozilla-central. (See meta bug 1558517.) We should follow through for Thunderbird.

See https://groups.google.com/forum/#!topic/mozilla.dev.platform/vSTs07US0cg

Looks like basically what we need to do is

• remove the line "prettier/prettier": "off" from .eslintrc
• run ./mach lint comm/ --fix

Preparation work is at least adding "curly: all" to eslint and do an intermediary step to fix that up (through mach lint --fix), or otherwise we get the ugly online if-elses where braces are missing.

Comment 1

[Jorg K (CEST = GMT+2)]

Can we do that at the end for the 76 cycle like we did for the clang-cl formatting at the end of 68? Otherwise backports will become a nightmare unless you also reformat the branches.

Comment 2

[Magnus Melin [:mkmelin]]

I think we should do it pretty soon and run over the older branches too, like Firefox (also) did.

Comment 3

[Paul Morris [:pmorris]]

For Firefox they landed the big change right before a beta release to avoid beta uplift conflicts. Seems like we should do that as well. The next beta release is FF70, on Tues Sept. 3rd. Assuming TB follows that schedule, that would give us about three weeks. (Then we'd follow-up with changes to the older release branches to avoid backport conflicts there.)

Comment 4

[Wayne Mery (:wsmwk)]

Bear in mind with releasing 68 much later than firefox, we can expect to have some fair amount of regression bugs in the next 1-2 months.

Comment 5

[Jorg K (CEST = GMT+2)]

Discussed with Paul and Patrick on IRC:
Best day for action is the 29th or 30th of August before the next merge on 2nd of September.
We need to exclude 3rd-party libraries, like they are used in chat, questionable are ical.js and jsmime.js.

Comment 6

[Paul Morris [:pmorris]]

Attachment: part1-turn-on-prettier-ignored-0.patch

1 of 2 WIP patches for demonstration purposes, in case anyone is curious what this will look like.

This one turns on prettier, but ignores all the top level directories, so nothing is formatted. This allows step by step conversion and lets us ignore particular files or directories, as needed.

Comment 7

[Paul Morris [:pmorris]]

Attachment: part2-wip-prettier-addrbook-0.patch

2 of 2 WIP patches for demonstration purposes. This applies the new auto-formatting to the mailnews/addrbook directory.

Here I temporarily deactivated prettier by commenting out that line in .eslintrc.js, and set curly: ["error", "all"] there for eslint. Then I used mach lint ... --fix to insert omitted curly brackets. Then I enabled prettier and did mach lint ... --fix to do the prettier formatting.

In the process I manually fixed up a few places that showed up as linting issues. These were mostly placement issues related to comments and the inserted curlies that the automation couldn't handle.

Comment 8

[Paul Morris [:pmorris]]

Attachment: part2-wip-prettier-addrbook-1.patch

Comment 9

[Jorg K (CEST = GMT+2)]

This shouldn't be called "prettier", it should be called "longer" :-(

Can you increase the line length to avoid nonsense like this:

-var {MailServices} = ChromeUtils.import("resource:///modules/MailServices.jsm");
-var {
-  fixIterator,
-} = ChromeUtils.import("resource:///modules/iteratorUtils.jsm");
-var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");
+var { MailServices } = ChromeUtils.import(
+  "resource:///modules/MailServices.jsm"
+);
+var { fixIterator } = ChromeUtils.import(
+  "resource:///modules/iteratorUtils.jsm"
+);
+var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");

Comment 10

[Magnus Melin [:mkmelin]]

It is slightly unfortunate for the imports. Perhaps we should use one of the .eslint max-len ignore options (https://eslint.org/docs/rules/max-len) for this case. Probably ChromeUtils.import will be replaced with standard module imports in the next year/s. That will make it shorter too...

Comment 11

[Paul Morris [:pmorris]]

(In reply to Magnus Melin [:mkmelin] from comment #10)

Perhaps we should use one of the .eslint max-len ignore options (https://eslint.org/docs/rules/max-len) for this case.

I wasn't able to get this to work. Given a max-len rule with an ignorePattern with a regex that matches these import lines, Prettier still wants to add in the line breaks. I also tried with a simpler regex to make sure that the regex wasn't the issue. (I added the other ignore clauses because without them eslint reported max-len errors for those things.)

 "max-len": ["error",
  {
    "ignoreStrings": true,
    "ignoreComments": true,
    "ignorePattern": "^\\s*var\\s*\\{\\s.+\\s\\}\\s=\\s*.+\\.import\\(",
  },
],

Comment 12

[Paul Morris [:pmorris]]

Attachment: part3-wip-calendar-prettier-config-0.patch

If calendar code is to remain formatted with 100 characters per line and 4 space indentation, then we will will need to add a .prettierrc file like this one. Prettier does not get such settings from the eslint config and so it needs this .prettierrc file to successfully auto-format calendar code. (I tested this with one file.)

Comment 13

[Paul Morris [:pmorris]]

Notes on a few conflicts between eslint configs and prettier, and proposed resolutions. Doing needinfo for feedback on the proposed resolutions. I'd like to reach agreement on these before Thurs Aug. 29th so I can go ahead with the changes starting then.

I started to do a smoke test by converting the comm/mailnews/db/gloda/ directory (to land just those changes first), but that uncovered a case where the current eslint rules conflict with prettier. After some more exploring I found 3 such cases. All 3 involve calendar but only the 1st involves Thunderbird. (The 4th below is not a conflict just a related note.)

1 of 4. generator-star-spacing: https://cn.eslint.org/docs/5.0.0/rules/generator-star-spacing
(Thunderbird and Calendar)

Prettier formats generator functions as follows, so the eslint rule needs to match.

function* generator() {}  // This is fine, nothing needs to change.

var anonymous = function*() {};  // Currently: |var anonymous = function* () {};|

var shorthand = { *generator() {} };  // Currently: |var shorthand = { * generator() {} };|

class Class {
  static *method() {}  // Currently: |static * method() {}|
}

Current Thunderbird eslint rule:

// Require function* name()
"generator-star-spacing": ["error", {"after": true, "before": false}],

Current Calendar eslint rule:

// Enforce the spacing around the * in generator functions.
"generator-star-spacing": [2, "after"],

Proposed Prettier-friendly rule:

// Require function* name()
// Require let obj = { *name() {...} };
// Require let anonymous = function*() {...};
"generator-star-spacing": ["error", {
  "before": false,
  "after": true,
  "anonymous": "neither",
  "method": { "before": true, "after": false }}],

2 of 4. object-curly-newline: https://eslint.org/docs/rules/object-curly-newline
(calendar only)

The cases where I found this conflict involved a line that was too long, so Prettier breaks it up by adding line breaks to object literals, but the Calendar eslint rule prevents such line breaks when there's only one property. Here is Prettier formatting that eslint doesn't like:

customElements.define("agenda-header-richlist-item", MozAgendaHeaderRichlistItem, {
    extends: "richlistitem",
});

Current eslint rule:

// Enforce consistent line breaks inside braces
"object-curly-newline": [2, { multiline: true }],

Proposed Prettier-friendly change:

just remove the eslint rule

3 of 4. quote-props: https://eslint.org/docs/rules/quote-props
(calendar only)

By default Prettier removes quotes around property names in object literals when possible and only keeps them "as-needed". But the current Calendar eslint rule requires all of them to have quotes if any have quotes ("consistent-as-needed"), and always puts quotes around any JS reserved keywords ("keywords"). Prettier has a setting for "consistent" which approximates eslint's "consistent-as-needed" setting, but there are cases where they still conflict, so best option is to let Prettier handle this rather than eslint. (https://prettier.io/docs/en/options.html#quote-props)

var foo = { "extends": 0, "foo": 1 };          // Calendar eslint ("extends" is a JS reserved keyword)
var foo = { "two-words": 0, "foo": 2 };        // Calendar eslint

var foo = { extends: 0, foo: 1 };              // Prettier (with quote-props: "consistent")
var foo = { "two-words": 0, "foo": 2 };        // Prettier (with quote-props: "consistent")

var foo = { extends: 0, foo: 1 };              // Prettier (default)
var foo = { "two-words": 1, foo: 2 };          // Prettier (default)

Current eslint:

// Quoting style for property names
"quote-props": [2, "consistent-as-needed", { keywords: true }],

Proposed Prettier-friendly option A:

remove the eslint rule and add this calendar/.prettierrc config:
"quoteProps": "consistent",

Another Prettier-friendly option B:

just remove the eslint rule, and use the Prettier default

4 of 4. curly: https://eslint.org/docs/rules/curly
Per discussion with mkmelin I'll be adding a curly: ["error", "all"] to Thunderbird's eslint. This won't affect calendar which already uses this style. It means for example that this:

if (x)
  foo();
else
  bar();

will become:

if (x) {
  foo();
} else {
  bar();
}

Comment 14

[Geoff Lankow (:darktrojan)]

Some of those rules were removed from mozilla-central when they turned on Prettier. They were added to our config because we had Prettier disabled and wanted to keep the checks.

https://hg.mozilla.org/comm-central/rev/1ba917da139c6e707d6d964c5d1de142cab1e666

Comment 15

[Magnus Melin [:mkmelin]]

For generator functions, absolutely - I think our current style is rather wrong.
For the rest too, I'd just use the standard prettier format. Personally I think all things "special" have a pretty high cost long term.

Comment 16

[Paul Morris [:pmorris]]

(In reply to Geoff Lankow (:darktrojan) from comment #14)

Some of those rules were removed from mozilla-central when they turned on Prettier. They were added to our config because we had Prettier disabled and wanted to keep the checks.

https://hg.mozilla.org/comm-central/rev/1ba917da139c6e707d6d964c5d1de142cab1e666

Aha, nice, thanks. So we can remove these again, which takes care of "generator-star-spacing".

Looks like the main question is #3 -- whether calendar wants to have the prettier setting |"quoteProps": "consistent"| or just go with the prettier default |"quoteProps": "as-needed"|.

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

I wonder where my comment went, I already answered this but it seems it never made it. I'm ok with the proposed changes, including using the prettier default for quoteProps if that is also what m-c does.

Comment 18

[Paul Morris [:pmorris]]

Attachment: temp-update-eslint-prettier-configs-for-rebasing-0.patch

Just a patch to use on a temporary basis to assist with rebasing patches from before the Prettier formatting. See write-up here:

https://wiki.mozilla.org/Thunderbird/Rebasing_Patches_and_Prettier_Formatting

Comment 19

[Paul Morris [:pmorris]]

Closing this now that we have documentation:
https://developer.thunderbird.net/contributing/fixing-a-bug/lint-and-format-code