Determine a general strategy for how to deal with content code when enabling no-undef for eslint

RESOLVED WORKSFORME

Status

()

Firefox
General
RESOLVED WORKSFORME
a year ago
9 months ago

People

(Reporter: standard8, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

a year ago
Currently, enabling no-undef in browser/ gives a lot of errors (~5000) in content code. For example:

/Users/mark/dev/gecko/browser/base/content/aboutDialog.js
  66:5   error  'gAppUpdater' is not defined.  no-undef (eslint)
  66:23  error  'appUpdater' is not defined.   no-undef (eslint)

/Users/mark/dev/gecko/browser/base/content/browser-ctrlTab.js
   12:25  error  'gBrowser' is not defined.          no-undef (eslint)
   14:5   error  'gBrowser' is not defined.          no-undef (eslint)
   15:5   error  'gBrowser' is not defined.          no-undef (eslint)

In the first case of aboutDialog.js, the issue is that the xul file also imports aboutDialog-appUpdater.js into the global scope. It is hard for us to detect that.

In the browser-ctrlTab.js case, this is imported into the global window and relies on a lot of globals from browser.js etc.

Enabling the `mozilla/import-browserjs-globals` rule will probably help for a lot of the errors of the brwoser-ctrlTab.js. Although, it may lead to hiding issues in dialogs (e.g. aboutDialog) where they exist in the same directory as browser.js based code.
(Reporter)

Comment 1

a year ago
Dave, any ideas here?
Flags: needinfo?(dtownsend)

Comment 2

a year ago
Could we separate code for separate windows into separate directories, or have the plugins have per-window lists of globals, with in-tree lists of scripts per window, so it provides the right globals for the right scripts?

Really what we want here is the ability to tell eslint that scripts X, Y and Z are all evaluated in the same global, and A, B and C in some other global. We do define gBrowser and its friends in one of these files, so we shouldn't need to define it separately in some plugin. Surely eslint already has functionality for that? It's pretty common in the web world to load multiple scripts in one HTML file...
(In reply to Mark Banner (:standard8, limited time in Dec) from comment #0)
> Currently, enabling no-undef in browser/ gives a lot of errors (~5000) in
> content code. For example:
> 
> /Users/mark/dev/gecko/browser/base/content/aboutDialog.js
>   66:5   error  'gAppUpdater' is not defined.  no-undef (eslint)
>   66:23  error  'appUpdater' is not defined.   no-undef (eslint)
> 
> /Users/mark/dev/gecko/browser/base/content/browser-ctrlTab.js
>    12:25  error  'gBrowser' is not defined.          no-undef (eslint)
>    14:5   error  'gBrowser' is not defined.          no-undef (eslint)
>    15:5   error  'gBrowser' is not defined.          no-undef (eslint)
> 
> In the first case of aboutDialog.js, the issue is that the xul file also
> imports aboutDialog-appUpdater.js into the global scope. It is hard for us
> to detect that.

How common is this case? We have a way to solve this, assuming it is a simple dependency, with the "import-globals-from" eslint directive.

> In the browser-ctrlTab.js case, this is imported into the global window and
> relies on a lot of globals from browser.js etc.
> 
> Enabling the `mozilla/import-browserjs-globals` rule will probably help for
> a lot of the errors of the brwoser-ctrlTab.js. Although, it may lead to
> hiding issues in dialogs (e.g. aboutDialog) where they exist in the same
> directory as browser.js based code.

So the import-browserjs-globals plugin knows which files are in browser.xul, they're listed in the plugin itself so it should be possible for it to only inject the globals for those files (plus the test files it already does it for).
Flags: needinfo?(dtownsend)
(Reporter)

Updated

11 months ago
Depends on: 1325374
(Reporter)

Comment 4

11 months ago
(In reply to Dave Townsend [:mossop] from comment #3)
> (In reply to Mark Banner (:standard8, limited time in Dec) from comment #0)
> > In the first case of aboutDialog.js, the issue is that the xul file also
> > imports aboutDialog-appUpdater.js into the global scope. It is hard for us
> > to detect that.
> 
> How common is this case? We have a way to solve this, assuming it is a
> simple dependency, with the "import-globals-from" eslint directive.

Not sure, quite frequently though, there's multiple files in that global scope.

> > Enabling the `mozilla/import-browserjs-globals` rule will probably help for
> > a lot of the errors of the brwoser-ctrlTab.js. Although, it may lead to
> > hiding issues in dialogs (e.g. aboutDialog) where they exist in the same
> > directory as browser.js based code.
> 
> So the import-browserjs-globals plugin knows which files are in browser.xul,
> they're listed in the plugin itself so it should be possible for it to only
> inject the globals for those files (plus the test files it already does it
> for).

I think there's something wrong with the import-browserjs-globals plugin, though I haven't quite worked out exactly what yet. In working towards the patches on bug 1325374, I think I found some cases where Services should have been seen as a global, but wasn't. I'll try investigating that soon.
(Reporter)

Comment 5

9 months ago
Through a mixture of import-browserjs-globals and environment of frame script, I think we've figure this out enough now.
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.