ESlint is designed to be pluggable unlike JSHint and JSLint. And will ship with stylistic rules, same rules that are dropped from JSHint.
http://eslint.org/docs/rules/

I'd like to switch considering we'll need JSHint and JSCS in the near future to enforce our coding standards because JSHint dropped all support for code style checks. Using ESlint we only have to rely on one for everything and we're able to add custom rules easily.

So far I tried with this config and things seems to work fine.

.eslintrc

{
  "env": {
    "browser": true
  },
  "globals": {
    "Drupal": true,
    "drupalSettings": true,
    "domready": true,
    "jQuery": true,
    "_": true,
    "matchMedia": true,
    "Backbone": true,
    "Modernizr": true,
    "CKEDITOR": true
  }, 
  "rules": {
    "quotes": [1, "single", "avoid-escape"],
    "semi": [2, "always"],
    "brace-style": ["2", "stroustrup"],
    "new-cap": 1,
    "strict": 2,
    "no-extra-strict": 1,
    "no-unused-vars": [2, {"vars": "local", "args": "none"}],
    "no-underscore-dangle": 0,
    "no-new": 0,
    "camelcase": 0
  }
}

.eslintignore

core/assets/vendor/*
core/modules/tour/js/jquery.joyride-2.0.3.js
core/vendor/*
sites/*/files

what's cool is that it allows us to say what is an error and what is a warning. Allowing the config to be very strict on code-style rules while not getting too much in the way and keeping the dangerous things as errors.

This config still gives 442 "problems" (errors and warnings).

CommentFileSizeAuthor
#2 core-eslint-2264205-2.patch1.87 KBnod_
#1 core-eslint-2264205-1.patch1.63 KBnod_

Comments

nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: Consider using eslint for JavaScript validation » Consider using ESLint for JavaScript validation
StatusFileSize
new core-eslint-2264205-1.patch1.63 KB

Tweaked the config some more, used JSHint compatibility table to replicate the config we have.

Using ESLint shows that JSHint is pretty buggy on some rules checks and that we're still sloppy, there are a couple of straight up bugs raised by this config:

{
  "env": {
    "browser": true
  },
  "globals": {
    "Drupal": true,
    "drupalSettings": true,
    "domready": true,
    "jQuery": true,
    "_": true,
    "matchMedia": true,
    "Backbone": true,
    "Modernizr": true,
    "CKEDITOR": true
  },
  "rules": {
    "eqeqeq": [2, "smart"],
    "guard-for-in": 2,
    "no-undef": 2,
    //"no-unused-vars": [2, {"vars": "local", "args": "none"}],
    "no-unused-vars": 0,
    "strict": 2,
    "new-cap": 0,
    "quotes": 0,
    "camelcase": 0,
    "no-underscore-dangle": 0,
    "no-new": 0,
    "no-alert": 0,
    "no-use-before-define": 0
  }
}

Output: 

core/misc/machine-name.js: line 45, col 12, Error - settings is already declared in the upper scope. (no-shadow)
core/misc/states.js: line 440, col 4, Error - Unexpected constant condition. (no-constant-condition)
core/modules/block/js/block.admin.js: line 22, col 12, Error - $details is already declared in the upper scope. (no-shadow)
core/modules/ckeditor/js/ckeditor.admin.js: line 1181, col 8, Error - 'event' is not defined. (no-undef)
core/modules/ckeditor/js/plugins/drupalimage/plugin.js: line 63, col 10, Error - Expected no return value. (consistent-return)
core/modules/ckeditor/js/plugins/drupalimage/plugin.js: line 74, col 58, Error - Trailing comma. (no-comma-dangle)
core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.js: line 156, col 10, Error - Expected no return value. (consistent-return)
core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.js: line 165, col 36, Error - Trailing comma. (no-comma-dangle)
core/modules/contextual/js/contextual.toolbar.js: line 34, col 56, Error - Trailing comma. (no-comma-dangle)
core/modules/contextual/js/contextual.toolbar.js: line 59, col 15, Error - Trailing comma. (no-comma-dangle)
core/modules/locale/tests/locale_test.js: line 12, col 3, Error - Missing semicolon. (semi)
core/modules/locale/tests/locale_test.js: line 7, col 0, Error - Unexpected space between function name and paren. (no-spaced-func)
core/modules/locale/tests/locale_test.js: line 13, col 0, Error - Unnecessary semicolon. (no-extra-semi)
core/modules/locale/tests/locale_test.js: line 37, col 3, Error - Missing semicolon. (semi)
core/modules/locale/tests/locale_test.js: line 30, col 0, Error - Unexpected space between function name and paren. (no-spaced-func)
core/modules/locale/tests/locale_test.js: line 38, col 0, Error - Unnecessary semicolon. (no-extra-semi)
core/modules/quickedit/js/views/AppView.js: line 210, col 19, Error - Empty block statement. (no-empty)
core/modules/system/system.modules.js: line 22, col 12, Error - $details is already declared in the upper scope. (no-shadow)
core/modules/views_ui/js/views-admin.js: line 534, col 6, Error - 'event' is not defined. (no-undef) 

19 problems
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Active » Needs review
StatusFileSize
new core-eslint-2264205-2.patch1.87 KB

With #2264241: Fix ESLint errors and fix js bugs and this patch, everything validates.

Now we get to pick how picky we want to be.

attiks’s picture

attiks commented
Status: Needs review » Reviewed & tested by the community

This looks like it is ready for commit, I assume we discuss how picky we want to be in a separate issue?

nod_’s picture

nod_
Primary language French
Location Lille
commented

yeah that's fair enough with me. We'll need to fix all the stuff we get picky about so it's another task than acting on not using jshint and updating all related docs.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: Consider using ESLint for JavaScript validation » Replace JSHint by ESLint for JavaScript validation

I'm not kidding anyone :p

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: core-eslint-2264205-2.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Title: Replace JSHint by ESLint for JavaScript validation » Replace JSHint with ESLint for JavaScript validation
Status: Needs work » Reviewed & tested by the community

Random fail from when HEAD was broken (Drupal\simpletest\Tests\InstallationProfileModuleTestsTest)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 5f62e92 and pushed to 8.x. Thanks!

  • Commit 5f62e92 on 8.x by alexpott: 
    Issue #2264205 by _nod: Replace JSHint with ESLint for JavaScript...
nod_’s picture

nod_
Primary language French
Location Lille
commented

Issue for configuration choice #2274195: Decide ESLint config

nod_’s picture

nod_
Primary language French
Location Lille
commented

Updated doc page: https://drupal.org/node/1955232
Will write a new change notice to replace https://drupal.org/node/1972428

nod_’s picture

nod_
Primary language French
Location Lille
commented

New change notice https://drupal.org/node/2274223 added a comment in the old one to say it's deprecated. Not sure the proper way to do that, we should probably delete/unpublish the previous JSHint change notice but I don't know.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I've unpublished the old JSHint change notice - @nod_ thanks for sorting out the change notices!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.