1168338 - DevTools ESLint should include globals from Cu.import when linting.

Status: RESOLVED DUPLICATE of bug 1203520

(DevTools :: General, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Also, I have written a rule for including Cu.imported variables such as Services, XPCOMUtils, LayoutHelpers etc.

This means that Cu.import("resource://gre/modules/devtools/LayoutHelpers.jsm"); will define LayoutHelpers in scope.

It assumes that the imported object's name matches the filename (as per the style guide):

```
This simple rule translates Cu.imported filenames e.g. XPCOMUtils.js into globals (XPCOMUtils). Although this is the standard that we use it may be possible to improve this to read the file and get the exports.

// components-imports
var escope = require("/usr/local/lib/node_modules/eslint/node_modules/escope");

module.exports = function(context) {
    return {
        ExpressionStatement: function(node) {
            var source = context.getSource(node);
            var matches = source.match(/^(?:Cu|Components\.utils)\.import\(".*\/(\w+)\.\w+"\);?$/);

            if (matches) {
                var name = matches[1];
                var scope = context.getScope();
                var variables = scope.variables;
                var variable = new escope.Variable(name, scope);

                variable.eslintExplicitGlobal = false;
                variable.writeable = true;

                variables.push(variable);
            }
        }
    };
};
```

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

FWIW, I am interested in this for use in Fennec (i.e. mobile/android).

Is this intended to land in a central location so it's easily used in other parts of the tree?

Comment 2

[Patrick Brosset <:pbro>]

Good point, I don't know what's the best place to put shared scripts like these, but we should find out. Then it'd be easy for .eslintrc files in various modules in the tree to reference those.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I should make a point of making this less DevTools specific too.

Most helper things like this and other config files go at the root of the tree but we will need permission to land them there.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I have spoken with a number of people about the file location and inside /testing/ appears to be the most agreeable.

I will tidy the code a little and then attach it.

Comment 5

[Joe Walker [:jwalker]]

Should we also consider doing the equivalent of s/Cu.import/require/g ?

Comment 6

[Joe Walker [:jwalker]]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #5)
> Should we also consider doing the equivalent of s/Cu.import/require/g ?

(To be clear where 'g' mean for all devtools code)

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #6)
> (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc)
> from comment #5)
> > Should we also consider doing the equivalent of s/Cu.import/require/g ?
> 
> (To be clear where 'g' mean for all devtools code)

Yes, this will include all types of imports.

Comment 8

[Joe Walker [:jwalker]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #7)
> (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc)
> from comment #6)
> > (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc)
> > from comment #5)
> > > Should we also consider doing the equivalent of s/Cu.import/require/g ?
> > 
> > (To be clear where 'g' mean for all devtools code)
> 
> Yes, this will include all types of imports.

So to be clear - I favour removing all instances of Cu.import from devtools code. So I'm kind of meh on us having special tooling for code that we're removing.

Maybe we should have this lint test just blow up if anyone uses Cu.import?

Or are there things that require can't do yet?

Comment 9

[Adam Moore]

Having a lint rule for disallowing this style of import sounds like a separate thing to me.  It would be better to have both.  A rule to warn about this particular form of Cu.import would actually be nice because the module could be exporting multiple symbols.

It would also be great if this handled XPCOMUtils.defineLazyModuleGetter imports; then this could be even more useful in other parts of code base, and for addon developers.

One question: how is this rule enabled?  Is there a way to add this rule in the .eslintrc so that it can be used in code editors with eslint integration?

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Adam Moore from comment #9)
> One question: how is this rule enabled?  Is there a way to add this rule in
> the .eslintrc so that it can be used in code editors with eslint integration?
I believe that's done through what ESLint calls "plugins": http://eslint.org/docs/user-guide/configuring#configuring-plugins
Once a plugin is registered, I think its rules can then be configured in the eslintrc file, just like the built-in rules.

Comment 11

[Mark Banner (:standard8)]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #10)
> (In reply to Adam Moore from comment #9)
> > One question: how is this rule enabled?  Is there a way to add this rule in
> > the .eslintrc so that it can be used in code editors with eslint integration?
> I believe that's done through what ESLint calls "plugins":
> http://eslint.org/docs/user-guide/configuring#configuring-plugins
> Once a plugin is registered, I think its rules can then be configured in the
> eslintrc file, just like the built-in rules.

For example, Loop uses the eslint-plugin-react:

http://hg.mozilla.org/mozilla-central/file/fa660238a158/browser/components/loop/.eslintrc#l4
https://github.com/yannickcr/eslint-plugin-react/

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #6)
> (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc)
> from comment #5)
> > Should we also consider doing the equivalent of s/Cu.import/require/g ?
> 
> (To be clear where 'g' mean for all devtools code)

As far as changing DevTools code to require instead of Cu.import, we have bug 996596 for this.  Not sure if it makes sense to rewrite the code base in the same bug as the new warning?  I could see it both ways, I guess.

Anyway, it does seem like this plugin would still have value for the larger browser group, since they can't easily convert to require immediately.

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #8)
> So to be clear - I favour removing all instances of Cu.import from devtools
> code. So I'm kind of meh on us having special tooling for code that we're
> removing.

True, but we don't want it creeping back into our codebase either. The point of the warnings is to help new contributors as well as to keep us from regressing.

There are also some Cu.import rules for which we still have no decent alternative e.g.
let { a, b, c } = Cu.import("...");

Of course, we should write a new lazy importer for this or adjust our current one.

Comment 14

[Joe Walker [:jwalker]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #12)
> (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc)
> from comment #6)
> > (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc)
> > from comment #5)
> > > Should we also consider doing the equivalent of s/Cu.import/require/g ?
> > 
> > (To be clear where 'g' mean for all devtools code)
> 
> As far as changing DevTools code to require instead of Cu.import, we have
> bug 996596 for this.  Not sure if it makes sense to rewrite the code base in
> the same bug as the new warning?  I could see it both ways, I guess.
> 
> Anyway, it does seem like this plugin would still have value for the larger
> browser group, since they can't easily convert to require immediately.

I'm not suggesting we should be doing the work for bug 996596 here. I'm questioning if this bug should be a devtools quarterly goal since it makes more sense to work on bug 996596 than this one.

Comment 15

[Joe Walker [:jwalker]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #13)
> (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc)
> from comment #8)
> > So to be clear - I favour removing all instances of Cu.import from devtools
> > code. So I'm kind of meh on us having special tooling for code that we're
> > removing.
> 
> True, but we don't want it creeping back into our codebase either. The point
> of the warnings is to help new contributors as well as to keep us from
> regressing.

So lets have a warning against using it, rather than one that helps people to use it?

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #15)
> So lets have a warning against using it, rather than one that helps people
> to use it?

Ah, that is a different thing. With the three rules I have written, bad imports types *are* highlighted and warned against. We still import the vars in scope otherwise we risk overwhelming people with warnings and they will get ignored.

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: include-imported-globals-1168338.patch

So, what do you need to know about this patch?

We plan on landing rules in /testing/eslint-rules/ but need to wait for feedback from Ted Mielczarek to see if he is okay with that. I am happy to take responsibility for the contents of that folder.

We no longer need the predefined globals in .eslintrc:
EventEmitter
Services
Task
XPCOMUtils

I have had to set block-scoped-var to 0 because of the way that imports works... no real surprise there.

The components-imports rule itself is very simple so no explaination needed there.

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Here are my SublimeLinter.sublime-settings:
{
    "user": {
        "debug": false,
        "delay": 0.25,
        "error_color": "FF0073",
        "gutter_theme": "Packages/SublimeLinter/gutter-themes/Danish Royalty/Danish Royalty.gutter-theme",
        "gutter_theme_excludes": [],
        "lint_mode": "load/save",
        "linters": {
            "eslint": {
                "@disable": false,
                "args": [
                    "--rulesdir=${project}/testing/eslint-rules/"
                ],
                "excludes": []
            }
        },
        "mark_style": "fill",
        "no_column_highlights_line": false,
        "passive_warnings": false,
        "paths": {
            "linux": [],
            "osx": [],
            "windows": []
        },
        "python_paths": {
            "linux": [],
            "osx": [],
            "windows": []
        },
        "rc_search_limit": 3,
        "shell_timeout": 10,
        "show_errors_on_save": false,
        "show_marks_in_minimap": true,
        "syntax_map": {
            "html (django)": "html",
            "html (rails)": "html",
            "html 5": "html",
            "php": "html",
            "python django": "python"
        },
        "warning_color": "DDB700",
        "wrap_find": true
    }
}

You will need the extra eslint args.

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

@Ted: The DevTools team are starting to use JSHint and need a place for our custom rules to live. These do things like import the globals from head.js when a mochitest-browser is open. Other teams want to share the code.

We plan on landing them eslint rules in /testing/eslint-rules/ but need to see if you are okay with that. I am happy to take responsibility for the contents of that folder.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: test.js

Comment 21

[(not currently active) Ted Mielczarek]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #19)
> @Ted: The DevTools team are starting to use JSHint and need a place for our
> custom rules to live. These do things like import the globals from head.js
> when a mochitest-browser is open. Other teams want to share the code.
> 
> We plan on landing them eslint rules in /testing/eslint-rules/ but need to
> see if you are okay with that. I am happy to take responsibility for the
> contents of that folder.

This seems fine, sorry for the delay.

Comment 22

[Patrick Brosset <:pbro>]

Mike, sorry I still haven't had the time to review your custom eslint rules patches.
I have one question before I do: isn't there a way to configure the custom rules in the /browser/devtools/.eslintrc file instead? Not everyone uses sublimelinter and that file isn't in the tree. So it's going to be really hard to get people to run those new rules if they're not configured by default.
I think that's what plugins are for: http://eslint.org/docs/user-guide/configuring#configuring-plugins
but I haven't tried that myself.

Comment 23

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: include-imported-globals-1168338.patch

Rebased... holding off on review until we have standardized how we are going to import things.

Comment 24

[Matthew N. [:MattN]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #23)
> Rebased... holding off on review until we have standardized how we are going
> to import things.

This is at least partly useful outside of Devtools, as was already pointed out, so I don't see why the devtools decision needs to block this improvement?

Comment 25

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: include-imported-globals-1168338.patch

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #24)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #23)
> > Rebased... holding off on review until we have standardized how we are going
> > to import things.
> 
> This is at least partly useful outside of Devtools, as was already pointed
> out, so I don't see why the devtools decision needs to block this
> improvement?

We are holding off landing this because we are removing a bunch of loaders. What I can do is attach the latest patch, which works perfectly.

Comment 26

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Rather than requiring people to add a rules dir we have decided to create an npm package called eslint-plugin-mozilla-devtools. This plugin will contain the following rules:

1. components-imports: Adds the filename of imported files e.g. Cu.import("some/path/Blah.jsm") adds Blah to the global scope.

2. onlylazygetters: Detects places where lazy getters should have been used but weren't.

3. import-headjs-globals: Import globals from head.js and from any files that were imported by head.js (as far as we can correctly resolve the path).

4. mark-test-function-used: Simply marks test (the test method) as used. This avoids ESHint telling us that the function is never called.

The default value for all four rules is 1 (warn).

The GitHub repo for this plugin will be:
https://github.com/mozilla/eslint-plugin-mozilla-devtools

Issues will be at:
https://github.com/mozilla/eslint-plugin-mozilla-devtools/issues

Comment 27

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Closing this in favour of bug 1203520.