Closed Bug 1323167 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: standard8, Unassigned)

References

Details

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.
Dave, any ideas here?
Flags: needinfo?(dtownsend)
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)
Depends on: 1325374
(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.
Through a mixture of import-browserjs-globals and environment of frame script, I think we've figure this out enough now.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.