removes angular from fieldFormatts #17581

ppisljar · 2018-04-05T17:24:12Z

Angular dependency is removed from field formatts (pretty much just removing the provider wrapper)
FieldFormatterRegistry is refactored to not depend on angular

ppisljar · 2018-04-05T17:28:06Z

src/ui/public/registry/field_formats.js

-    function init() {
-      config.watch('format:defaultTypeMap', parseDefaultTypeMap);
-    }
+    this._config.subscribe(({ key, newValue }) => {


@spalger: i am not sure why the watch was used here and i couldn't get this subscribe to fire ... do we need this ? if so does it look correct ? when would we need to unsubscribe ?

It is called whenever the uiSettings change, seems to be working when I test it:

when would we need to unsubscribe ?

That's a good question... I don't think we need to unsubscribe since the fieldFormatsRegistry is never torn down, but I'm concerned how that will work with tests, especially since the uiSettingsClient is currently stubbed out for each test https://github.com/elastic/kibana/blob/master/src/ui/public/test_harness/test_harness.js#L33-L49

How about we add this as another method on the chrome object, like https://github.com/elastic/kibana/blob/master/src/ui/public/chrome/api/ui_settings.js#L12-L14, and then stub it out in the tests just like we do for the uiSettings client. Then I'll prioritize migrating the uiSettings and fieldFormats services over to the new platform once we have that plumbing in place.

elasticmachine · 2018-04-05T18:04:20Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

spalger · 2018-04-05T21:48:09Z

src/ui/public/registry/field_formats.js

-      createSomeFormat(FieldFormat)
-    ));
-  },
+    this._config = chrome.getUiSettingsClient();


I know it used to be the config service, but now that this class is using the uiSettings client can we reflect that in the property naming? this._uiSettings?

spalger · 2018-04-05T22:00:15Z

src/ui/public/vis/__tests__/_agg_config.js


  beforeEach(ngMock.module('kibana'));
  beforeEach(ngMock.inject(function (Private) {
    Vis = Private(VisProvider);
    AggType = Private(AggTypesAggTypeProvider);
    AggConfig = Private(VisAggConfigProvider);
    indexPattern = Private(FixturesStubbedLogstashIndexPatternProvider);
-    fieldFormat = Private(RegistryFieldFormatsProvider);


If we move the fieldFormat registry to be like the uiSettings client, we'll want to keep the fieldFormat = chrome.getFieldFormatsRegistry() at this point so that it gets the unique registry for this test.

elasticmachine · 2018-04-09T13:44:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-09T18:16:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

spalger · 2018-04-09T20:06:19Z

src/ui/public/test_harness/test_harness.js

-        return { settings: stubUiSettings.getAll() };
-      }
+// stub the UiSettingsClient
+if (chrome.getUiSettingsClient.restore) {


This check is only necessary if we are calling this logic multiple times.

elasticmachine · 2018-04-09T23:37:19Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-10T16:03:00Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-10T17:49:28Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-10T19:11:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger

Looking good, just a couple lame naming asks

spalger · 2018-04-10T21:03:18Z

src/ui/public/registry/field_formats.js

-    init();
-  }
-});
+export const registryFieldFormats = new FieldFormatRegistry();


registryFieldFormats seems backwards, can we please just call this fieldFormats like we do nearly everywhere we use it?

spalger · 2018-04-10T23:53:08Z

src/ui/public/registry/field_formats.js

-  name: 'fieldFormats',
-  index: ['id'],
-  group: ['fieldType'],
+class FieldFormatRegistry extends IndexedArray {


I think we should move this into ui/field_formats/field_formats.js. IMO this isn't really a registry, and that's fine but we might as well keep it next to the rest of the field formats code.

that folder doesn't exist so i'll leave it where it is for now

elasticmachine · 2018-04-11T16:03:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes · 2018-04-12T11:13:29Z

src/ui/public/vis/__tests__/_agg_config.js

@@ -5,23 +5,22 @@ import { VisProvider } from 'ui/vis';
 import { AggTypesAggTypeProvider } from 'ui/agg_types/agg_type';
 import { VisAggConfigProvider } from 'ui/vis/agg_config';
 import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern';
-import { RegistryFieldFormatsProvider } from 'ui/registry/field_formats';
+import { fieldFormats } from 'ui/registry/field_formats';


Just use import { fieldFormats as fieldFormat } then we can get rid of the const fieldFormat = fieldFormats in this file ... (or use search and replace :D)

timroes · 2018-04-12T12:23:21Z

src/ui/public/registry/field_formats.js

-    const getConfig = (...args) => config.get(...args);
-    const self = this;
-    let defaultMap;
+    this._uiSettings.subscribe(({ key, newValue }) => {


I wonder whether we maybe should introduce a subscribeTo('key', (newVal) => {}) to UiSettingsClient, since it seems for me like most people who want to subscribe want to subscribe to specific changes in the long run, and all just having that if statement then. @spalger

Though just a thought nothing to change in this PR.

timroes

LGTM, if Spencer approves the way test harness is handled.

elasticmachine · 2018-04-12T12:45:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ppisljar added WIP Work in progress Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v7.0.0 v6.4.0 labels Apr 5, 2018

ppisljar requested review from timroes, spalger and thomasneirynck April 5, 2018 17:24

ppisljar commented Apr 5, 2018

View reviewed changes

spalger reviewed Apr 5, 2018

View reviewed changes

spalger mentioned this pull request Apr 6, 2018

[testHarnes/uiSettings] create stub uiSettings client with actual state #17588

Closed

ppisljar force-pushed the ang/removeAngularFieldFormatts branch from 4120bc9 to f41220b Compare April 9, 2018 12:34

spalger reviewed Apr 9, 2018

View reviewed changes

ppisljar force-pushed the ang/removeAngularFieldFormatts branch 2 times, most recently from a8b7e0b to d4a8411 Compare April 9, 2018 23:00

ppisljar added 5 commits April 10, 2018 12:00

removing angular from FieldFormatts

0863ef0

rename _config to _uiSettings

9d32926

fixing tests

a601383

moving uiSettings test stub out of beforeEach

b38aaed

fixing tests

69814df

ppisljar force-pushed the ang/removeAngularFieldFormatts branch from c621654 to 69814df Compare April 10, 2018 17:00

adding fieldType group

dfe4d71

ppisljar added the review label Apr 10, 2018

ppisljar removed the WIP Work in progress label Apr 10, 2018

ppisljar changed the title ~~WIP: removes angular from fieldFormatts~~ removes angular from fieldFormatts Apr 10, 2018

spalger reviewed Apr 10, 2018

View reviewed changes

renaming to fieldFormats

f44720d

thomasneirynck removed their request for review April 11, 2018 20:23

timroes added the chore label Apr 12, 2018

timroes reviewed Apr 12, 2018

View reviewed changes

fieldFormat to fieldFormats

ee2ff3a

timroes reviewed Apr 12, 2018

View reviewed changes

timroes approved these changes Apr 12, 2018

View reviewed changes

ppisljar merged commit adabbf3 into elastic:master Apr 12, 2018

ppisljar added a commit to ppisljar/kibana that referenced this pull request Apr 12, 2018

removes angular from fieldFormatts (elastic#17581)

9a2bb72

ppisljar mentioned this pull request Apr 12, 2018

[6.x] removes angular from fieldFormatts (#17581) #17676

Merged

ppisljar deleted the ang/removeAngularFieldFormatts branch April 12, 2018 12:31

ppisljar added a commit that referenced this pull request Apr 12, 2018

removes angular from fieldFormatts (#17581) (#17676)

3e7b6f0

ppisljar restored the ang/removeAngularFieldFormatts branch September 26, 2018 10:02

timroes mentioned this pull request Dec 12, 2018

Use the Canvas rendering pipeline/expressions for all visualizations #19813

Closed

58 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removes angular from fieldFormatts #17581

removes angular from fieldFormatts #17581

ppisljar commented Apr 5, 2018 •

edited

Loading

ppisljar Apr 5, 2018

spalger Apr 5, 2018 •

edited

Loading

spalger Apr 5, 2018

elasticmachine commented Apr 5, 2018

spalger Apr 5, 2018

spalger Apr 5, 2018

elasticmachine commented Apr 9, 2018

elasticmachine commented Apr 9, 2018

spalger Apr 9, 2018

elasticmachine commented Apr 9, 2018

elasticmachine commented Apr 10, 2018

elasticmachine commented Apr 10, 2018

elasticmachine commented Apr 10, 2018

spalger left a comment •

edited

Loading

spalger Apr 10, 2018

spalger Apr 10, 2018

ppisljar Apr 11, 2018

elasticmachine commented Apr 11, 2018

timroes Apr 12, 2018

timroes Apr 12, 2018

timroes left a comment

elasticmachine commented Apr 12, 2018

removes angular from fieldFormatts #17581

removes angular from fieldFormatts #17581

Conversation

ppisljar commented Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

spalger Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Apr 5, 2018

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Apr 9, 2018

💔 Build Failed

elasticmachine commented Apr 9, 2018

💔 Build Failed

Choose a reason for hiding this comment

elasticmachine commented Apr 9, 2018

💔 Build Failed

elasticmachine commented Apr 10, 2018

💔 Build Failed

elasticmachine commented Apr 10, 2018

💔 Build Failed

elasticmachine commented Apr 10, 2018

💚 Build Succeeded

spalger left a comment • 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

elasticmachine commented Apr 11, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

elasticmachine commented Apr 12, 2018

💔 Build Failed

ppisljar commented Apr 5, 2018 •

edited

Loading

spalger Apr 5, 2018 •

edited

Loading

spalger left a comment •

edited

Loading