Default to named ui public n to z #11221

stacey-gammon · 2017-04-13T14:40:18Z

Related to #10981. Converting rest of ui/public to using named exports only. Default exports may still be around where they are used prolifically, or in x-pack.

Background: #8641

stacey-gammon · 2017-04-13T17:28:29Z

Hmm, error looks strange:

Running "checkPlugins" task

Running "esvm:test" (esvm) task
starting up "test" cluster
INFO -  - cluster - Downloading & installing from "master" branch.
Fatal error: unexpected end of file�
runbld>>> <<<<<<<<<<<< SCRIPT EXECUTION END <<<<<<<<<<<<
runbld>>> DURATION: 257349ms
runbld>>> STDOUT: 86987 bytes
runbld>>> STDERR: 1929 bytes
runbld>>> WRAPPED PROCESS: FAILURE (3)

jenkins, test this

stacey-gammon · 2017-04-13T18:24:33Z

Looks like another invalid failure, different one this time:

Running "functionalTestRunner" task
 debg  Loading config file from "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/config.js"
 info  Config loaded
  log   [17:49:27.198] [info][status][plugin:[email protected]] Status changed from yellow to yellow - No existing Kibana index found
INFO - -woNMIT - meta_data_create_index_service - [.kibana] creating index, cause [api], templates [], shards [1]/[1], mappings [server, config]
  log   [17:49:27.621] [info][status][plugin:[email protected]] Status changed from yellow to green - Kibana index ready
  log   [17:49:27.621] [info][status][ui settings] Status changed from yellow to green - Ready
Warning: Error: remote failed to start within 2 minutes
    at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/services/remote/leadfoot_command.js:13:13
    at undefined.next (native)
    at step (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/services/remote/leadfoot_command.js:5:1)
    at /var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/services/remote/leadfoot_command.js:5:1� Use --force to continue.

Aborted due to warnings.

jenkins, test this

stacey-gammon · 2017-04-14T14:08:56Z

jenkins, test this

(functional runner failed to start)

stacey-gammon · 2017-04-18T13:56:56Z

Build error:

Chrome 57.0.2987 (Linux 0.0.0) kibana_map tests KibanaMap - baseLayer WMS FAILED
	Error: timeout of 10000ms exceeded. Ensure the done() callback is being called in this test.
Chrome 57.0.2987 (Linux 0.0.0): Executed 485 of 719 (1 FAILED) (0 secs / 16.69 secs)

I think I've seen this one before and it's flaky (exceeds the timeout anyway, doesn't actually fail...)

jenkins, test this.

ycombinator

Amazing work, as before! I left a few comments but overall LGTM!

ycombinator · 2017-04-18T23:07:10Z

src/ui/public/agg_types/__tests__/agg_param_writer.js

@@ -1,12 +1,12 @@
 import _ from 'lodash';
-import VisProvider from 'ui/vis';
+import { VisProvider } from 'ui/vis';
 import AggTypesIndexProvider from 'ui/agg_types/index';


Any reason this one didn't change?

ycombinator · 2017-04-18T23:19:38Z

src/ui/public/agg_types/__tests__/buckets/date_histogram/_params.js

@@ -5,7 +5,8 @@ import ngMock from 'ng_mock';
 import AggParamWriterProvider from '../../agg_param_writer';
 import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
 import AggTypesIndexProvider from 'ui/agg_types/index';


Any reason this one didn't change?

ycombinator · 2017-04-18T23:24:23Z

src/ui/public/agg_types/__tests__/param_types/_calculate_interval.js

@@ -1,7 +1,7 @@
 import _ from 'lodash';
 import expect from 'expect.js';
 import ngMock from 'ng_mock';
-import VisProvider from 'ui/vis';
+import { VisProvider } from 'ui/vis';
 import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
 import AggTypesParamTypesCalculateIntervalProvider from 'ui/agg_types/param_types/_calculate_interval';


AFAICT, there is no file corresponding to ui/agg_types/param_types/_calculate_interval in the source code. I looked in https://github.com/elastic/kibana/tree/master/src/ui/public/agg_types/param_types. How is this even working? 😄

Hah, interesting. Looks like this test is never being run because it's not mentioned in the index.js file. I'll remove the file in a separate PR, so the vis team can take a more focused look and in case we ever need to bring it back, it's decoupled from this monster PR. :)

ycombinator · 2017-04-18T23:30:51Z

src/ui/public/time_buckets/calc_es_interval.js

@@ -45,3 +46,6 @@ export default function () {

  return esDuration;
 }
+
+// used by x-pack. TODO: switch to named and remove.


I'd suggest removing this comment and others like it that reference x-pack from OSS Kibana source code. When we see these default imports in x-pack-kibana, we'll know which ones to come back and cleanup in OSS Kibana.

ycombinator · 2017-04-18T23:34:20Z

src/ui/public/vis/agg_configs.js

@@ -9,9 +9,10 @@

 import _ from 'lodash';
 import { IndexedArray } from 'ui/indexed_array';
-import VisAggConfigProvider from 'ui/vis/agg_config';
+import { VisAggConfigProvider } from 'ui/vis/agg_config';
 import AggTypesIndexProvider from 'ui/agg_types/index';


Any reason not to change this?

Looks like this file got left out of the a - m chunk. I'll add the conversion in here. It still works bc I kept the default export around in that file.

ycombinator · 2017-04-18T23:34:40Z

src/ui/public/vis/vis.js

@@ -10,13 +10,13 @@

 import _ from 'lodash';
 import AggTypesIndexProvider from 'ui/agg_types/index';


Any reason not to change this?

ycombinator · 2017-04-18T23:40:45Z

src/ui/public/vislib/lib/data.js

-import VislibComponentsLabelsLabelsProvider from '../components/labels/labels';
+import { VislibComponentsZeroInjectionInjectZerosProvider } from '../components/zero_injection/inject_zeros';
+import { VislibComponentsZeroInjectionOrderedXKeysProvider } from '../components/zero_injection/ordered_x_keys';
+import { VislibComponentsLabelsLabelsProvider } from '../components/labels/labels';
 import VislibComponentsColorColorProvider from 'ui/vis/components/color/color';


Any reason not to change this?

Initially because I was waiting for this PR to get merged: #11251

But now it's merged so I'll convert.

ycombinator

LGTM!

* Merge * fix * Update resize checker which is in a different location than master

stacey-gammon force-pushed the deafult-to-named-ui-public-n-to-z branch from 74578ae to e0faa74 Compare April 13, 2017 16:01

stacey-gammon force-pushed the deafult-to-named-ui-public-n-to-z branch from 06c266a to d593c33 Compare April 13, 2017 20:37

stacey-gammon force-pushed the deafult-to-named-ui-public-n-to-z branch 6 times, most recently from 7c4b5dc to 09cc7e3 Compare April 17, 2017 20:20

stacey-gammon requested a review from ycombinator April 18, 2017 11:12

stacey-gammon force-pushed the deafult-to-named-ui-public-n-to-z branch from 5b1a269 to f186247 Compare April 18, 2017 13:07

stacey-gammon force-pushed the deafult-to-named-ui-public-n-to-z branch from f186247 to 9327c55 Compare April 18, 2017 15:59

stacey-gammon added 15 commits April 18, 2017 12:22

notify/notifier

0254823

Notify and uiRegistry

c46bbbb

Registry/*

d13d83a

saved_object_registry and routes

4c2a098

ui/public/state_management/*

27bcd3f

stringify

131814f

ui/public/template_vis_type

981ea78

ui/public/time_buckets

a41403a

ui/public/time*

e3328b7

ui/public/utils

70ad39f

ui/public/vis/.

0869d5e

ui/public/vis/components/tooltip

7bfabe8

ui/public/vis_maps/*

4621a48

ui/public/vislib/*

c40a78c

Add a named export for uiModules in prep for that monolith conversion.

721b18a

Fix from merge

c4621e0

stacey-gammon force-pushed the deafult-to-named-ui-public-n-to-z branch from 9327c55 to c4621e0 Compare April 18, 2017 16:24

ycombinator requested changes Apr 18, 2017

View reviewed changes

address code review comments

847a04b

stacey-gammon added chore v5.5.0 review labels Apr 19, 2017

ycombinator approved these changes Apr 19, 2017

View reviewed changes

stacey-gammon merged commit 3eeecb7 into elastic:master Apr 19, 2017

stacey-gammon mentioned this pull request Apr 19, 2017

[5.x] Default to named ui public n to z #11221 #11320

Merged

stacey-gammon added a commit that referenced this pull request Apr 19, 2017

[5.x] Default to named ui public n to z #11221 (#11320)

72bdeee

* Merge * fix * Update resize checker which is in a different location than master

stacey-gammon deleted the deafult-to-named-ui-public-n-to-z branch April 19, 2017 19:53

w33ble added a commit to elastic/template-kibana-plugin that referenced this pull request Jun 15, 2017

update template for elastic/kibana#11221

5a252fd

w33ble mentioned this pull request Jun 15, 2017

Cannot access my plugin elastic/generator-kibana-plugin#52

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default to named ui public n to z #11221

Default to named ui public n to z #11221

stacey-gammon commented Apr 13, 2017 •

edited

Loading

stacey-gammon commented Apr 13, 2017

stacey-gammon commented Apr 13, 2017

stacey-gammon commented Apr 14, 2017

stacey-gammon commented Apr 18, 2017

ycombinator left a comment

ycombinator Apr 18, 2017

ycombinator Apr 18, 2017

ycombinator Apr 18, 2017 •

edited

Loading

stacey-gammon Apr 19, 2017

ycombinator Apr 18, 2017

ycombinator Apr 18, 2017

stacey-gammon Apr 19, 2017

ycombinator Apr 18, 2017

ycombinator Apr 18, 2017

stacey-gammon Apr 19, 2017

ycombinator left a comment

		@@ -10,13 +10,13 @@

		import _ from 'lodash';
		import AggTypesIndexProvider from 'ui/agg_types/index';

Default to named ui public n to z #11221

Default to named ui public n to z #11221

Conversation

stacey-gammon commented Apr 13, 2017 • edited Loading

stacey-gammon commented Apr 13, 2017

stacey-gammon commented Apr 13, 2017

stacey-gammon commented Apr 14, 2017

stacey-gammon commented Apr 18, 2017

ycombinator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator Apr 18, 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

ycombinator left a comment

Choose a reason for hiding this comment

stacey-gammon commented Apr 13, 2017 •

edited

Loading

ycombinator Apr 18, 2017 •

edited

Loading