Lots of ESLint failures, fallout from bug 1478305

RESOLVED FIXED in 6.5

Status

enhancement
RESOLVED FIXED
Last year
11 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

unspecified
Dependency tree / graph

Details

Attachments

(3 attachments, 6 obsolete attachments)

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
Posted patch 1480393-eslint-fallout-1.diff (obsolete) — Splinter Review
Here's what I have so far, almost all of it imported modules we don't appear to use any more.
… 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.
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.
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.
Posted patch 1480393-eslint-fallout-2.diff (obsolete) — Splinter Review
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?
Assignee: nobody → geoff
Attachment #8996985 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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");
>        }
>      }
Flags: needinfo?(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.
Flags: needinfo?(standard8)
(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.
(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?
Flags: needinfo?(tcampbell)
Posted patch 1480393-eslint-fallout-3.diff (obsolete) — Splinter Review
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
Attachment #8996988 - Attachment is obsolete: true
Attachment #8997263 - Attachment is obsolete: true
Attachment #8997760 - Flags: review?(philipp)
Posted patch 1480393-eslint-fallout-4.diff (obsolete) — Splinter Review
Apparently you can't export a const. And I should've rerun the tests *before* posting the patch.
Attachment #8997760 - Attachment is obsolete: true
Attachment #8997760 - Flags: review?(philipp)
Attachment #8997763 - Flags: review?(philipp)
Posted patch 1480393-eslint-fallout-4.diff (obsolete) — Splinter Review
… and post the right patch. I think I'm going to leave this bug for a while.
Attachment #8997763 - Attachment is obsolete: true
Attachment #8997763 - Flags: review?(philipp)
Attachment #8997766 - Flags: review?(philipp)
Let's wait for Ted's response before I review these. Thanks for preparing the patches though!
(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.
Flags: needinfo?(tcampbell)
Now using null, instead of {}.
Attachment #8997766 - Attachment is obsolete: true
Attachment #8997766 - Flags: review?(philipp)
Attachment #9002648 - Flags: review?(philipp)
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);
Attachment #9002648 - Flags: review?(philipp) → review+
(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.
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/7973e4623106
Explicitly declare what is being imported with ChromeUtils.import; r=Fallen
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → 6.5
Attachment #9003232 - Flags: review?(geoff)
Comment on attachment 9003232 [details] [diff] [review]
1480393-follow-up.patch

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

I'd forgotten about that one.
Attachment #9003232 - Flags: review?(geoff) → review+
Attachment #9003347 - Flags: review?(jorgk)
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?
Attachment #9003347 - Flags: review?(jorgk) → review?(philipp)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8b16afac8b1e
Follow-up: Remove throw-away object in ChromeUtils.import() call. r=darktrojan
(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.
(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?
Attachment #9003347 - Flags: review?(philipp) → review+
Depends on: 1485886
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/af4b954adafd
Follow-up: fix redeclaration of const setTimeout in caldav provider. r=philipp
You need to log in before you can comment on or make changes to this bug.