Upgrade eslint by spalger · Pull Request #9357 · elastic/kibana

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

  },
  "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

    "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

            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

## Dependency updates - `@elastic/eui`: v112.2.0 ⏩ v112.3.0 - `@elastic/eslint-plugin-eui`: v2.7.0 ⏩ v2.8.0 --- ## Package updates ### `@elastic/eui` [v112.3.0](https://github.com/elastic/eui/releases/tag/v112.3.0) - Added new `server` icon. ([#9355](elastic/eui#9355)) - Added `className` support to `EuiMarkdownEditor`'s `toolbarProps` for custom toolbar styling ([#9349](elastic/eui#9349)) - Updated `EuiFilePicker` to use the `upload` icon to better indicate uploads. ([#9351](elastic/eui#9351)) - Exported the flyout system store singleton and added an event observer for emitting close session events ([#9347](elastic/eui#9347)) - Updated `EuiIcon` to use standard dynamic imports for icon assets, enabling native support for modern bundlers (Rollup, Parcel) and improving initial load performance ([#9342](elastic/eui#9342)) **Bug fixes** - Fixed a potential crash in the flyout system: due to asynchronous state updates and React's batching behavior, it was possible to experience a crash when closing a managed flyout. ([#9356](elastic/eui#9356)) ### `@elastic/eslint-plugin-eui` v2.8.0 - Added new `icon-accessibility-rules` rule. ([#9357](elastic/eui#9357)) - Added new `badge-accessibility-rules` rule. ([#9354](elastic/eui#9354))

upgrade eslint, all related deps, and config files

054e798

spalger added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// 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 cjcenizal and epixa 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

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

Conversation

spalger commented Dec 2, 2016 • edited by epixa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjcenizal commented Dec 10, 2016

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

epixa commented Dec 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epixa left a comment

Choose a reason for hiding this comment

Uh oh!

spalger commented Dec 12, 2016

Uh oh!

epixa commented Dec 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

spalger commented Dec 2, 2016 •

edited by epixa

Loading