1240068 - eslint Cu.import rule does not work properly

Status: RESOLVED WONTFIX

(Testing :: General, defect)

Description

[Tom Tromey :tromey]

The components-import rule attempts to handle Cu.import.
However, there are some uses it doesn't handle correctly.

For example:

pokyo. ./mach eslint --no-ignore devtools/client/styleeditor/styleeditor-panel.js 
[...]
   91:24  error    "_" is not defined                                                            no-undef


However, this is defined in StyleEditorUtil.jsm, which is imported by
styleeditor-panel.js using:

Cu.import("resource://devtools/client/styleeditor/StyleEditorUtil.jsm");

Comment 1

[:Gijs (he/him)]

Per https://dxr.mozilla.org/mozilla-central/source/testing/eslint-plugin-mozilla/docs/components-imports.rst, shouldn't you be making it explicit that you're fetching the _ symbol in that file? By default it'll only declare the file-name's own name.

Of course, that's redundant in a way, because now you have to define it both in the exports of the module and when you import. The alternative would mean actually loading the module you're importing, and I don't know how we would teach the plugin to do that, because it has no idea about how to resolve the resource:/// URI. Dave, thoughts?

Maybe the simplest thing is to just require you to explicitly declare such globals? There shouldn't be very many...

Comment 2

[Dave Townsend [:mossop]]

(In reply to :Gijs Kruitbosch from comment #1)
> Per
> https://dxr.mozilla.org/mozilla-central/source/testing/eslint-plugin-mozilla/
> docs/components-imports.rst, shouldn't you be making it explicit that you're
> fetching the _ symbol in that file? By default it'll only declare the
> file-name's own name.
> 
> Of course, that's redundant in a way, because now you have to define it both
> in the exports of the module and when you import. The alternative would mean
> actually loading the module you're importing, and I don't know how we would
> teach the plugin to do that, because it has no idea about how to resolve the
> resource:/// URI. Dave, thoughts?

Yeah this is really a non-starter. It would be very complex to actually get the expected symbols from the jsm as well as making eslint checks even slower. The plugin exists as a best guess that works in most cases but fails in others meaning we accidentally claim a variable exists that doesn't and don't declare variables that do. It's not ideal but it is a fast path to getting eslint working.

> Maybe the simplest thing is to just require you to explicitly declare such
> globals? There shouldn't be very many...

Yeah. In these cases just do:

/* global _ */

Comment 3

[Tom Tromey :tromey]

(In reply to :Gijs Kruitbosch from comment #1)
> Per
> https://dxr.mozilla.org/mozilla-central/source/testing/eslint-plugin-mozilla/
> docs/components-imports.rst, shouldn't you be making it explicit that you're
> fetching the _ symbol in that file? By default it'll only declare the
> file-name's own name.

Yes, that rule is doing the wrong thing here.
This use of Cu.import is perfectly ok according to

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.import


> Of course, that's redundant in a way, because now you have to define it both
> in the exports of the module and when you import. The alternative would mean
> actually loading the module you're importing, and I don't know how we would
> teach the plugin to do that, because it has no idea about how to resolve the
> resource:/// URI. Dave, thoughts?
> 
> Maybe the simplest thing is to just require you to explicitly declare such
> globals? There shouldn't be very many...

This approach means duplicating information -- mentioning the symbol in a comment
that must be kept in sync with what is really happening.  This will hide bugs.

Comment 4

[:Gijs (he/him)]

(In reply to Tom Tromey :tromey from comment #3)
> This approach means duplicating information -- mentioning the symbol in a
> comment
> that must be kept in sync with what is really happening.  This will hide
> bugs.

I don't see a way around this that doesn't involve greatly complicating the plugin, and making it much slower. Do you?

Comment 5

[Tom Tromey :tromey]

I don't think it's all that different from the other importing rules,
import-globals-from.js, import-headjs-globals.js, or import-browserjs-globals.js.

The main difficulty seems to be path mapping.  devtools at least has tried
to normalize this, so I guess there would just be a small list of odd cases.
Perhaps we could even DRY this by sharing a list with the devtools loader.

I'm not very concerned about the performance.  This also doesn't seem different
from the already existing import rules.

Comment 6

[:Gijs (he/him)]

(In reply to Tom Tromey :tromey from comment #5)
> I don't think it's all that different from the other importing rules,
> import-globals-from.js, import-headjs-globals.js, or
> import-browserjs-globals.js.

I mean, all of those are pretty specific and load small files. Modules are generally much bigger, and almost every file in the tree uses some.

> I'm not very concerned about the performance.

Based on evidence about performance, or because you think almost any performance is an acceptable trade-off for accuracy here?

For context, on my Windows machine, eslint is already causing several-second delays to saving files because of vim starting up eslint, and running it on the file. That's going to be much worse if it also has to load all the modules in use by a file...

Comment 7

[Tom Tromey :tromey]

(In reply to :Gijs Kruitbosch from comment #6)

> Based on evidence about performance, or because you think almost any
> performance is an acceptable trade-off for accuracy here?
> 
> For context, on my Windows machine, eslint is already causing several-second
> delays to saving files because of vim starting up eslint, and running it on
> the file. That's going to be much worse if it also has to load all the
> modules in use by a file...

Yes, that's much too slow.  If it were that slow here I would just turn off eslint.

We can drop this one, I'll handle it some other way.