1170804 - Add .eslintrc files for /mobile JS files

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[:Margaret Leibovic]

So that we can use ESLint in IJ.

Comment 1

[:Margaret Leibovic]

See this blog post for inspiration about how the hello team did this:
https://blog.mozilla.org/standard8/2015/05/13/using-eslint-alongside-the-firefox-hello-code-base-to-help-productivity/

Also, directions here for turning on ESLint in IJ:
https://www.jetbrains.com/webstorm/help/using-javascript-code-quality-tools.html#installESLint

Comment 2

[Nick Alexander :nalexander [he/him]]

For the record:

17:14 nalexander: margaret: it's important to install npm globally (brew install npm) and then |npm install -g eslint|.  Otherwise, you get a copy in $OBJDIR, which will disappear eventually.

It looks like devtools and hello have configurations we might want to take from.

Comment 3

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

Attachment: MozReview Request: Bug 1170804 - Add .eslintrc configuration file to mobile/android. r=margaret

Bug 1170804 - Add .eslintrc configuration file to components/. r=margaret

Comment 4

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

Attachment: MozReview Request: Bug 1170804 - Fix bugs found by eslint in components/. r=margaret

Bug 1170804 - Fix bugs found by eslint in components/. r=margaret

A few errors still remain.

Comment 5

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

The remaining errors are:

HelperAppDialog.js
  213:13  error  "ContentAreaUtils" is not defined  no-undef

Is this inherited from browser.js?

PromptService.js
  399:60  error  "aText" is not defined  no-undef
  401:49  error  "aText" is not defined  no-undef

I imagine aText is supposed to be in the method header, but I can't find the idl definition.

Snippets.js
  256:24  error  Unexpected identifier

I don't think it likes yield, but HelperAppDialog didn't have any issues.

Any ideas, Margaret?

Comment 6

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

Comment on attachment 8626030 [details]
MozReview Request: Bug 1170804 - Add .eslintrc configuration file to mobile/android. r=margaret

Bug 1170804 - Add .eslintrc configuration file to mobile/android. r=margaret

eslint can be run by `cd`ing to mobile/android and running `eslint .` (assuming
eslint is installed). Note that .jsm files have not yet been configured.

Add an eslintignore to avoid the files that will take more work.

Comment 7

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

Comment on attachment 8626031 [details]
MozReview Request: Bug 1170804 - Fix bugs found by eslint in components/. r=margaret

Bug 1170804 - Fix bugs found by eslint in components/. r=margaret

A few errors still remain.

Comment 8

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

Margaret, one more:

modules/WebappManagerWorker.js
  13:0  error  "onmessage" is not defined  no-undef

Should this just be let onmessage = ? Where is onmessage used?

Comment 9

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

Attachment: MozReview Request: Bug 1170804 - Get .jsm to work under eslint. r=margaret

Bug 1170804 - Get .jsm to work under eslint. r=margaret

You can now run by `cd mobile/android && eslint . --ext "[.js,.jsm]"`.

Comment 10

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

And for the jsms:

modules/HelperApps.jsm
  183:17  error  "ContentAreaUtils" is not defined  no-undef

Again, from browser.js?

modules/HomeProvider.jsm
  208:10  error  Illegal yield expression

It just doesn't like generators.

modules/MatchstickApp.jsm
  245:16  error  Unexpected token if

A new expression type, probably. Not sure what to do here.

modules/SharedPreferences.jsm
  64:37  error  "level" is not defined  no-undef

This actually seems undefined.

modules/WebappManager.jsm
  54:67  error  Unexpected token for

Another new syntax that it doesn't seem to recognize.

Comment 11

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

Attachment: MozReview Request: Bug 1170804 - Set non-final eslintrc for chrome/content. r=margaret

Bug 1170804 - Set non-final eslintrc for chrome/content. r=margaret

Still a lot of errors but it's progress.

Comment 12

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

Attachment: MozReview Request: Bug 1170804 - Add non-final eslintrc for tests/. r=margaret

Bug 1170804 - Add non-final eslintrc for tests/. r=margaret

There are still some failures but it's a start.

Comment 13

[:Margaret Leibovic]

Comment on attachment 8626030 [details]
MozReview Request: Bug 1170804 - Add .eslintrc configuration file to mobile/android. r=margaret

https://reviewboard.mozilla.org/r/11977/#review10507

Ship It!

Comment 14

[:Margaret Leibovic]

Comment on attachment 8626031 [details]
MozReview Request: Bug 1170804 - Fix bugs found by eslint in components/. r=margaret

https://reviewboard.mozilla.org/r/11979/#review10505

::: mobile/android/components/PromptService.js:377
(Diff revision 2)
> -    return nsIAuthPrompt_loginPrompt(aTitle, aText, aPasswordRealm, aSavePassword, aUser, aPass);
> +    return this.nsIAuthPrompt_loginPrompt(aTitle, aText, aPasswordRealm, aSavePassword, aUser, aPass);

/me facepalm

::: mobile/android/components/BlocklistPrompt.js:51
(Diff revision 2)
> -        addonList[i].item.disabled = true;
> +        aAddons[i].item.disabled = true;

/me facepalm

Comment 15

[:Margaret Leibovic]

Comment on attachment 8626052 [details]
MozReview Request: Bug 1170804 - Get .jsm to work under eslint. r=margaret

https://reviewboard.mozilla.org/r/11983/#review10509

::: mobile/android/modules/Notifications.jsm:22
(Diff revision 1)
> -  this._when = (new Date).getTime();
> +  this._when = (new Date()).getTime();

This actually does work, but yeah, better to be explicit here :)

Comment 16

[:Margaret Leibovic]

Comment on attachment 8626200 [details]
MozReview Request: Bug 1170804 - Set non-final eslintrc for chrome/content. r=margaret

https://reviewboard.mozilla.org/r/11989/#review10511

Ship It!

Comment 17

[:Margaret Leibovic]

Comment on attachment 8626202 [details]
MozReview Request: Bug 1170804 - Add non-final eslintrc for tests/. r=margaret

https://reviewboard.mozilla.org/r/11991/#review10513

Ship It!

Comment 18

[:Margaret Leibovic]

Comment on attachment 8626031 [details]
MozReview Request: Bug 1170804 - Fix bugs found by eslint in components/. r=margaret

https://reviewboard.mozilla.org/r/11979/#review10517

Ship It!

Comment 19

[:Margaret Leibovic]

Comment on attachment 8626052 [details]
MozReview Request: Bug 1170804 - Get .jsm to work under eslint. r=margaret

https://reviewboard.mozilla.org/r/11983/#review10519

Ship It!

Comment 20

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #5)
> The remaining errors are:
> 
> HelperAppDialog.js
>   213:13  error  "ContentAreaUtils" is not defined  no-undef
> 
> Is this inherited from browser.js?

Yeah, looks like this was probably copy/pasted from browser.js, where we do define this:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#374

But looking how HelperAppDialog.js is loaded, I don't think it loads in the global window context, so this is probably busted... we should file another bug about this to look into whether or not this logic works. Probably we just need to add this lazy getter definition.

> PromptService.js
>   399:60  error  "aText" is not defined  no-undef
>   401:49  error  "aText" is not defined  no-undef
> 
> I imagine aText is supposed to be in the method header, but I can't find the
> idl definition.

Looks like text should be a parameter here...
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIAuthPrompt.idl#31

wesj, is there just a typo in here that we're missing an aText parameter?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/PromptService.js#384

> Snippets.js
>   256:24  error  Unexpected identifier
> 
> I don't think it likes yield, but HelperAppDialog didn't have any issues.
> 
> Any ideas, Margaret?

Maybe because gStatsPath is a global defined in a lazy getter?
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/Snippets.js#59

Comment 21

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

https://hg.mozilla.org/integration/fx-team/rev/14f8333949e8
https://hg.mozilla.org/integration/fx-team/rev/7c445f47c4d6
https://hg.mozilla.org/integration/fx-team/rev/de00e0badee1
https://hg.mozilla.org/integration/fx-team/rev/a348a4336c71
https://hg.mozilla.org/integration/fx-team/rev/c8b9161a17df

Comment 22

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

(Moving request to bug 1177774)

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14f8333949e8
https://hg.mozilla.org/mozilla-central/rev/7c445f47c4d6
https://hg.mozilla.org/mozilla-central/rev/de00e0badee1
https://hg.mozilla.org/mozilla-central/rev/a348a4336c71
https://hg.mozilla.org/mozilla-central/rev/c8b9161a17df