Problem/Motivation

The process for applying our ESLint rules is a manual process. Without a test command in a package.json file this process is often overlooked during development.

Proposed resolution

Adding a script to package.json will allow other developers to apply the eslint standards more easily. After the patch and once the dependencies are installed (using npm install) the code can be checked with npm run lint.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drpal created an issue. See original summary.

dawehner’s picture

<3

GrandmaGlassesRopeMan’s picture

Issue summary: View changes
droplet’s picture

this will finish without error message from npm.

nod_’s picture

Title: [patch] Add ESLint to package.json » Add ESLint to package.json
Issue summary: View changes
Status: Active » Needs review
Manuel Garcia’s picture

+1

Gave it a go just now working as expected...

/v/w/drupal8 (8.3.x) $ git apply --index -v eslint-2809343-4.patch 
Checking patch package.json...
Applied patch package.json cleanly.
/v/w/drupal8 (8.3.x) $ npm install
Drupal@8.3.0 /var/www/drupal8
└─┬ eslint@3.6.1 
  ├─┬ chalk@1.1.3 
  │ ├── ansi-styles@2.2.1 
  │ ├── escape-string-regexp@1.0.5 
  │ ├── has-ansi@2.0.0 
  │ ├── strip-ansi@3.0.1 
  │ └── supports-color@2.0.0 
  ├─┬ concat-stream@1.5.2 
  │ ├── inherits@2.0.3 
  │ ├─┬ readable-stream@2.0.6 
  │ │ ├── core-util-is@1.0.2 
  │ │ ├── process-nextick-args@1.0.7 
  │ │ ├── string_decoder@0.10.31 
  │ │ └── util-deprecate@1.0.2 
  │ └── typedarray@0.0.6 
  ├─┬ debug@2.2.0 
  │ └── ms@0.7.1 
  ├─┬ doctrine@1.4.0 
  │ └── isarray@1.0.0 
  ├─┬ escope@3.6.0 
  │ ├─┬ es6-map@0.1.4 
  │ │ ├── d@0.1.1 
  │ │ ├── es5-ext@0.10.12 
  │ │ ├── es6-iterator@2.0.0 
  │ │ ├── es6-set@0.1.4 
  │ │ ├── es6-symbol@3.1.0 
  │ │ └── event-emitter@0.3.4 
  │ ├── es6-weak-map@2.0.1 
  │ └─┬ esrecurse@4.1.0 
  │   └── estraverse@4.1.1 
  ├─┬ espree@3.3.2 
  │ ├── acorn@4.0.3 
  │ └─┬ acorn-jsx@3.0.1 
  │   └── acorn@3.3.0 
  ├── estraverse@4.2.0 
  ├── esutils@2.0.2 
  ├─┬ file-entry-cache@2.0.0 
  │ ├─┬ flat-cache@1.2.1 
  │ │ ├── circular-json@0.3.1 
  │ │ ├─┬ del@2.2.2 
  │ │ │ ├─┬ globby@5.0.0 
  │ │ │ │ ├─┬ array-union@1.0.2 
  │ │ │ │ │ └── array-uniq@1.0.3 
  │ │ │ │ └── arrify@1.0.1 
  │ │ │ ├── is-path-cwd@1.0.0 
  │ │ │ ├─┬ is-path-in-cwd@1.0.0 
  │ │ │ │ └── is-path-inside@1.0.0 
  │ │ │ ├── pify@2.3.0 
  │ │ │ ├─┬ pinkie-promise@2.0.1 
  │ │ │ │ └── pinkie@2.0.4 
  │ │ │ └── rimraf@2.5.4 
  │ │ ├── graceful-fs@4.1.9 
  │ │ └── write@0.2.1 
  │ └── object-assign@4.1.0 
  ├─┬ glob@7.1.0 
  │ ├── fs.realpath@1.0.0 
  │ ├─┬ inflight@1.0.5 
  │ │ └── wrappy@1.0.2 
  │ ├─┬ minimatch@3.0.3 
  │ │ └─┬ brace-expansion@1.1.6 
  │ │   ├── balanced-match@0.4.2 
  │ │   └── concat-map@0.0.1 
  │ ├── once@1.4.0 
  │ └── path-is-absolute@1.0.1 
  ├── globals@9.10.0 
  ├── ignore@3.1.5 
  ├── imurmurhash@0.1.4 
  ├─┬ inquirer@0.12.0 
  │ ├── ansi-escapes@1.4.0 
  │ ├── ansi-regex@2.0.0 
  │ ├─┬ cli-cursor@1.0.2 
  │ │ └─┬ restore-cursor@1.0.1 
  │ │   ├── exit-hook@1.1.1 
  │ │   └── onetime@1.1.0 
  │ ├── cli-width@2.1.0 
  │ ├── figures@1.7.0 
  │ ├─┬ readline2@1.0.1 
  │ │ ├─┬ code-point-at@1.0.1 
  │ │ │ └── number-is-nan@1.0.1 
  │ │ ├── is-fullwidth-code-point@1.0.0 
  │ │ └── mute-stream@0.0.5 
  │ ├── run-async@0.1.0 
  │ ├── rx-lite@3.1.2 
  │ ├── string-width@1.0.2 
  │ └── through@2.3.8 
  ├─┬ is-my-json-valid@2.14.0 
  │ ├── generate-function@2.0.0 
  │ ├─┬ generate-object-property@1.2.0 
  │ │ └── is-property@1.0.2 
  │ ├── jsonpointer@2.0.0 
  │ └── xtend@4.0.1 
  ├─┬ is-resolvable@1.0.0 
  │ └── tryit@1.0.2 
  ├─┬ js-yaml@3.6.1 
  │ ├─┬ argparse@1.0.9 
  │ │ └── sprintf-js@1.0.3 
  │ └── esprima@2.7.3 
  ├─┬ json-stable-stringify@1.0.1 
  │ └── jsonify@0.0.0 
  ├─┬ levn@0.3.0 
  │ ├── prelude-ls@1.1.2 
  │ └── type-check@0.3.2 
  ├── lodash@4.16.2 
  ├─┬ mkdirp@0.5.1 
  │ └── minimist@0.0.8 
  ├── natural-compare@1.4.0 
  ├─┬ optionator@0.8.2 
  │ ├── deep-is@0.1.3 
  │ ├── fast-levenshtein@2.0.5 
  │ └── wordwrap@1.0.0 
  ├── path-is-inside@1.0.2 
  ├── pluralize@1.2.1 
  ├── progress@1.1.8 
  ├─┬ require-uncached@1.0.2 
  │ ├─┬ caller-path@0.1.0 
  │ │ └── callsites@0.2.0 
  │ └── resolve-from@1.0.1 
  ├── shelljs@0.6.1 
  ├── strip-bom@3.0.0 
  ├── strip-json-comments@1.0.4 
  ├─┬ table@3.8.0 
  │ ├─┬ ajv@4.7.5 
  │ │ └── co@4.6.0 
  │ ├── ajv-keywords@1.1.1 
  │ └── slice-ansi@0.0.4 
  ├── text-table@0.2.0 
  └─┬ user-home@2.0.0 
    └── os-homedir@1.0.2 
/v/w/drupal8 (8.3.x) $ npm run lint

> Drupal@8.3.0 lint /var/www/drupal8
> eslint -c .eslintrc core || exit 0
GrandmaGlassesRopeMan’s picture

@droplet

Could you possibly paste the error you were getting? I didn't see anything. What node version are you running on. We should probably specify a engine version.

dawehner’s picture

So I'm wondering whether the file should be maybe in core/package.json kind of similar to have the main composer.json in core as well.

nod_’s picture

Unlike composer.json files, there is no merge or extend functionality built in package.json file.

@matt: introduce a syntax violation in some code, we need the exit 0 otherwise it's ugly.

dawehner’s picture

Unlike composer.json files, there is no merge or extend functionality built in package.json file.

Well, I'm asking that because in a world of drupal-project/drupal-scaffold just the core folder is actually shipped, so it might be useful to still have the package.json shipped then.

droplet’s picture

@drpal,

edit a file in CORE dir and make it fail.

more background info: https://github.com/eslint/eslint/issues/2409

nod_’s picture

GrandmaGlassesRopeMan’s picture

wrong patch here.

GrandmaGlassesRopeMan’s picture

Sorry, this is the correct patch. Rerolled due to #2809477: Add ES6 to ES5 build process being merged.

nod_’s picture

Status: Needs review » Needs work

Folder looks wrong already in core folder.

GrandmaGlassesRopeMan’s picture

Status: Needs work » Needs review
FileSize
746 bytes
535 bytes

This feels slightly wrong, since it doesn't appear you can 'extend' the eslint ignore rules. I tried to get --ignore-path to work, but encountered some strange file system bugs.

nod_’s picture

  1. Renamed .eslintrc to .eslintrc.json as recommended by eslint.
  2. Added a .eslintignore file to core/ and leave the one in the root alone.
  3. Added a .eslintrc.json in the scripts/js subfolder to fix rules setting for scripts in there.

code changes in the babel script are to follow airbnb coding standards.

GrandmaGlassesRopeMan’s picture

+++ b/core/package.json
@@ -5,12 +5,15 @@
+    "babel-core": "^6.17.0",

Should we keep these at specific versions like the rest of our dependencies.

droplet’s picture

alexpott’s picture

Status: Needs review » Fixed

Committed 193aae3 and pushed to 8.3.x. Thanks!

  • alexpott committed 193aae3 on 8.3.x
    Issue #2809343 by drpal, droplet, nod_: Add ESLint to package.json
    
tstoeckler’s picture

Status: Fixed » Needs work

I think the change to active-link.js that was committed was unintentional ?!

alexpott’s picture

Status: Needs work » Fixed

@tstoeckler++ thank you.

commit 93c635b982369f0cc099eafd3c2adc27bd789165
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Sun Oct 23 22:56:27 2016 -0700

    Issue #2809343 by drpal, droplet, nod_, tstoeckler: Followup revert incorrect change

diff --git a/core/misc/active-link.js b/core/misc/active-link.js
index 65037f7..9cf55b4 100644
--- a/core/misc/active-link.js
+++ b/core/misc/active-link.js
@@ -26,7 +26,7 @@
       // Start by finding all potentially active links.
       var path = drupalSettings.path;
       var queryString = JSON.stringify(path.currentQuery);
-      var querySelector = path.currentQuery ? "[data-eedrupal-link-query='" + queryString + "']" : ':not([data-drupal-link-query])';
+      var querySelector = path.currentQuery ? "[data-drupal-link-query='" + queryString + "']" : ':not([data-drupal-link-query])';
       var originalSelectors = ['[data-drupal-link-system-path="' + path.currentPath + '"]'];
       var selectors;
 

I reverted the change to this single file. @tstoeckler is correct - nothing to do with this change. I should have spotted that.

  • alexpott committed 93c635b on 8.3.x
    Issue #2809343 by drpal, droplet, nod_, tstoeckler: Followup revert...
droplet’s picture

Ouch. I made a big mistake. Sorry! I will investigate why I included it. I staged them before interdiff & patch..

and this is my concern in Point 3 of :
https://www.drupal.org/node/2809477#comment-11680919

and comment 8
https://www.drupal.org/node/2809477#comment-11681103

Status: Fixed » Closed (fixed)

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