[eslint] enable no undef #10825

spalger · 2017-03-21T01:26:17Z

Re-enabled the no-undef eslint rule and fixed all the violation using a mixture of jscodeshift transforms and manual adjustments.

w33ble · 2017-03-21T18:02:51Z

src/cli/cluster/cluster_manager.js

@@ -83,8 +80,8 @@ module.exports = class ClusterManager {
  }

  setupWatching(extraPaths) {
-    const chokidar = require('chokidar');
-    const fromRoot = require('../../utils/from_root');
+    const chokidar = require('chokidar'); // kibana-jscodeshift-ignore


// kibana-jscodeshift-ignore

What is this?

It's a signal to the jscodeshift transforms that the require statement on that line needs to stay on that line, I planned to remove them before submitting the pr

w33ble · 2017-03-21T18:03:29Z

src/cli/serve/__tests__/read_yaml_config.js


 function fixture(name) {
  return resolve(__dirname, 'fixtures', name);
 }

 describe('cli/serve/read_yaml_config', function () {
  it('reads a single config file', function () {
-    const config = readYamlConfig(fixture('one.yml'));
+    readYamlConfig(fixture('one.yml'));


Do we need this call anymore? Are there other places where we have situations like this?

Doesn't look like it

w33ble · 2017-03-21T18:03:53Z

src/cli/serve/__tests__/reload_logging_config.js

@@ -33,7 +33,7 @@ describe(`Server logging configuration`, function () {

      let asserted = false;
      let json = Infinity;
-      const conf = setLoggingJson(true);
+      setLoggingJson(true);


Do we need this call anymore? What is this doing?

Yeah, this is writing a config file that is then read by the server on startup.

kimjoar · 2017-03-21T15:36:27Z

src/cli/cluster/cluster_manager.js

@@ -83,8 +80,8 @@ module.exports = class ClusterManager {
  }

  setupWatching(extraPaths) {
-    const chokidar = require('chokidar');
-    const fromRoot = require('../../utils/from_root');
+    const chokidar = require('chokidar'); // kibana-jscodeshift-ignore


What's kibana-jscodeshift-ignore?

Ah, ignore require -> import?

kimjoar · 2017-03-21T15:46:33Z

src/cli_plugin/install/pack.js

@@ -17,7 +17,7 @@ async function listPackages(settings) {
    .map(file => file.replace(/\\/g, '/'))
    .map(file => file.match(regExp))
    .compact()
-    .map(([ file, _, folder ]) => ({ file, folder }))


Another thing that eslint complains about. If we really don't like this style I think we should just stop destructuring the arg.

Ah, true. I'll live with it. It's just JS being weird.

kimjoar · 2017-03-21T15:49:44Z

src/core_plugins/console/public/src/autocomplete/url_params.js

@@ -37,13 +37,13 @@ function UrlParams(description, defaults) {
  description = _.clone(description || {});
  _.defaults(description, defaults);
  _.each(description, function (p_description, param) {
-    var values, component;
+    var component;
    component = new ParamComponent(param, this.rootComponent, p_description);


nitpick: could be changed to const component = new ParamComponent(...) now

kimjoar · 2017-03-21T15:51:14Z

src/core_plugins/console/public/src/sense_editor/mode/output.js

@@ -10,7 +9,7 @@ var HighlightRules = require("./output_highlight_rules").OutputJsonHighlightRule
 var MatchingBraceOutdent = ace.require("ace/mode/matching_brace_outdent").MatchingBraceOutdent;
 var CstyleBehaviour = ace.require("ace/mode/behaviour/cstyle").CstyleBehaviour;
 var CStyleFoldMode = ace.require("ace/mode/folding/cstyle").FoldMode;
-var WorkerClient = ace.require("ace/worker/worker_client").WorkerClient;


This require has side-effects?

I'm not sure, playing it safe

kimjoar · 2017-03-21T15:53:02Z

src/core_plugins/console/public/tests/tests.js

@@ -2,10 +2,8 @@ require('ace');

 const module = require('ui/modules').get('app/sense');

-module.run(function (Private, $rootScope) {
-  module.setupResizeCheckerForRootEditors = ($el, ...editors) => {
-    // mock the resize checker


Maybe keep the comment? (can put it above instead of within)

kimjoar · 2017-03-21T18:15:51Z

src/core_plugins/metrics/public/visualizations/components/flot_chart.js

-      const { series, markings } = newProps;
+      const {
+        series
+      } = newProps;


nitpick: styling

kimjoar · 2017-03-21T18:17:06Z

src/core_plugins/metrics/public/visualizations/components/timeseries.js

@@ -68,7 +68,7 @@ class Timeseries extends Component {
              if (row.data[i] && pos.x < row.data[i][0]) break;
            }
            if (!row.data[closest]) return values[row.id] = null;
-            const [ time, value ] = row.data[closest];
+            const [ , value ] = row.data[closest];


hm, I think this is actually legal, but for clarity we probably want to keep time here

eslint complains, and I think that's the right thing to do unless we want to pick a variable pattern we can use for unused placeholders (something we can tell eslint to ignore, like unused{N})

Maybe it should just be const value = row.data[closest][1]? I just found this a bit "confusing" (as in: I wasn't sure whether or not it was actually legal, which I find to be a bad sign)

(However: I'm absolutely okey with keeping it, I think enabling this rule in eslint is much more important than discussing this case)

kimjoar · 2017-03-21T19:18:15Z

src/core_plugins/timelion/server/lib/load_functions.js

@@ -23,7 +22,6 @@ module.exports = function (directory) {
  })
  .map(function (file) {
    const parts = file.split('/');
-    const name = parts[parts.length - 2];
    return getTuple(directory, parts[parts.length - 2]);


Maybe getTuple(directory, name) instead of removing name?

kimjoar · 2017-03-21T19:39:49Z

src/ui/public/directives/__tests__/parse_query.js

@@ -25,14 +22,12 @@ const init = function () {
  // Load the application
  ngMock.module('kibana');

-  ngMock.module('kibana', function ($provide) {
+  ngMock.module('kibana', function () {
  });


not needed anymore?

kimjoar · 2017-03-21T20:03:20Z

test/support/page_objects/dashboard_page.js

@@ -44,8 +44,7 @@ export default class DashboardPage {
      await PageObjects.common.try(async () => {
        const goToDashboardLink = await PageObjects.common.findByCssSelector('a[href="#/dashboard"]');
        await goToDashboardLink.click();
-        // Once the searchFilter can be found, we know the page finished loading.
-        const searchFilter = await PageObjects.common.findTestSubject('searchFilter');


keep the comment?

spalger · 2017-03-22T02:16:17Z

@w33ble @kjbekkelund pushed updates to address feedback if you would like to take another look

kimjoar · 2017-03-22T02:19:21Z

src/core_plugins/metrics/public/components/vis_types/top_n/series.js

  } = props;

  const aggs = model.metrics.map(createAggRowRender(props));

-  let caretClassName = 'fa fa-caret-down';
-  if (!visible) caretClassName = 'fa fa-caret-right';
+  !visible;


Now commenting on the correct line 🙈 This one is weird ^

thomasneirynck · 2017-03-22T03:36:47Z

src/core_plugins/kibana/public/management/sections/indices/_edit.js

@@ -64,7 +64,8 @@ uiModules.get('apps/management')
  });

  $scope.$watchCollection('indexPattern.fields', function () {
-    $scope.indexPattern.fields.filter(() => { type: 'conflict'; });
+    $scope.conflictFields = $scope.indexPattern.fields
+      .filter(field => field.type === 'conflict');


thanks @spalger!

kimjoar

As far as I can see this looks great now.

epixa · 2017-03-22T14:37:02Z

Can we do this change to 5.x as well? @jgowdyelastic Would this be a problem for ML? I think this is already happening on x-pack-kibana, but I figured I'd confirm anyway.

stacey-gammon · 2017-03-22T15:20:53Z

Hm, I'm not sure how this passed, I am getting build failures on a PR for code I didn't touch:
https://kibana-ci.elastic.co/job/elastic+kibana+pull-request+multijob-intake/4866/console

e.g. this file still seems to have issues:
https://github.com/elastic/kibana/blob/master/src/core_plugins/metrics/public/lib/__tests__/add_scope.js

@spalger the rule doesn't seem to pick up when a component is used in inlined html. e.g.:

  it('adds $scope variables as props to wrapped component', () => {
    const WrappedComponent = addScope(Component, $scope, ['testOne', 'testTwo']);
    const wrapper = shallow(<WrappedComponent/>);
    expect(wrapper.state('testOne')).to.equal(1);
    expect(wrapper.state('testTwo')).to.equal(2);
  });

the WrappedComponent is throwing the error, but I think it's still needed because it's used in the < tags /> Did you encounter this?

* [codeshift] add proper ignore comments * use more descriptive file ignore pattern * [codeshift] apply require-to-import transform * [codeshift/fixup] remove duplicate imports * [eslint] upgrade config for react "unused" support (cherry picked from commit aa2bb17) * [eslint] remove no-unused-vars override * [eslint] remove no-unused-vars override * add eslint-plugin-react peerDependency * [codeshift] apply remove-unused-basic-requires transform * [codeshift] apply remove-unused-function-arguments transform * [lintroller] fix argument list spacing * [codeshift] apply remove-unused-basic-vars transform * [codeshift/fixup] fixup unused basic var removals * manually apply remove-unused-assignments transform * [codeshift] reapply remove-unused-imports transform * [codeshift] reapply remove-unused-function-arguments transform * [resizeChecker] remove assignment to unused var * [eslint] autofix param spacing * manually fix remaining no-undef errors * replace values that looked unused in tests * remove // kibana-jscodeshift-no-babel comment * remove import statements from code required by api tests * Remove '// kibana-jscodeshift-ignore' comments * address review feedback * remove remnant of removed if condition * [console] use * import for settings

spalger added 19 commits March 20, 2017 16:03

[codeshift] add proper ignore comments

8b74e23

[codeshift] apply require-to-import transform

476178d

[codeshift/fixup] remove duplicate imports

c5feea6

[eslint] upgrade config for react "unused" support

aa2bb17

[codeshift] apply remove-unused-imports transform

9da354a

[codeshift] apply remove-unused-basic-requires transform

23ed1d2

[codeshift] apply remove-unused-function-arguments transform

f90e6c1

[lintroller] fix argument list spacing

bfe0cac

[codeshift] apply remove-unused-basic-bars transform

c07e840

[codeshift/fixup] fixup unused basic var removals

8f0214b

manually apply remove-unused-assignments transform

4b471a5

[codeshift] reapply remove-unused-imports transform

e2c149a

[codeshift] reapply remove-unused-function-arguments transform

4cd362b

[eslint] autofix param spacing

0df17de

manually fix remaining no-undef errors

337a465

use more descriptive file ignore pattern

cabd018

add eslint-plugin-react peerDependency

74cad8c

replace values that looked unused in tests

3588734

remove // kibana-jscodeshift-no-babel comment

2dd2761

spalger force-pushed the eslint/enable-no-undef branch from ed1bceb to 2dd2761 Compare March 21, 2017 03:28

remove import statements from code required by api tests

c3cfac3

spalger requested a review from kimjoar March 21, 2017 09:59

spalger changed the title ~~[WIP][Eslint] enable no undef~~ [eslint] enable no undef Mar 21, 2017

spalger added the review label Mar 21, 2017

w33ble reviewed Mar 21, 2017

View reviewed changes

kimjoar suggested changes Mar 21, 2017

View reviewed changes

spalger added 2 commits March 21, 2017 19:05

Remove '// kibana-jscodeshift-ignore' comments

b8958ea

address review feedback

9af995e

kimjoar reviewed Mar 22, 2017

View reviewed changes

thomasneirynck reviewed Mar 22, 2017

View reviewed changes

remove remnant of removed if condition

c987885

kimjoar approved these changes Mar 22, 2017

View reviewed changes

spalger merged commit d8d6552 into elastic:master Mar 22, 2017

This was referenced Mar 22, 2017

[backport/5.x] [eslint] enable no-undef rule - #10825 #10846

Merged

[eslint] remove unused vars #10847

Merged

[backport/5.3] [eslint] enable no-undef rule - #10825 #10848

Closed

kimjoar mentioned this pull request Mar 23, 2017

ESLint should catch unused vars #7609

Closed

spalger deleted the eslint/enable-no-undef branch March 29, 2017 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[eslint] enable no undef #10825

[eslint] enable no undef #10825

spalger commented Mar 21, 2017 •

edited

Loading

w33ble Mar 21, 2017

spalger Mar 21, 2017 •

edited

Loading

w33ble Mar 21, 2017

spalger Mar 21, 2017

w33ble Mar 21, 2017 •

edited

Loading

spalger Mar 21, 2017

kimjoar Mar 21, 2017

kimjoar Mar 21, 2017

kimjoar Mar 21, 2017

spalger Mar 21, 2017

kimjoar Mar 21, 2017

kimjoar Mar 21, 2017

kimjoar Mar 21, 2017

spalger Mar 21, 2017 •

edited

Loading

kimjoar Mar 21, 2017

kimjoar Mar 21, 2017

spalger Mar 22, 2017

kimjoar Mar 21, 2017

kimjoar Mar 21, 2017

spalger Mar 21, 2017

kimjoar Mar 21, 2017

kimjoar Mar 21, 2017

kimjoar Mar 21, 2017

kimjoar Mar 21, 2017

spalger commented Mar 22, 2017

kimjoar Mar 22, 2017

thomasneirynck Mar 22, 2017

kimjoar left a comment

epixa commented Mar 22, 2017

stacey-gammon commented Mar 22, 2017 •

edited

Loading

[eslint] enable no undef #10825

[eslint] enable no undef #10825

Conversation

spalger commented Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

spalger Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble Mar 21, 2017 • edited Loading

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

Choose a reason for hiding this comment

spalger Mar 21, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Mar 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimjoar left a comment

Choose a reason for hiding this comment

epixa commented Mar 22, 2017

stacey-gammon commented Mar 22, 2017 • edited Loading

spalger commented Mar 21, 2017 •

edited

Loading

spalger Mar 21, 2017 •

edited

Loading

w33ble Mar 21, 2017 •

edited

Loading

spalger Mar 21, 2017 •

edited

Loading

stacey-gammon commented Mar 22, 2017 •

edited

Loading