Support ESLint in WebExtension code

RESOLVED FIXED in Firefox 45

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: kmag, Assigned: kmag)

Tracking

unspecified
mozilla45
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

4 years ago
No description provided.
Assignee

Comment 1

4 years ago
Bug 1229874: Fix components-import eslint helper. r?mratcliffe

This has the side-effect of causing these imports to trigger unused variable
warnings if they're unused, but I think that's actually desirable.
Attachment #8695057 - Flags: review?(mratcliffe)
Assignee

Comment 2

4 years ago
Bug 1229874: Part 2 - Fix the major errors detected by ESLint. r?billm
Attachment #8695058 - Flags: review?(wmccloskey)
Assignee

Comment 3

4 years ago
Bug 1229874: Part 3 - Enable ESLint in WebExtension code. r?billm

The base .eslintrc is essentially a merge of the root Toolkit .eslintrc and
the devtools .eslintrc, with some minor changes to match our prevalent style.

For the most enforces the coding styles that we've been using most
consistently. There are a couple of significant differences, though:

 * The rule for opening brace alignment can only be applied globally, and
   doesn't make exceptions for top-level functions. I chose to turn it on, and
   change the brace style of existing top-level functions that violated it,
   since the rule seemed worth using, and that's the direction most Toolkit JS
   code has been headed anyway.

 * The rule for switch/case statements requires an added indentation level for
   case statements. Most of our switch statements did not use an extra level
   of indentation, and I initially wrote the rule to enforce that style, until
   I came across case statements that used blocks, and required the extra
   indentation level for sanity.
Attachment #8695059 - Flags: review?(wmccloskey)
Assignee

Comment 4

4 years ago
There are a lot of changes here, but most of them are trivial things, like missing semicolons or commas.

I'm not really attached to most of the rules. I just kept the ones that seemed reasonable and matched our current style, so feel free to veto any of them.

The ones that seemed to be particularly useful, though, were:

  - mozilla/balanced-listeners: Found one new error, and would have found several other errors we've noticed over the past few weeks.
  - no-undef: Found a couple of errors, one of which was responsible for bug 1229552.
  - no-unused-vars: Caught a fair amount of dead code.
  - comma-dangle: Found a lot of missing commas, which tend to cause errors.
  - strict: Most of our files were not running in strict mode.
Attachment #8695058 - Flags: review?(wmccloskey) → review+
Comment on attachment 8695058 [details]
MozReview Request: Bug 1229874: Part 2 - Fix the major errors detected by ESLint. r?billm

https://reviewboard.mozilla.org/r/26985/#review24395

::: toolkit/components/extensions/Extension.jsm:561
(Diff revision 1)
> -                           JSON.stringify(`_locales/${default_locale}/`));
> +                           JSON.stringify(`_locales/${this.manifest.default_locale}/`));

Did eslint catch this? That would be impressive.
Assignee

Comment 6

4 years ago
https://reviewboard.mozilla.org/r/26985/#review24395

> Did eslint catch this? That would be impressive.

It did. I was surprised.
Comment on attachment 8695059 [details]
MozReview Request: Bug 1229874: Part 3 - Enable ESLint in WebExtension code. r?billm

https://reviewboard.mozilla.org/r/26987/#review24511

Thanks Kris! This is fantastic stuff.

::: browser/components/extensions/test/browser/browser_ext_browserAction_context.js:17
(Diff revision 1)
> -      var details = [
> +      let details = [

I'm surprised this works. I thought we weren't able to use let in extension code.
Attachment #8695059 - Flags: review?(wmccloskey) → review+
Assignee

Comment 8

4 years ago
(In reply to Bill McCloskey (:billm) from comment #7)
> browser/components/extensions/test/browser/browser_ext_browserAction_context.
> js:17
> (Diff revision 1)
> > -      var details = [
> > +      let details = [
> 
> I'm surprised this works. I thought we weren't able to use let in extension
> code.

Shu enabled it globally in bug 932517 just before the 44 merge (without waiting for all the dependencies), so we can now. :)
Assignee

Comment 9

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/a618b79155185a63ef94710be7d80c4f571e92bb
Bug 1229874: Part 2 - Fix the major errors detected by ESLint. r=billm
Assignee

Updated

4 years ago
Keywords: leave-open
Assignee

Comment 12

4 years ago
Landing part 3 with workarounds for part 1, so it doesn't bit rot. It looks like the main issues with the components-import rule have already been fixed in central, but it still doesn't support |defineLazyServiceGetter|.
Comment on attachment 8695057 [details]
MozReview Request: Bug 1229874: Fix components-import eslint helper. r?mratcliffe

https://reviewboard.mozilla.org/r/26983/#review24717
Attachment #8695057 - Flags: review?(mratcliffe) → review+
Assignee

Comment 15

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/d8ce27c85590380ef025bb4ed66e564a4dff9bff
Bug 1229874: Support defineLazyServiceGetter in components-import eslint helper. r=miker
Assignee

Updated

4 years ago
Keywords: leave-open

Comment 16

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d8ce27c85590
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
I don't know if it's my eslint version (I use the v2 beta) but the current .eslintrc in toolkit/component/extensions does not seem to work properly. Here are the problems I see:

1. It has the same rules several times.
2. It references eslint-mozilla-plugin and it's not obvious that I have to go to the project root and run "npm install testing/eslint-mozilla-plugin".
3. having "extensions/**" in .eslintignore seems to apply to toolkit/component/extensions. Replacing with "/extensions/**" seems to fix it for me.

Moreover I think the "extends" line is useless as eslint (contrary to others like jshint) should merge .eslintrc in the parent directories automatically.

I have a patch for 1 and 3 if you're interested, I can file a separate bug for this. But I have no idea how to do 2.
Assignee

Comment 18

4 years ago
(In reply to Julien Wajsberg [:julienw] from comment #17)
> 2. It references eslint-mozilla-plugin and it's not obvious that I have to
> go to the project root and run "npm install testing/eslint-mozilla-plugin".

The entire tree requires that plugin, not just WebExtension code.

The setup process is documented here:
https://wiki.mozilla.org/WebExtensions/Hacking#Checking_your_code_with_ESLint

I'm sorry, but I don't know what we can do to make it clearer. Perhaps a
comment in the RC files wouldn't hurt.

> 3. having "extensions/**" in .eslintignore seems to apply to
> toolkit/component/extensions. Replacing with "/extensions/**" seems to fix
> it for me.

This doesn't seem to be a problem for me. If this needs to be changed, though,
it should be changed for the other ignore lines as well. Please file a
separate bug.

> Moreover I think the "extends" line is useless as eslint (contrary to others
> like jshint) should merge .eslintrc in the parent directories automatically.

The "extends" line is there because we include the file from other directories
that aren't children of toolkit/

> I have a patch for 1 and 3 if you're interested, I can file a separate bug
> for this. But I have no idea how to do 2.

I'd be interested in a patch for #1, but you'll need to file another bug.

#3 will have to be a separate bug too, in a different component, since it applies to the whole tree.

Thanks.
#3 seems to actually be a bug to eslint, see [1].

So I'll file a separate bug for #1.

[1] https://github.com/eslint/eslint/issues/5087#issuecomment-176288977
Assignee

Comment 20

3 years ago
Good to know. Thanks for looking into that.
Julien, I just stumbled upon problems #1 and #3 as well. It would be cool to get rid of the duplicate rules eventually. Are you still working on a patch? :)
Flags: needinfo?(felash)
Depends on: 1248523
Yes sorry, I already had it and just needed to make it nice. I just uploaded it in bug 1248523 and requested review.

#3 should be fixed in latest v2.0 release of eslint (I haven't had the chance to check though).
Flags: needinfo?(felash)
(In reply to Julien Wajsberg [:julienw] from comment #22)

> #3 should be fixed in latest v2.0 release of eslint (I haven't had the
> chance to check though).

yes it is :)

Updated

Last year
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.