Upgrade eslint #9357

spalger · 2016-12-02T23:12:22Z

A prereq for #8315

Upgrades eslint and @elastic/eslint-config-kibana to their latest versions.

Some of the new rules from @elastic/eslint-config-kibana had to be disabled and should be reenabled one-by-one as a part of #9355

As a part of this change gruntify-eslint was replaced with a custom grunt task. This was necessary because gruntify-eslint:

embeds its own version of eslint, rather than relying on a peerDependency
relies on grunt's file resolution algorithm rather than eslint's, which means it does not utilitze the .eslintignore file
does not support "fix" mode

Fixes #7622

cjcenizal · 2016-12-10T01:00:34Z

FYI this will also address #7622

cjcenizal

Looks sweet overall. I had one question.

cjcenizal · 2016-12-10T01:03:50Z

Gruntfile.js

-      '!<%= src %>/core_plugins/timelion/vendor_components/**/*.js',
-      '!<%= src %>/fixtures/**/*.js',
-      '!<%= root %>/test/fixtures/scenarios/**/*.js'
-    ]


cjcenizal · 2016-12-10T01:05:07Z

src/cli/cluster/cluster_manager.js

+      ...extraPaths
+    ].map(path => resolve(path));
+
+    this.watcher = chokidar.watch(uniq(watchPaths), {


Really nice attention to detail dude... I ❤️ it

cjcenizal · 2016-12-10T01:09:48Z

tasks/eslint.js

+    if (report.warningCount > 0) errTypes.push('warning');
+    if (!errTypes.length) return;
+
+    grunt.log.write(formatter(report.results));


Do we get to this point if there are no errors? If there are no errors, do we still want to see the report results? Without knowing what the report contains, it seems like this could be useful information?

The report actually only contains errors and warnings, so if there are none of those then there isn't anything to report

Sounds good. How about renamingreport to errorReport or adding a comment to make that clearer? Otherwise I can see how someone might have the same question I had from reading this code.

Added a comment describing what the report is. Since it contains errors, warnings, and fix info, errorReport didn't seem appropriate.

…t-try2

cjcenizal

Sweeeeeeet!

epixa · 2016-12-12T14:23:09Z

@spalger Can you update the PR description with an explanation of why the new task was necessary and such?

epixa · 2016-12-12T14:23:40Z

.eslintignore

-test/fixtures/scenarios
-optimize
-test/fixtures/scenarios
+/src/ui/public/utils/decode_geo_hash.js


Why is this specific file excluded?

It's copied from an external source

epixa · 2016-12-12T14:25:23Z

package.json

@@ -165,17 +165,17 @@
    "wreck": "6.2.0"
  },
  "devDependencies": {
-    "@elastic/eslint-config-kibana": "0.0.3",
+    "@elastic/eslint-config-kibana": "0.2.1",


Once all of this eslint stuff is done, let's bump this to 1.0.0.

epixa · 2016-12-12T14:25:49Z

package.json

@@ -189,7 +189,6 @@
    "grunt-karma": "2.0.0",
    "grunt-run": "0.6.0",
    "grunt-simple-mocha": "0.4.0",
-    "gruntify-eslint": "1.0.1",


epixa · 2016-12-12T14:26:00Z

src/cli/cluster/__tests__/_mock_cluster_fork.js

@@ -24,7 +24,7 @@ export default class MockClusterFork extends EventEmitter {
            dead = true;
            this.emit('exit');
            cluster.emit('exit', this, this.exitCode || 0);
-          }());


How did this ever work?

The babel parser missed the standard there apparently. it's valid for function expressions, just not arrow function expressions.

epixa · 2016-12-12T14:27:18Z

tasks/config/eslint.js

@@ -1,30 +1,33 @@
-var resolve = require('path').resolve;
+import { resolve } from 'path';
+module.exports = grunt => ({


export default?

epixa · 2016-12-12T14:28:28Z

tasks/eslint.js

+  fix: false
+};
+
+module.exports = grunt => {


export default?

epixa

I left a few comments (mostly questions).

…t-try2

spalger · 2016-12-12T20:28:31Z

@epixa I believe that addresses your feedback

epixa · 2016-12-12T20:40:28Z

LGTM once the build is green

Backports PR #9357 **Commit 1:** upgrade eslint, all related deps, and config files * Original sha: 054e798 * Authored by spalger <email@spalger.com> on 2016-08-24T23:39:11Z * Committed by spalger <spalger@users.noreply.github.com> on 2016-12-02T23:04:20Z **Commit 2:** replace gruntify-eslint with basic eslint-cli wrapper * Original sha: 71732e7 * Authored by spalger <email@spalger.com> on 2016-09-02T21:33:02Z * Committed by spalger <spalger@users.noreply.github.com> on 2016-12-02T23:41:36Z **Commit 3:** arrow-IIFEs must be invoked outside of the parens * Original sha: b05662c * Authored by spalger <email@spalger.com> on 2016-08-25T17:47:57Z * Committed by spalger <spalger@users.noreply.github.com> on 2016-12-02T23:41:40Z **Commit 4:** move import statements before their use * Original sha: 3572ab8 * Authored by spalger <email@spalger.com> on 2016-08-25T17:48:30Z * Committed by spalger <spalger@users.noreply.github.com> on 2016-12-02T23:41:40Z **Commit 5:** reindent to satisfy new indentation check algorithm * Original sha: b31dae1 * Authored by spalger <email@spalger.com> on 2016-08-25T17:49:47Z * Committed by spalger <spalger@users.noreply.github.com> on 2016-12-02T23:41:58Z **Commit 6:** place missing semicolon * Original sha: 7b39475 * Authored by spalger <email@spalger.com> on 2016-09-06T22:27:10Z * Committed by spalger <spalger@users.noreply.github.com> on 2016-12-02T23:42:04Z **Commit 7:** ignore copy-pasted decode geohash code * Original sha: 3c02df9 * Authored by spalger <email@spalger.com> on 2016-09-10T01:49:42Z * Committed by spalger <spalger@users.noreply.github.com> on 2016-12-02T23:42:04Z **Commit 8:** Merge branch 'master' of github.com:elastic/kibana into upgrade/eslint-try2 * Original sha: 1224b18 * Authored by spalger <spalger@users.noreply.github.com> on 2016-12-10T04:14:32Z **Commit 9:** [grunt/eslint] fix argument spacing * Original sha: 6fa2c6c * Authored by spalger <spalger@users.noreply.github.com> on 2016-12-10T04:22:42Z **Commit 10:** [gurnt/eslint] add comment about contents of report * Original sha: 71834ca * Authored by spalger <spalger@users.noreply.github.com> on 2016-12-10T07:59:11Z **Commit 11:** Merge branch 'master' of github.com:elastic/kibana into upgrade/eslint-try2 * Original sha: 76e77a7 * Authored by spalger <spalger@users.noreply.github.com> on 2016-12-12T20:17:05Z **Commit 12:** [grunt/tasks] use `export default` * Original sha: 803c0da * Authored by spalger <spalger@users.noreply.github.com> on 2016-12-12T20:19:27Z

upgrade eslint, all related deps, and config files

054e798

spalger added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc review v5.2.0 v6.0.0 labels Dec 2, 2016

spalger assigned epixa and cjcenizal Dec 2, 2016

spalger added 6 commits December 2, 2016 16:41

replace gruntify-eslint with basic eslint-cli wrapper

71732e7

arrow-IIFEs must be invoked outside of the parens

b05662c

move import statements before their use

3572ab8

reindent to satisfy new indentation check algorithm

b31dae1

place missing semicolon

7b39475

ignore copy-pasted decode geohash code

3c02df9

spalger force-pushed the upgrade/eslint-try2 branch from 733cc1b to 3c02df9 Compare December 2, 2016 23:42

spalger requested review from epixa and cjcenizal December 10, 2016 00:45

cjcenizal reviewed Dec 10, 2016

View reviewed changes

spalger added 2 commits December 9, 2016 21:14

Merge branch 'master' of github.com:elastic/kibana into upgrade/eslin…

1224b18

…t-try2

[grunt/eslint] fix argument spacing

6fa2c6c

cjcenizal approved these changes Dec 10, 2016

View reviewed changes

[gurnt/eslint] add comment about contents of report

71834ca

epixa reviewed Dec 12, 2016

View reviewed changes

tasks/eslint.js

fix: false

};

module.exports = grunt => {

Copy link

Contributor

epixa Dec 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export default?

spalger reacted with thumbs up emoji

epixa suggested changes Dec 12, 2016

View reviewed changes

spalger added 2 commits December 12, 2016 13:17

Merge branch 'master' of github.com:elastic/kibana into upgrade/eslin…

76e77a7

…t-try2

[grunt/tasks] use export default

803c0da

epixa approved these changes Dec 12, 2016

View reviewed changes

spalger merged commit 0c73672 into elastic:master Dec 12, 2016

elastic-jasper mentioned this pull request Dec 12, 2016

[5.x] Upgrade eslint #9449

Merged

spalger deleted the upgrade/eslint-try2 branch December 13, 2016 00:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade eslint #9357

Upgrade eslint #9357

spalger commented Dec 2, 2016 •

edited by epixa

Loading

cjcenizal commented Dec 10, 2016

cjcenizal left a comment

cjcenizal Dec 10, 2016

cjcenizal Dec 10, 2016

cjcenizal Dec 10, 2016

spalger Dec 10, 2016

cjcenizal Dec 10, 2016

spalger Dec 12, 2016

cjcenizal left a comment

epixa commented Dec 12, 2016

epixa Dec 12, 2016

spalger Dec 12, 2016

epixa Dec 12, 2016

epixa Dec 12, 2016

epixa Dec 12, 2016

spalger Dec 12, 2016

epixa Dec 12, 2016

epixa Dec 12, 2016

epixa left a comment

spalger commented Dec 12, 2016

epixa commented Dec 12, 2016

Upgrade eslint #9357

Upgrade eslint #9357

Conversation

spalger commented Dec 2, 2016 • edited by epixa Loading

cjcenizal commented Dec 10, 2016

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

epixa commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epixa left a comment

Choose a reason for hiding this comment

spalger commented Dec 12, 2016

epixa commented Dec 12, 2016

spalger commented Dec 2, 2016 •

edited by epixa

Loading