Style error information

https://eslint.org/docs/rules/no-var

Remark

- Code refactoring. Review it carefully :)
- Introduces another issue, no-case-declarations.

How to Review

## 1. Apply Patch
## 2. Review Code Changes
## 3. Confirm no Code Standard Errors
yarn & yarn lint:core-js-passing
## 4.1 If `NO` errors, mark the issue as `Reviewed & tested by the community` (Don't be shy, we're all friendly)
## 4.2 If `HAS` errors, fix it and upload a new patch (Just do it and you can!!!)

Background

- #2912962: Step 1 JS codestyle: [meta] Fix JS coding standards in core

- We adapted the airbnb coding standard (#2815077: Adopt airbnb javascript style guide v14.1 as new baseline javascript coding standards for Drupal 8 core and contrib), but we are not fully compliant to it yet.

More Information

- Using ES6 in Core
https://www.drupal.org/node/2815083

- To find JS code standard errors stats
cd core/ && yarn & yarn lint:core-js-stats

Valuable Follow-up

- N/A

CommentFileSizeAuthor
#12 interdiff-2914946-10-12.txt3.22 KBGrandmaGlassesRopeMan
#12 2914946-12.patch35.74 KBGrandmaGlassesRopeMan
#10 interdiff-2914946-3-10.txt30.14 KBGrandmaGlassesRopeMan
#10 2914946-10.patch39.09 KBGrandmaGlassesRopeMan
#3 2914946.patch12.3 KBGrandmaGlassesRopeMan

Comments

drpal created an issue. See original summary.

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
Title: JS codestyle: no-lonely-if » JS codestyle: no-var
GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
StatusFileSize
new 2914946.patch12.3 KB
dawehner’s picture

dawehner
Primary language German
commented 
  1. +++ b/core/misc/tableselect.es6.js
@@ -129,7 +129,7 @@
     for (let i = from[mode]; i; i = i[mode]) {
-      var $i;
+      let $i;
       // Make sure that we're only dealing with elements.
       if (i.nodeType !== 1) {
         continue;

    IMHO the $i could be initialized as const later $i = $(i);... Should we do that instead? 

  2. +++ b/core/modules/quickedit/js/views/EditorView.es6.js
@@ -124,7 +124,7 @@
           // set the 'active' state. A "loading" indicator will be shown in the
           // UI for as long as the field remains in this state.
-          var loadDependencies = function (callback) {
+          const loadDependencies = function (callback) {
             // Do the loading here.
             callback();
           };

    There is some weird code going on if you look at it with more context, ¯\_(ツ)_/¯

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented

@dawehner

Yep. I saw both of those when going through these changes. I opted not to actually fix the larger issues to avoid introducing any regressions. I can file a followup to add the 'no-case-declarations' to the ignore list.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Fair point, do you mind creating these follow ups?

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented

#2915263: Exclude eslint rule no-case-declarations

droplet’s picture

droplet commented

Why not fix it together? `no-case-declarations` helping you to understand const/let/var scope around that switch.

droplet’s picture

droplet commented

and I checkout the patch locally. I think @dawehner's point 1 should do it together when it's a trivial change. We better to fix it to final, not introduce another bug, haha (Another new bug might get fixed on another patch :p, no more reroll)

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
StatusFileSize
new 2914946-10.patch39.09 KB
new interdiff-2914946-3-10.txt30.14 KB

Yeah, ok. Easy enough. 👍👏

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2914946-10.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
Status: Needs work » Needs review
StatusFileSize
new 2914946-12.patch35.74 KB
new interdiff-2914946-10-12.txt3.22 KB

🤷‍♀️

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
dawehner’s picture

dawehner
Primary language German
commented 
index b013f1445a..9fe1d22f7b 100644
--- a/core/.eslintrc.passing.json

--- a/core/.eslintrc.passing.json
+++ b/core/.eslintrc.passing.json

+++ b/core/.eslintrc.passing.json
@@ -1,7 +1,6 @@

@@ -1,7 +1,6 @@
 {
   "extends": "./.eslintrc.json",
   "rules": {
-    "no-var": "off",
     "no-useless-escape": "off",
     "no-useless-concat": "off",
     "no-use-before-define": "off",

Don't we want to have the second rule removed from here as well?

Status: Needs review » Needs work

The last submitted patch, 12: 2914946-12.patch, failed testing. View results

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

The testbots had a bit of a quickup.

No errors are left: 

 $ npm run lint:core-js-passing

> Drupal@ lint:core-js-passing /Users/dawehner/www/d8/core
> node ./node_modules/eslint/bin/eslint.js --quiet --config=.eslintrc.passing.json --ext=.es6.js . || exit 0

In #4 I went through all the instances and checked whether const/let was applied correctly, and just found the instances above.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
droplet’s picture

droplet commented
Status: Needs work » Needs review
StatusFileSize
new eslint-no-var.patch35.74 KB

I help @drpal reroll it quick. Fixes `core/.eslintrc.passing.json` conflicts. (no credits to me :p)

droplet’s picture

droplet commented
Issue tags: -Needs reroll
GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented

@droplet Literally seconds before me. 🤘

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Committed 29cd930 and pushed to 8.5.x. Thanks!

  • lauriii committed 29cd930 on 8.5.x 
    Issue #2914946 by drpal, dawehner, droplet: JS codestyle: no-var

Status: Fixed » Closed (fixed)

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