chore(lint): switch from JSHint to ESLint #2578

TDA · 2015-06-10T22:25:36Z

Fixes #2550
Switch from JSHint to ESLint
Also have JSHint in parallel
References #2558, runs into same problem
@zaach

TDA · 2015-06-10T22:26:43Z

The build should fail, seeing as this has the same problem in a file called run_locally.js , should I once again change 'const' to 'var' in that file?
@pdehaan
@vladikoff
@zaach
@shane-tomlinson

vladikoff · 2015-06-10T22:29:59Z

app/.eslintrc

+    "valid-typeof": 2,
+    "wrap-iife": 0
+  }
+}


need new line at the end of file here

vladikoff · 2015-06-10T22:32:53Z

package.json

@@ -82,6 +82,7 @@
    "grunt-contrib-watch": "0.6.1",
    "grunt-conventional-changelog": "1.1.0",
    "grunt-copyright": "0.1.0",
+    "grunt-eslint": "^14.0.0",


lock this to 14.0.0 or highest released version , don't use ^

vladikoff · 2015-06-10T22:34:37Z

scripts/run_locally.js

@@ -21,7 +22,7 @@ module.exports = function (done) {
    if (done) {
      done(code);
    } else {
-      process.exit(code);
+      process.exit(code); //eslint-disable-line


what is the warning here?

vladikoff · 2015-06-10T22:35:22Z

app/.eslintrc

+    "mocha": true,
+    "browser": true,
+    "builtin": true
+  },


does eslint support extending config, so we don't have to repeat this in two files? cc @pdehaan

It does. By default too, if I recall correctly. So we'd probably only need to override whatever envs we need and any special rule overrides.

For example, since we're in "./app/", I think we should be setting node: false and es6: false.

nit: ~~Plus maybe convert this to YAML too~~ and A-Z sort the envs above.

I think the "app/.eslintrc" can be reduced to:

{ "env": { "browser": true, "jquery": true, "mocha": true, "node": false }, "rules": { "globals": { "router": false } } }

With the rest of the rules being inherited from the parent .eslintrc config. (see https://github.com/mozilla/fxa-content-server/pull/2578/files#r32183274)

Will check this out :)

Setting Node: false actually throws 21 errors of "no-console" in the app/ directory.. What should I do?
@pdehaan

I added "no-console": 0, to the .eslintrc file.
https://github.com/mozilla/fxa-content-server/pull/2578/files#r32183274

Setting no-console: 0 to remedy this.

vladikoff · 2015-06-10T22:36:12Z

@TDA I think @shane-tomlinson confirmed in irc that we should use var. does that answer your question?

TDA · 2015-06-10T23:16:59Z

@vladikoff: Was just confirming :) I guess it passes the tests now.

vladikoff · 2015-06-10T23:25:56Z

grunttasks/eslint.js

+'use strict';
+
+module.exports = function (grunt) {
+  grunt.config('eslint', {


@TDA we need to run this task in https://github.com/mozilla/fxa-content-server/blob/master/grunttasks/lint.js#L10

Ok, done, but guess that results in a merge conflict?
@vladikoff

@TDA yea needs to be rebased against 'master' , we just removed concurrent

Should be right now @vladikoff :)

vladikoff · 2015-06-10T23:34:27Z

grunttasks/lint.js

@@ -7,6 +7,10 @@

 module.exports = function (grunt) {
  grunt.registerTask('lint', 'lint all the things', [
-    'concurrent:lint'
+  	'eslint',


bad spacing / bad indent

vladikoff · 2015-06-10T23:42:55Z

app/tests/spec/views/base.js
  43:8  warning  relier is defined but never used  no-unused-vars
app/tests/spec/views/complete_account_unlock.js
  39:8  warning  account is defined but never used  no-unused-vars
app/tests/spec/views/complete_reset_password.js
  318:12  warning  account is defined but never used  no-unused-vars
app/tests/spec/views/mixins/loading-mixin.js
  28:8  warning  view is defined but never used  no-unused-vars
app/tests/spec/views/permissions.js
  30:8  warning  email is defined but never used  no-unused-vars
app/tests/spec/views/settings.js
  33:8  warning  windowMock is defined but never used  no-unused-vars
✖ 6 problems (0 errors, 6 warnings)
Running "eslint:tests" (eslint) task
tests/functional/oauth_iframe.js
  33:6  warning  user is defined but never used  no-unused-vars
tests/functional/oauth_preverified_sign_up.js
  19:6  warning  user is defined but never used  no-unused-vars
tests/functional/oauth_reset_password.js
  23:6  warning  user is defined but never used         no-unused-vars
  26:6  warning  accountData is defined but never used  no-unused-vars
tests/functional/oauth_sign_up.js
  24:6  warning  user is defined but never used  no-unused-vars
tests/functional/oauth_sync_sign_in.js
  25:6  warning  user is defined but never used  no-unused-vars
tests/functional/oauth_webchannel.js
  23:6  warning  user is defined but never used  no-unused-vars
tests/functional/oauth_webchannel_keys.js
  25:6  warning  user is defined but never used  no-unused-vars
tests/functional/sign_in.js
  20:6  warning  user is defined but never used  no-unused-vars
tests/functional/sign_in_cached.js
  38:6  warning  user is defined but never used   no-unused-vars
  39:6  warning  user2 is defined but never used  no-unused-vars
tests/functional/sync_reset_password.js
  27:6  warning  accountData is defined but never used  no-unused-vars
tests/functional/sync_sign_in.js
  24:6  warning  user is defined but never used  no-unused-vars
tests/functional/sync_v2_sign_up.js
  26:6  warning  client is defined but never used  no-unused-vars
✖ 14 problems (0 errors, 14 warnings)

TDA · 2015-06-10T23:47:30Z

@vladikoff So i remove those unused variables? or change the lint rule?

vladikoff · 2015-06-10T23:55:07Z

@TDA something is not correct with the linter, the first warning:

    var relier;
    var user;
    var screenName = 'screen';

    var View = BaseView.extend({
      template: Template
    });


    beforeEach(function () {
      translator = new Translator('en-US', ['en-US']);
      translator.set({
        'the error message': 'a translated error message',
        'the success message': 'a translated success message'
      });

      router = new RouterMock();
      windowMock = new WindowMock();
      ephemeralMessages = new EphemeralMessages();
      metrics = new Metrics();
      relier = new Relier();

Relier is defined below, we need to figure out why it thinks that it is defined but never used

app/tests/spec/views/base.js
  43:8  warning  relier is defined but never used  no-unused-vars

TDA · 2015-06-11T00:00:05Z

@vladikoff Well, it is defined but never used. I don't see the variable relier being used in any statement other than the definition statement.

vladikoff · 2015-06-11T00:06:40Z

@TDA , see #2578 (comment) last line

relier = new Relier();

zaach · 2015-06-11T00:12:30Z

@TDA @vladikoff I don't think an assignment counts as being used– the value has to be accessed at some point.

pdehaan · 2015-06-11T00:13:25Z

.eslintrc

+    "eqeqeq": [2, "allow-null"],
+    "global-strict": [0, "never"],
+    "globals": {
+      "define": true,


IIRC, we don't need to add define as a global here if we have the "amd" env.

pdehaan · 2015-06-11T00:14:03Z

.eslintrc

+    "node": true,
+    "mocha": true,
+    "browser": true,
+    "builtin": true


nit: Can we sort these A-Z.

Yes, will do! :)

pdehaan · 2015-06-11T00:15:10Z

.eslintrc

+    "no-spaced-func": 0,
+    "no-undef": 2,
+    "no-underscore-dangle": 0,
+    "no-unused-vars": [1, {vars: all, args: none}],


Not sure if these are strictly valid JSON, or if they need quotes.
~~Alternatively, we could (should?) convert this file from JSON to YAML because YAML is super awesome.~~

I will, once I figure out what valid YAML looks like :P :D But we did have the jshint ones in JSON didn't we?

I'd say let's keep it JSON for consistency's sake. No need for yet another markup language at this point. ;)

@TDA: Please add .eslintrc (and .jscsrc while we're in there) to the /grunttasks/jsonlint.js:11 file.

Ok will do :)

pdehaan · 2015-06-11T00:19:58Z

app/scripts/lib/storage.js

@@ -24,7 +24,7 @@ define([
    var item;
    try {
      item = JSON.parse(this._backend.getItem(fullKey(key)));
-    } catch (e) {
+    } catch (e) {//eslint-disable-line no-empty


nit: Add a space between { and the //eslint comment:

} catch (e) { //eslint-disable-line no-empty

Is there a lint rule for this? ;)

Yeah, I'm actually a bit surprised that JSCS wouldn't have complained.

JSCS doesn't seem to complain about anything to be honest, but I guess that might be cause we haven't set "requireSpacesInsideObjectBrackets" to true.

pdehaan · 2015-06-11T00:23:17Z

app/tests/spec/lib/xss.js

@@ -39,7 +39,9 @@ function (chai, _, XSS, Constants) {
      });

      it('disallows javascript: href', function () {
-        expectEmpty('javascript:alert(1)'); // jshint ignore:line
+        /*eslint-disable */


What about using // eslint-disable-line on line 43?

That actually didn't work, it made either the jshint tests fail or the eslint tests fail. So I went for the block disable one.

pdehaan · 2015-06-11T00:25:30Z

scripts/run_locally.js



-const BIN_ROOT = path.join(__dirname, '..', 'server', 'bin');
+var BIN_ROOT = path.join(__dirname, '..', 'server', 'bin');


Curious, why couldn't these be const if we were using the es6 env in .eslintrc? I thought that worked (but I admittedly tested very quickly and have a very poor memory)

Because the linter is ok with the const, but our node interpreter throws an error. Can't figure out why.

pdehaan · 2015-06-11T00:26:04Z

server/bin/fxa-content-server.js

@@ -20,7 +20,6 @@ if (isMain) {
  process.chdir(path.dirname(__dirname));
 }

-var config = require('../lib/configuration');


Wait, why was this removed?

redundant require. See line 12 above.

what @jrgm said.

Wat. Good thing the config is idempotent.

vladikoff · 2015-06-12T15:36:53Z

tests/functional/sync_sign_in.js

@@ -41,7 +40,8 @@ define([

    beforeEach: function () {
      email = TestHelpers.createEmail();
-      user = TestHelpers.emailToUser(email);
+      // removed variable user


remove this comment

vladikoff · 2015-06-12T15:37:42Z

tests/functional/sync_sign_in.js

@@ -41,7 +40,8 @@ define([

    beforeEach: function () {
      email = TestHelpers.createEmail();
-      user = TestHelpers.emailToUser(email);
+      // removed variable user
+      TestHelpers.emailToUser(email);


this is not used ?

vladikoff · 2015-06-12T15:38:19Z

tests/functional/sign_in_cached.js

      email2 = TestHelpers.createEmail();
-      user2 = TestHelpers.emailToUser(email2);
+      // removed variable user2


remove comment, also above.

vladikoff · 2015-06-12T15:38:24Z

tests/functional/sign_in_cached.js

      email2 = TestHelpers.createEmail();
-      user2 = TestHelpers.emailToUser(email2);
+      // removed variable user2
+      TestHelpers.emailToUser(email2);


is this used?

You are right, again :D removed :)

vladikoff · 2015-06-12T15:38:55Z

tests/functional/oauth_reset_password.js

@@ -39,12 +37,13 @@ define([

    beforeEach: function () {
      email = TestHelpers.createEmail();
-      user = TestHelpers.emailToUser(email);
+      // removed variable user


see other notes about these

vladikoff · 2015-06-12T15:39:03Z

tests/functional/oauth_sign_up.js

@@ -32,7 +31,8 @@ define([
    beforeEach: function () {
      email = TestHelpers.createEmail();
      bouncedEmail = TestHelpers.createEmail();
-      user = TestHelpers.emailToUser(email);
+      // removed variable user


see other notes about these

TDA · 2015-06-12T16:42:50Z

@vladikoff @pdehaan Removed the comments, and the unnecessary statements, switched EMAIL to email. Passes tests locally. :)

pdehaan · 2015-06-12T16:53:26Z

@TDA:

Running "eslint:tests" (eslint) task
tests/functional/oauth_webchannel.js
  61:6  error  Trailing spaces not allowed  no-trailing-spaces
tests/functional/oauth_webchannel_keys.js
  67:6  error  Trailing spaces not allowed  no-trailing-spaces
✖ 2 problems (2 errors, 0 warnings)
Warning: Task "eslint:tests" failed.� Use --force to continue.
Aborted due to warnings.

Those were the first/only errors I saw in the Travis logs. After fixing them you'll want to make sure you run $ grunt lint locally to make sure there weren't others.

TDA · 2015-06-12T16:56:01Z

@pdehaan That was actually my stupid mistake. I have this habit of saving stuff before closing them, and sublimetext 3 has its weird way of autopushing to git when I save. And I didn't realize I had accidentally hit 'space' when pressing 'cmd' key. Oh well, fixed now.

zaach · 2015-06-12T18:01:41Z

app/tests/spec/lib/xhr.js

@@ -12,7 +11,7 @@ define([
  'lib/xhr',
  'lib/promise'
 ],
-function (chai, sinon, $, _, Xhr, p, undefined) {
+function (chai, sinon, $, _, Xhr, p, undefined) { //eslint-disable-line no-shadow-restricted-names



This convention of declaring undefined seems unnecessary. Let's just ditch it.

zaach · 2015-06-12T18:05:26Z

tests/functional/sync_v2_sign_up.js

@@ -23,7 +23,7 @@ define([
  var TOO_YOUNG_YEAR = new Date().getFullYear() - 13;
  var OLD_ENOUGH_YEAR = TOO_YOUNG_YEAR - 1;

-  var client;
+  var client; //eslint-disable-line no-unused-vars


client doesn't seem to be used. Can we remove it and the lines here: https://github.com/mozilla/fxa-content-server/pull/2578/files#diff-420999c0bd8fd929634bff4b02e76788R67

We also no longer need the FxaClient module, so it can be removed from the module list: https://github.com/mozilla/fxa-content-server/pull/2578/files#diff-420999c0bd8fd929634bff4b02e76788R12, https://github.com/mozilla/fxa-content-server/pull/2578/files#diff-420999c0bd8fd929634bff4b02e76788R16

Ok, cool, this needed the XHR stuff to be removed as well.

zaach · 2015-06-12T18:08:46Z

app/tests/spec/views/base.js

@@ -40,7 +38,7 @@ function (chai, $, sinon, BaseView, p, Translator, EphemeralMessages, Metrics,
    var translator;
    var metrics;
    var fxaClient;
-    var relier;
+    var relier; //eslint-disable-line no-unused-vars


relier is not used in this file, so we can remove it and remove Relier from the module list so that amdcheck passes.

Done, and I found why I couldn't do this yesterday. So if we remove the relier dependency, we need to remove the formal parameter Relier as well for the tests to pass :D I missed that yesterday, which is why the "firefox platform" error was thrown :D

zaach · 2015-06-12T18:11:46Z

app/tests/spec/views/complete_account_unlock.js

@@ -36,7 +34,7 @@ function (chai, sinon, p, View, AuthErrors, Metrics, Constants, FxaClient,
    var relier;
    var broker;
    var user;
-    var account;
+    var account; //eslint-disable-line no-unused-vars


account is not used in this file, so we can remove all references to it.

Okie, this one was straightforward.

zaach · 2015-06-12T18:20:08Z

app/tests/spec/views/complete_reset_password.js

@@ -273,7 +271,7 @@ function (chai, sinon, p, AuthErrors, Metrics, FxaClient, InterTabChannel,
      });

      it('non-direct-access signs the user in and redirects to `/reset_password_complete` if broker does not say halt', function () {
-        var account;
+        var account; //eslint-disable-line no-unused-vars


This one is kind of legit– we need account so that we can make the assertion here. But we can refactor this to make it better!

We can remove account and it's usage here and just return newAccount directly. Then, for the test here we can use some of Sinon's reflection capabilities and do:

assert.isTrue(broker.afterCompleteResetPassword.calledWith(user.setSignedInAccount.returnValues[0]));

Changing the assertions to this results in the following assertionerrors being thrown:

@zaach

Oh right, it returns a Promise. You'll have to put something like this as the last assertion in the block instead:

return user.setSignedInAccount.returnValues[0].then(function (returnValue) { assert.isTrue(broker.afterCompleteResetPassword.calledWith(returnValue)); });

The return statement ensures that if the assertion fails and causes the promise to throw, the test will fail.

Awesome! It runs now :) Just pushed.

P.s: Sinon is an anime character 💃

zaach · 2015-06-12T18:20:48Z

app/tests/spec/views/complete_reset_password.js

@@ -317,7 +315,7 @@ function (chai, sinon, p, AuthErrors, Metrics, FxaClient, InterTabChannel,
      });

      it('direct access signs the user in and redirects to `/settings` if broker does not say halt', function () {
-        var account;
+        var account; //eslint-disable-line no-unused-vars


Same as before

zaach · 2015-06-12T18:21:10Z

app/tests/spec/views/mixins/loading-mixin.js

@@ -33,7 +31,7 @@ define([

      windowMock = new WindowMock();

-      view = new View({
+      void new View({


zaach · 2015-06-12T18:23:10Z

app/tests/spec/views/settings.js

@@ -30,7 +28,7 @@ function (chai, $, sinon, View, RouterMock, WindowMock, TestHelpers,
  describe('views/settings', function () {
    var view;
    var routerMock;
-    var windowMock;
+    var windowMock; //eslint-disable-line no-unused-vars


windowMock isn't used in this file, so let's remove it and the WindowMock module so that amdcheck passes.

zaach · 2015-06-12T18:34:40Z

I made some more comments. I think those should be the last things we have to fix.

239 files changed! Wow. It's like our codebase hadn't brushed its teeth in 2 years and finally went to the dentist.

TDA · 2015-06-12T18:47:06Z

Wow @zaach , ok let me fix these up :)

Fixes #2550 Switch from JSHint to ESLint Add rules to ESLint Also have JSHint in parallel Remove old dependencies reported by amdcheck Remove unused variables reported by ESLint

TDA · 2015-06-12T21:20:53Z

@zaach Seems right?

pdehaan · 2015-06-15T17:40:06Z

@zaach @vladikoff @shane-tomlinson Anybody want to look at this one last time before merging?

chore(lint): switch from JSHint to ESLint r=vladikoff

TDA added the waffle:needs review label Jun 10, 2015