Switch from JSHint to ESLint

[pdehaan]

JSHint isn't super-active anymore and we increasingly need to use it with a mix of JSCS rules to get the formatting and linting that we like.

Should we just switch out to ESLint since it is technically superior?

Ref: #1318

[vladikoff]

+1

[vladikoff]

@zaach @shane-tomlinson any thoughts against this?

[zaach]

👍

[TDA]

+1 for this.

[pdehaan]

Obviously also requires tweaking all the inline //jshint ... and /*jshint ... */ snippets sprinkled throughout the codebase.

Also, figure out why we're currently using grunt-contrib-jshint AND jshint in the dev dependencies... (and then remove them both, along w/ jshint-stylish):

    "grunt-contrib-jshint": "0.10.0",
    "jshint": "2.5.3",
    "jshint-stylish": "0.4.0",

[pdehaan]

Ref: #2537 (comment)

[shane-tomlinson]

I dig this plan. Can we run them in parallel for a bit until we have eslint set up just so, and then rip out jshint as a 2nd step?

[vladikoff]

I dig this plan. Can we run them in parallel for a bit until we have eslint set up just so, and then rip out jshint as a 2nd step?

That's a good idea!

[pdehaan]

Actually, I had a rough version of an .eslintrc file already for anybody that wants a starting point:

.eslintrc

env:
  browser: true
  jasmine: true
  jquery: true
  mocha: true
  node: true

globals:
  FxaHead: true
  Promise: true
  translator: true

rules:
  camelcase: 0
  comma-dangle: 0
  complexity: [2, 6]
  curly: [2, all]
  dot-notation: 0
  eqeqeq: [2, allow-null]
  global-strict: [0, never]
  handle-callback-err: 0
  key-spacing: 0
  new-cap: 0
  no-comma-dangle: 0
  no-cond-assign: [2, except-parens]
  no-debugger: 2
  no-empty: 0
  no-eval: 2
  no-irregular-whitespace: 0
  no-multi-spaces: 0
  no-new: 2
  no-script-url: 2
  no-shadow: 0
  no-spaced-func: 0
  no-undef: 2
  no-unused-vars: [1, {vars: all, args: none}]
  no-use-before-define: true
  no-with: 2
  quotes: [2, single]
  semi: [2, always]
  space-unary-ops: 0
  strict: 2
  valid-typeof: 1
  wrap-iife: 0
  yoda: 0

.eslintignore

.tmp/**
app/bower_components/**
app/scripts/vendor/**
dist/**

output:

$ eslint .

app/scripts/lib/app-start.js
  355:36  error  Function 'anonymous' has a complexity of 7  complexity

app/scripts/lib/metrics.js
  64:2  error  Function 'Metrics' has a complexity of 18  complexity

app/scripts/lib/screen-info.js
  13:2  error  Function 'ScreenInfo' has a complexity of 8  complexity

app/scripts/views/base.js
  89:17  error  Function 'anonymous' has a complexity of 10  complexity

app/tests/mocks/crosstab.js
  14:22  error  Don't make functions within a loop  no-loop-func

app/tests/spec/lib/channels/web.js
  34:8  error  Do not use 'new' for side effects  no-new

app/tests/spec/lib/xhr.js
  15:37  error  Shadowing of global property "undefined"  no-shadow-restricted-names

app/tests/spec/lib/xss.js
  42:20  error  Script URL is a form of eval  no-script-url

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
  32:8  warning  windowMock is defined but never used  no-unused-vars

grunttasks/l10n-create-json.js
  9:1  error  Unexpected token const

grunttasks/l10n-extract.js
  7:1  error  Unexpected token const

grunttasks/l10n-merge.js
  7:1  error  Unexpected token const

grunttasks/l10n-supported-locales.js
  5:1  error  Unexpected token const

grunttasks/marked.js
  5:1  error  Unexpected token const

grunttasks/po2json.js
  8:1  error  Unexpected token const

scripts/run_locally.js
  6:1  error  Unexpected token const

server/bin/fxa-content-server.js
  23:4  error  config is already defined  no-redeclare

server/lib/csp.js
  16:11  error  Strings must use singlequote  quotes

server/lib/statsd-collector.js
  13:0  error  Function 'getGenericTags' has a complexity of 7  complexity

tests/functional/oauth_iframe.js
  34:6  warning  user is defined but never used  no-unused-vars

tests/functional/oauth_preverified_sign_up.js
  20:6  warning  user is defined but never used  no-unused-vars

tests/functional/oauth_reset_password.js
  24:6  warning  user is defined but never used         no-unused-vars
  27:6  warning  accountData is defined but never used  no-unused-vars

tests/functional/oauth_sign_up.js
  25:6  warning  user is defined but never used  no-unused-vars

tests/functional/oauth_sync_sign_in.js
  26:6  warning  user is defined but never used  no-unused-vars

tests/functional/oauth_webchannel.js
  24:6  warning  user is defined but never used  no-unused-vars

tests/functional/oauth_webchannel_keys.js
  26:6  warning  user is defined but never used  no-unused-vars

tests/functional/sign_in.js
  21:6  warning  user is defined but never used  no-unused-vars

tests/functional/sign_in_cached.js
  39:6  warning  user is defined but never used   no-unused-vars
  40:6  warning  user2 is defined but never used  no-unused-vars

tests/functional/sync_reset_password.js
  28:6  warning  accountData is defined but never used  no-unused-vars

tests/functional/sync_sign_in.js
  25:6  warning  user is defined but never used  no-unused-vars

tests/functional/sync_v2_sign_up.js
  27:6  warning  client is defined but never used  no-unused-vars

tests/intern.js
  13:0  error  Function 'anonymous' has a complexity of 11  complexity

✖ 39 problems (19 errors, 20 warnings)

[pdehaan]

We can "fix" the const warnings, "Unexpected token const", by setting an env var of es6: true in the .eslintrc file, like this:

env:
  es6: true
  ...

Unfortunately, it doesn't look like we can set that inline for just the specific files that use const [yet], per eslint/eslint#2134

/*eslint-env es6 */
// ^^^ This doesn't currently work. :(

[pdehaan]

Ah, I also converted our .jshintrc to the .eslintrc file above using https://github.com/valorkin/eslint-rules-mapper and then a bit of manual tweaking to make it more YAML-ey and silence some other errors/warnings for the time being.

[pdehaan]

Talking w/ @TDA, it seems like we have two options for Grunt+ESLint modules:

• https://www.npmjs.com/package/grunt-contrib-eslint
• https://www.npmjs.com/package/grunt-eslint

I strongly vote for grunt-eslint here since grunt-contrib-eslint doesn't seem to be Grunt official module (just poorly named), and the grunt-eslint is maintained by Sindre (who is awesome).

[vladikoff]

use https://www.npmjs.com/package/grunt-eslint

[vladikoff]

Thank you @TDA!

[TDA]

Couldn't have done it without your help guys :) @pdehaan @vladikoff @zaach