Eslint has had a few version since the last time I checked the config. They added new options, fixed a few that were broken and our codebase needs to get up to date. Some of the options are not "required" but enforce parts of our JS coding standards:

Here are the config changes:

    "block-scoped-var": 2,

This help spot variable reuse, and make it explicit at which level the variable is used. 

    "no-undefined": 2,

This is to forbid if (somevar === undefined) because it should be written if (typeof somevar === 'undefined')

    "brace-style": [2, "stroustrup", { "allowSingleLine": true }],
    "comma-style": [2, "last"],
    "semi": [2, "always"],
    "space-after-keywords": [2, "always", {"checkFunctionKeyword": true}],
    "spaced-line-comment": [2, "always"],

Just enforcing our coding standards.

CommentFileSizeAuthor
#17 interdiff.txt304 bytesnod_
#17 core-js-eslint-rules-2389515-17.patch69.55 KBnod_
#16 eslint-output.txt3.34 MBwim leers
#13 core-js-eslint-rules-2389515-12.patch69.21 KBnod_
#13 core-js-eslint-rules-2389515-12-ignorespace-do-not-test.patch27.04 KBnod_
#6 interdiff-w.txt645 bytesnod_
#6 core-js-eslint-rules-2389515-6.patch69.98 KBnod_
#5 core-js-eslint-rules-2389515-5.patch69.75 KBnod_
#5 interdiff-w.txt742 bytesnod_
#5 interdiff.txt44.41 KBnod_
#4 core-js-eslint-rules-2389515-4.patch29.47 KBnod_
#4 interdiff.txt2.59 KBnod_
#3 core-js-eslint-rules-2389515-3.patch27.86 KBnod_
core-js-eslint-rules.patch25.55 KBnod_

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +DX (Developer Experience)

This helps improve the DX.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

#2382533: Attach assets only via the asset library system landed, this will need a reroll now.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
StatusFileSize
new core-js-eslint-rules-2389515-3.patch27.86 KB

Updated

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new interdiff.txt2.59 KB
new core-js-eslint-rules-2389515-4.patch29.47 KB

Added a couple of rules to prevent the code from being crazy.

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new interdiff.txt44.41 KB
new interdiff-w.txt742 bytes
new core-js-eslint-rules-2389515-5.patch69.75 KB

Some more coding standard rules.

    "no-implied-eval": 2,
    "space-before-blocks": [2, "always"],
    "space-in-brackets": [2, "never"],
    "space-in-parens": [2, "never"],

for the space in brackets thing, I tried both ways. Requiring a space got me 1200 errors, requiring no space, 178 errors. I went with that.

I attached the interdiff, and the interdiff ignoring whitespace.

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-eslint-rules-2389515-6.patch69.98 KB
new interdiff-w.txt645 bytes

Found some jshint related comment that needs to go away.

The last submitted patch, 4: core-js-eslint-rules-2389515-4.patch, failed testing.

The last submitted patch, 5: core-js-eslint-rules-2389515-5.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: core-js-eslint-rules-2389515-6.patch, failed testing.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review

daffie queued 6: core-js-eslint-rules-2389515-6.patch for re-testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll, +Needs manual testing

Just committed #2397681: field_ui.js fails eslint validation so this one will need a re-roll. Also tagging for manual testing (this shouldn't break anything but we should do a click-through of a few things to make sure).

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new core-js-eslint-rules-2389515-12-ignorespace-do-not-test.patch27.04 KB
new core-js-eslint-rules-2389515-12.patch69.21 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

If I run eslint . in the Drupal 8 root, then I get thousands of errors. Mostly in 3rd party libraries, but also in core JS. Am I not using the right incantation?

In any case: the patch looks great. I can't find anything to complain about!

nod_’s picture

nod_
Primary language French
Location Lille
commented

Version is important, latest is 0.11 what you got?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new eslint-output.txt3.34 MB

I installed it a few weeks ago, and I've got 0.9.2 apparently. Doesn't .eslintrc allow you to specify a minimum version? That'd prevent this problem.

After updating, only one problem remains.

Output of this command attached:

wim.leers at wimleers-acquia in ~/Work/drupal-tres on 8.0.x*
$ eslint . > output.txt

eslint is also linting JS in the libraries directory. This directory is indeed non-standard, but this also means it should not be checked by eslint.

nod_’s picture

nod_
Primary language French
Location Lille
commented
StatusFileSize
new core-js-eslint-rules-2389515-17.patch69.55 KB
new interdiff.txt304 bytes

Excluding libraries folder, from root, profile and sites directories.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

Looks great now :)

All the affected JS still works fine.

alexpott’s picture

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

Committed a3efcb6 and pushed to 8.0.x. Thanks!

  • alexpott committed a3efcb6 on 8.0.x 
    Issue #2389515 by nod_: Update ESLint rules

Status: Fixed » Closed (fixed)

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