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.

Bug 1229874: Part 2 - Fix the major errors detected by ESLint. r?billm

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.

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.

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.

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.

(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. :)

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

https://hg.mozilla.org/integration/fx-team/rev/b1821f8cf26114fe25d5da0c479377379ab53b7b Bug 1229874: Part 3 - Enable ESLint in WebExtension code. r=billm

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

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

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.

(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

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? :)

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).

(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 :)