1480393 - Lots of ESLint failures, fallout from bug 1478305

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Description

[Geoff Lankow (:darktrojan)]

Bug 1478305 has made the linter cleverer when dealing with module imports. Since we're not in the habit of naming our modules the same as what they export, it has caused a ton of errors.

https://taskcluster-artifacts.net/eEdTRtGAQgOvRcWD0EX7Ig/0/public/logs/live_backing.log

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1480393-eslint-fallout-1.diff

Here's what I have so far, almost all of it imported modules we don't appear to use any more.

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1480393-eslint-fallout-mc-1.diff

… and here's some changes to apply to mozilla-central. It's a list of everything we export from modules (actually only calendar and three other common ones).

Naturally they won't take this, but it's helping sort things out. Perhaps we can persuade them to modify their new code to look for an extra file, which we keep in comm-central.

Comment 3

[Geoff Lankow (:darktrojan)]

Actually with both of these patches applied there's only three failures remaining, which are weird uses in the gdata provider, which we can disable the rule for, or something.

Comment 4

[Geoff Lankow (:darktrojan)]

If we just had our own list, there's very few modules from m-c we'd need to add to it. Okay, so most of our stuff isn't linted, but let's ignore that for now.

Comment 5

[Geoff Lankow (:darktrojan)]

Attachment: 1480393-eslint-fallout-2.diff

Okay, so the real questions are:

1. Where do we want to put our list of modules?
2. Can we convince the m-c people to have the code look for it there?

Comment 6

[Geoff Lankow (:darktrojan)]

Mark, do you think comm-central having its own version of modules.json, and modification of helpers.js to look for it (probably something like below) is a good idea?

> diff --git a/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js b/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
> --- a/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
> +++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
> @@ -49,11 +49,13 @@ const imports = [
>  const workerImportFilenameMatch = /(.*\/)*(.*?\.jsm?)/;
>  
>  module.exports = {
>    get modulesGlobalData() {
>      if (!gModules) {
> -      if (this.isMozillaCentralBased()) {
> +      if (fs.existsSync(path.join(this.rootDir, "comm", "modules.json"))) {
> +        gModules = require(path.join(this.rootDir, "comm", "modules.json"));
> +      } else if (this.isMozillaCentralBased()) {
>          gModules = require(path.join(this.rootDir, "tools", "lint", "eslint", "modules.json"));
>        } else {
>          gModules = require("./modules.json");
>        }
>      }

Comment 7

[Mark Banner (:standard8)]

I would strongly recommend switching to using this format of imports:

const {Foo, Bar} = ChromeUtils.import(..., {});

It is where we are heading generally and has the benefit of less work for ESLint to do, as well as being more explicit and avoiding the hard coded lookup table.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Mark Banner (:standard8) from comment #7)
> I would strongly recommend switching to using this format of imports:
> 
> const {Foo, Bar} = ChromeUtils.import(..., {});
> 
> It is where we are heading generally and has the benefit of less work for
> ESLint to do, as well as being more explicit and avoiding the hard coded
> lookup table.

I thought this wasn't recommended since what ChromeUtils.import() returns is the BackstagePass and not the exported symbols, and that importing into the empty object would decrease perf because of less caching? Or did this change in the meanwhile? I agree it is better for ESLint though and I like the explicitness of it.

Comment 9

[Mark Banner (:standard8)]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #8)
> (In reply to Mark Banner (:standard8) from comment #7)
> > I would strongly recommend switching to using this format of imports:
> > 
> > const {Foo, Bar} = ChromeUtils.import(..., {});
> > 
> > It is where we are heading generally and has the benefit of less work for
> > ESLint to do, as well as being more explicit and avoiding the hard coded
> > lookup table.
> 
> I thought this wasn't recommended since what ChromeUtils.import() returns is
> the BackstagePass and not the exported symbols, and that importing into the
> empty object would decrease perf because of less caching? Or did this change
> in the meanwhile? I agree it is better for ESLint though and I like the
> explicitness of it.

I haven't heard anything about that, but I haven't explicity asked... there are things like bug 1432901 where we're investigating to switch to es6 module style, and I know switching to the const {Foo} = ... format is likely to be part of that.

Ted, can you advise the the current performance aspects here?

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1480393-eslint-fallout-3.diff

This is the most exciting patch you'll review all week. Almost as fun as writing it.

I've converted all the offending |ChromeUtils.import(xyz)| lines to |const { abc } = ChromeUtils.import(xyz, {})|, except in .js files where I've used var instead of const when importing cal, ICAL, fixIterator, or MailServices, as we're likely to import twice and be trying to redefine a const.

I made a Try run recently, but it's not quite the latest version, should be close enough: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6c2c08a262057f4de334fb87fa610eeaa19f2779&group_state=expanded

Comment 11

[Geoff Lankow (:darktrojan)]

Attachment: 1480393-eslint-fallout-4.diff

Apparently you can't export a const. And I should've rerun the tests *before* posting the patch.

Comment 12

[Geoff Lankow (:darktrojan)]

Attachment: 1480393-eslint-fallout-4.diff

… and post the right patch. I think I'm going to leave this bug for a while.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Let's wait for Ted's response before I review these. Thanks for preparing the patches though!

Comment 14

[Ted Campbell [:tcampbell]]

(I totally lost track of this and didn't realize it was holding on me =\)

I've given some mixed messages on this stuff already and have learned a bit more as we've worked through more JSM and FrameScript changes...

In general naming the exports is preferable for performance going forward since the engine no longer needs to determine who is currently implementing the give method. The explicit names also sets us up for switching to ES6 modules in some future in a semi-automated way.

As was pointed out above, the return value of the import call used to be a BackstagePass and not the exported symbols. Currently this is a different type of exotic but fills the same role as a BackstagePass. It also suffers from the same problem that let/const is not visible due to some nuance of the definition of a Realm vs a Global.

Another dirty secret is that |let {Foo} = ChromeUtils.import("foo.jsm", {})| is (surprisingly) nonsense. The object literal is filled in with exports but then thrown away. Using |let {Foo} = ChromeUtils.import("foo.jsm", null);| is a better fit.

Going forward, I think we need to fork the behavior of ChromeUtils.import so that it actual returns the exported symbols. We'd need an escape-hatch version that returns the existing value so that tests that cheat can still work.

Using |const {Foo} = ChromeUtils.import("foo.jsm", null);| is preferred from a technical perspective, but it completely undermines EXPORTED_SYMBOLS. I'm not sure what the best choices are today. Perhaps prioritizing adding a ChromeUtils.importJSM as discussed in Bug 1432901.

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: 1480393-eslint-fallout-5.diff

Now using null, instead of {}.

Comment 16

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9002648 [details] [diff] [review]
1480393-eslint-fallout-5.diff

Review of attachment 9002648 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the comments all! Looking forward to ES6 imports! r+wc

::: calendar/base/content/calendar-extract.js
@@ +7,2 @@
>  ChromeUtils.import("resource://gre/modules/Preferences.jsm");
>  ChromeUtils.import("resource://gre/modules/Services.jsm");

Does it make sense to also translate the Preferences/Services.jsm imports?

::: calendar/providers/storage/calStorageCalendar.js
@@ +10,5 @@
> +    DB_SCHEMA_VERSION,
> +    getSqlTable,
> +    upgradeDB
> +} = ChromeUtils.import("resource://calendar/modules/calStorageUpgrade.jsm", null);
> +const { CAL_ITEM_FLAG, newDateTime } = ChromeUtils.import("resource://calendar/modules/calStorageHelpers.jsm", null);

Sometimes const and sometimes var. Any specific reason? Might be good to stay consistent.

::: common/src/customizeToolbar.js
@@ +547,5 @@
>    Services.prefs.setBoolPref("mail.tabs.drawInTitlebar", !titlebarCheckbox.checked);
>  
>    // Bring the customizeToolbar window to front (on linux it's behind the main
>    // window). Otherwise the customization window gets left in the background.
> +  setTimeout(function() { window.focus(); }, 100);

prefer-arrow-callback while you are at it

setTimeout(() => window.focus(), 100);

Comment 17

[Geoff Lankow (:darktrojan)]

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #16)
> Does it make sense to also translate the Preferences/Services.jsm imports?

When do you want this to land? ;-)

> Sometimes const and sometimes var. Any specific reason? Might be good to
> stay consistent.

See comment 10 on that. I'd rather use const as we shouldn't be redefining stuff, but that causes problems if we re-import something.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7973e4623106
Explicitly declare what is being imported with ChromeUtils.import; r=Fallen

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1480393-follow-up.patch

Comment 20

[Geoff Lankow (:darktrojan)]

Comment on attachment 9003232 [details] [diff] [review]
1480393-follow-up.patch

Review of attachment 9003232 [details] [diff] [review]:
-----------------------------------------------------------------

I'd forgotten about that one.

Comment 21

[Geoff Lankow (:darktrojan)]

Attachment: 1480393-caldav-followup.diff

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 9003347 [details] [diff] [review]
1480393-caldav-followup.diff

Sorry, I'm not a Calendar peer. Why does it need to be const?

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8b16afac8b1e
Follow-up: Remove throw-away object in ChromeUtils.import() call. r=darktrojan

Comment 24

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #22)
> Sorry, I'm not a Calendar peer. Why does it need to be const?

I don't think it should. Caldav seems broken on trunk, "redeclaration of const setTimeout" - so the patch would fix that.

Comment 25

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin from comment #24)
> (In reply to Jorg K (GMT+2) from comment #22)
> > Sorry, I'm not a Calendar peer. Why does it need to be const?
> I don't think it should. Caldav seems broken on trunk, "redeclaration of
> const setTimeout" - so the patch would fix that.
Oops, got confused, I meant to ask for 'var'. Answer in comment #10. Anyway, Magnus, you want to stick an r+ onto it?

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/af4b954adafd
Follow-up: fix redeclaration of const setTimeout in caldav provider. r=philipp