1369722 - Use non-browser environment for jsms

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement, P2)

Description

[:Gijs (he/him)]

`window` and `document` and friends are not available in jsms, and eslint should warn about their use just like any other undefined globals.

Comment 1

[Mark Banner (:standard8)]

Also xref bug 1293298 comment 5 where we get other globals wrong in various instances.

Comment 2

[Mark Banner (:standard8)]

We should totally do it. I think the config options in eslint 4 would make this easier. Setting low priority for now until we get the upgrade.

Comment 3

[Mark Banner (:standard8)]

Bumping priority as ESLint 4 should land next week.

Comment 4

[Mark Banner (:standard8)]

The bugs I've just filed as blocking this are ones that I've found from an initial brief experimentation with ESLint 4 and the glob configuration.

Most of the rest of the failures are in relation to globals available in jsms that ESLint doesn't know about (either exposed by default/webidl or imported via Cu.importGlobalProperties) - we'll need to expand our capabilities to cope with those.

Comment 5

[Mark Banner (:standard8)]

We might need the whitelist from bug 1415483, to be able to handle globals correctly, unless the existing undefineds are all already covered by Cu.importGlobalProperties.

Comment 6

[Mark Banner (:standard8)]

I'm starting to look into this, though it might take a week or so to get something fully together.

Comment 7

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/5b02c1f5799a43dfbb4710612d90b4433bda0107
Merge pull request #3908 from Standard8/undef

Prepare for Bug 1369722 - Use non-browser environment for jsms

Comment 8

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #4)
> Most of the rest of the failures are in relation to globals available in
> jsms that ESLint doesn't know about (either exposed by default/webidl or
> imported via Cu.importGlobalProperties) - we'll need to expand our
> capabilities to cope with those.

I've been thinking about this over the last few weeks, and working on a patch. So far I've got Cu.importGlobalProperties being detected and generating globals.

I was thinking of somehow parsing the webidl globals, but I haven't had time to look at that yet. I'm also starting to think it may be better for the build system to produce a list that we can then import (since a lot seems to be generated at build time already).

For now, I've manually generated a list based on the webidl files and created a new mozilla/jsm environment. This gets the immediate improvement of better globals detection for jsm files, and we can work out how to automate it later (if possible).

Comment 9

[Mark Banner (:standard8)]

Attachment: Bug 1369722 - Disable ESLint browser environment for jsm files.

Review commit: https://reviewboard.mozilla.org/r/208486/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208486/

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8937775 [details]
Bug 1369722 - Disable ESLint browser environment for jsm files.

https://reviewboard.mozilla.org/r/208486/#review214322

Awesome!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3052b1df15ef
Disable ESLint browser environment for jsm files. r=mossop

Comment 12

[Mark Banner (:standard8)]

@Mardak: Since this is already landed in your repo, and the patch here included that fix, I went ahead and landed this. Hopefully that doesn't affect your exports too much.

Comment 13

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/3052b1df15ef

Comment 14

[Ed Lee :Mardak]

(In reply to Mark Banner (:standard8) (mainly afk until 3rd Jan) from comment #12)
> @Mardak: Since this is already landed in your repo, and the patch here
> included that fix, I went ahead and landed this. Hopefully that doesn't
> affect your exports too much.
Oh, turns out I wasn't CCed. Bug 1426203 export landed without issue though as the change here was already in m-c by the time we made the patch for export, so there was no diff to browser/extensions/activity-stream/common/PerfService.jsm to conflict. (And because your PR already merged to activity-stream, the change wasn't accidentally reverted during export.)