Closed Bug 1330545 Opened 3 years ago Closed 3 years ago

[eslint] Handle importScripts in workers for determining global variables

Categories

(Firefox Build System :: Lint and Formatting, defect)

3 Branch
defect
Not set

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

Part of the ongoing work for no-undef.

We need to be able to handle workers which use `importScripts`. This is reasonably easy to do by extending the current global detection code.
Comment on attachment 8826099 [details]
Bug 1330545 - [eslint] Handle importScripts in workers for determining global variables.

This isn't quite polished/fully tidy yet, but I'm looking for feedback on the general approach.

The basic idea is:

- Anything with "[Ww]orker" in the filename is assumed to be a worker.
-- Maybe not 100% ideal, but probably the best we can do.
-- We probably also want some sort of comment flag to indicate files that don't include worker in the filename but are actually workers. I think there's only a handful in the tree.
- If the importScripts argument is a url, then we use modules.json.
- If the importScripts argument is a filename (no path), then we do the same as import-globals-from (if the file exists).
- There's also a few globals that workers define so I've put them in as well, although I'm thinking we might want a better place to define those.
Attachment #8826099 - Flags: review?(jaws)
Attachment #8826099 - Flags: review?(dtownsend)
Comment on attachment 8826099 [details]
Bug 1330545 - [eslint] Handle importScripts in workers for determining global variables.

and I totally meant feedback, not review.
Attachment #8826099 - Flags: review?(jaws)
Attachment #8826099 - Flags: review?(dtownsend)
Attachment #8826099 - Flags: feedback?(jaws)
Attachment #8826099 - Flags: feedback?(dtownsend)
Comment on attachment 8826099 [details]
Bug 1330545 - [eslint] Handle importScripts in workers for determining global variables.

https://reviewboard.mozilla.org/r/104136/#review105726

::: tools/lint/eslint/eslint-plugin-mozilla/lib/globals.js:122
(Diff revision 1)
> +        // Only available to workers.
> +        globals.push({name: "FileReaderSync", writable: false});
> +        globals.push({name: "onmessage", writable: false});
> +        // Only available to chrome workers, but since we don't know which is which,
> +        // we make it available anyway.
> +        globals.push({name: "ctypes", writable: false});

If I'm reading this right these globals get added only if the worker script calls importScript, and then they get added everytime it calls importScript. We really want them added at the Program node.

::: tools/lint/eslint/eslint-plugin-mozilla/lib/globals.js:138
(Diff revision 1)
>     * import-globals-from directives and also includes globals defined by
>     * standard eslint directives.
>     *
>     * @param  {String} path
>     *         The absolute path of the file to be parsed.
> +   * @return {Object}

This returns an array of those objects no?

::: tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js:191
(Diff revision 1)
> +        expr.callee.name === "importScripts") {
> +      for (var arg of expr.arguments) {
> +        var match = arg.value.match(workerImportFilenameMatch);
> +        if (match) {
> +          if (!match[1]) {
> +            results.push({file: match[2]})

Why not resolve and parse out the globals here then you can always be consistent about what is returned.
Comment on attachment 8826099 [details]
Bug 1330545 - [eslint] Handle importScripts in workers for determining global variables.

I keep wondering if we need to start trying to add features to or even fork (a little!) eslint so we can add cleaner ways to do things like this. Making the environment something that can be plugged at runtime would be a big help.
Attachment #8826099 - Flags: feedback?(dtownsend) → feedback+
Comment on attachment 8826099 [details]
Bug 1330545 - [eslint] Handle importScripts in workers for determining global variables.

I'll defer to Mossop's feedback here since he already looked at this. Thanks for your continued work on no-undef!
Attachment #8826099 - Flags: feedback?(jaws)
Comment on attachment 8826099 [details]
Bug 1330545 - [eslint] Handle importScripts in workers for determining global variables.

https://reviewboard.mozilla.org/r/104136/#review105726

> Why not resolve and parse out the globals here then you can always be consistent about what is returned.

I think we would need to do quite a bit of restructuring - the file matches need `getGlobalsForFile` in globals.js, but this is in helpers.js. So we'd either need to move most of globals.js to another file, or pass in a reference to the function.

I can do a change if you want.
Comment on attachment 8826099 [details]
Bug 1330545 - [eslint] Handle importScripts in workers for determining global variables.

https://reviewboard.mozilla.org/r/104136/#review105726

> I think we would need to do quite a bit of restructuring - the file matches need `getGlobalsForFile` in globals.js, but this is in helpers.js. So we'd either need to move most of globals.js to another file, or pass in a reference to the function.
> 
> I can do a change if you want.

getGlobalsForFile is exported from globals.js so you should be able to just require it. There is a cyclical dependency which node sort of supports but the easy way is just to do the require inside this function rather than at the top-level.
Comment on attachment 8826099 [details]
Bug 1330545 - [eslint] Handle importScripts in workers for determining global variables.

https://reviewboard.mozilla.org/r/104136/#review109710

r+ but see if you can get the getGlobalsForFile import to work to simplify what that function returns. Don't spend too much time on it though.
Attachment #8826099 - Flags: review?(dtownsend) → review+
Comment on attachment 8826099 [details]
Bug 1330545 - [eslint] Handle importScripts in workers for determining global variables.

https://reviewboard.mozilla.org/r/104136/#review105726

> getGlobalsForFile is exported from globals.js so you should be able to just require it. There is a cyclical dependency which node sort of supports but the easy way is just to do the require inside this function rather than at the top-level.

Ok, I got this to work with the require, so I've updated for that.

I also just realised, this doesn't yet fix 100% of workers - but this is an improvement, and we'll deal with the rest as we get to them.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ce2d81c539d
[eslint] Handle importScripts in workers for determining global variables. r=mossop
https://hg.mozilla.org/mozilla-central/rev/9ce2d81c539d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Product: Testing → Firefox Build System
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.