Expose field formatters in kibana server #12625

nreese · 2017-06-30T21:56:47Z

Move field formatters from ui/public to common so they can be used in both client and server.

Field formatters are exposed on the server via decorating the server with fieldFormatters which is an instance of FieldFormatsService . FieldFormatsService closely parallels how field formatters are exposed on the client by proving methods like getDefaultInstance and getType.

The FieldFormat classes were also updated. Previously, the FieldFormats exported a function. Executing the function returned the FieldFormat class. Now the FieldFormats directly export a Class.

kobelb

The additional tests are great!! Just a few comments/questions.

kobelb · 2017-07-05T18:55:08Z

src/core_plugins/kibana/common/field_formats/__tests__/field_formats.js

+
+  let fieldFormats;
+  beforeEach(function () {
+    fieldFormats = new FieldFormatsService;


Can we add some parens here so it's new FieldFormatsService()

kobelb · 2017-07-05T19:10:46Z

src/core_plugins/kibana/common/field_formats/__tests__/field_formats.js

+
+  const config = {};
+  config['format:defaultTypeMap'] = {
+    'ip': { 'id': 'ip', 'params': {} },


The items besides number aren't used by this test, can we remove them?

kobelb · 2017-07-05T19:17:50Z

src/core_plugins/kibana/common/field_formats/types/_numeral.js

@@ -1,6 +1,6 @@
 import _ from 'lodash';
-import numeral from 'numeral';
-import { FieldFormat } from 'ui/index_patterns/_field_format/field_format';
+import numeral from '@spalger/numeral';


Should we consider moving this to elastic/numeral now that https://github.com/elastic/numeral-js exists?

I created @elastic/numeral-js and upgraded to numeral 3.0 at the same time, pretty sure we'll have to publish the old version at @spalger/numeral-js in the new package if we want to move it over. That said, I'm not opposed.

Can this wait for a separate PR? This seems like an existing artifact that is outside the scope of this PR and I am trying to get this PR finished ASAP since another feature is dependent on it and Tuesday is feature cutoff.

kobelb · 2017-07-05T19:24:29Z

src/ui/field_formats_mixin.js

+import { getUiSettingDefaults } from '../core_plugins/kibana/ui_setting_defaults';
+
+export function fieldFormatsMixin(kbnServer, server) {
+  server.decorate('server', 'uiSettingDefaults', getUiSettingDefaults());


This doesn't feel like it belongs here, specifically because it's related to uiSettings. I know you're using the uiSettingDefaults with the fieldFormats, but the fieldFormatsMixin really shouldn't be responsible for this.

Additionally, you should be able to do uiSettings.getDefaults(); and not have to expose this separately.

uiSettings.getDefaults() is just a wrapper around options.getDefaults. If options.getDefaults is not provided than the default implementation of '() => ({})' is used. I was unable to figure out how to create a functioning getDefaults implementation to pass to server.uiSettingsServiceFactory that would return the defaults. Please advise on how to implement getDefaults that properly returns the results of getUiSettingDefaults().

If field_formats_mixin is not the place, then where should uiSettingDefaults be exposed to the server?

Never mind. Spencer straightened my out and you are correct. After a rebase, getDefaults is populated and spitting out the correct values. I will remove the uiSettingsDefaults decorator.

kobelb · 2017-07-05T19:32:06Z

src/ui/public/agg_types/buckets/range.js

@@ -1,6 +1,6 @@
 import { AggTypesBucketsBucketAggTypeProvider } from 'ui/agg_types/buckets/_bucket_agg_type';
 import { AggTypesBucketsCreateFilterRangeProvider } from 'ui/agg_types/buckets/create_filter/range';
-import { FieldFormat } from 'ui/index_patterns/_field_format/field_format';
+import { FieldFormat } from '../../../../core_plugins/kibana/common/field_formats/field_format';


Should we be reaching inside the core_plugins/kibana folder from within src/ui/public? What made you and @spalger choose to put the common folder inside the kibana plugin and not a direct descendant of src/? This same issue applies to a few other situations below as well.

kobelb · 2017-07-05T19:39:03Z

src/core_plugins/kibana/common/field_formats/types/boolean.js

@@ -0,0 +1,29 @@
+import { asPrettyString } from '../../utils/as_pretty_string';


Were any changes made to the types that are showing up as additions and not renames in the diff?

The types changed a lot. Instead of being wrapped in a function, now they just export the FieldFormat class

Gotcha, I'll give them a closer look then :)

… uiSettings

kobelb · 2017-07-06T12:09:14Z

src/core_plugins/kibana/common/field_formats/types/color.js

+  ];
+}
+
+ColorFormat.prototype._convert = {


This ColorFormat.prototype._convert should be able to just me moved to a `_convert_method on the ColorFormat class.

I tried to move those under the class with PR - Remove lodash mixin dependencies from Field Formatters but things are messy because _convert is an object and not a function and the way _convert is used in content types make references to this tricky. @stacey-gammon and I decided it was best to leave this alone for now.

Ah... interesting... I'm fine leaving it as is for now in that case.

kobelb · 2017-07-06T12:13:05Z

src/core_plugins/kibana/common/field_formats/types/duration.js

+  static outputFormats = outputFormats;
+}
+
+DurationFormat.prototype.getParamDefaults = function () {


Can we move this to a regular method on the DurationFormat class instead of modifying the prototype as well?

kobelb · 2017-07-06T12:15:22Z

src/core_plugins/kibana/common/field_formats/types/source.js

+  static fieldType = '_source';
+}
+
+SourceFormat.prototype._convert = {


Same comment about not extending the prototype directly.

kobelb · 2017-07-06T12:17:56Z

src/core_plugins/kibana/common/field_formats/types/url.js

+  ];
+}
+
+UrlFormat.prototype._convert = {


Same as above.

spalger

Still working complete testing, but some early feedback

spalger · 2017-07-06T21:57:31Z

src/core_plugins/kibana/common/field_formats/field_formats.js

@@ -0,0 +1,45 @@
+export class FieldFormatsService {


IMO, this belongs in src/ui/field_formats/field_formats_service.js, not the kibana plugin. It's not optional like the kibana plugin theoretically is.

spalger · 2017-07-06T22:05:59Z

src/core_plugins/kibana/common/field_formats/types/__tests__/duration.js

+describe('Duration Format', function () {
+
+  test({ inputFormat: 'seconds', outputFormat: 'humanize' })
+    (-60, 'minus a minute')


Interesting style, but I'm thinking this would be a little less mind bending as:

defineTests({ inputFormat: 'seconds', outputFormat: 'humanize', fixtures: [ [ 60, 'a minute' ], // or { input: -60, output: 'minus a minute' }, ] })

spalger · 2017-07-06T22:28:28Z

src/core_plugins/kibana/common/field_formats/field_formats.js

@@ -0,0 +1,45 @@
+export class FieldFormatsService {
+  constructor() {
+    this.fieldFormats = new Map();


As far as I can tell this is intended for internal use only, it should be prefixed with an _.

This isn't actually a rule in our style guide, right? Airbnb also says not to use the leading underscore (https://github.com/airbnb/javascript#naming--leading-underscore), though we never officially committed to airbnb style guide asfaik.

Maybe we need a "vote" issue for this so we can put something in our style guide, because I see inconsistent use of leading underscores.

spalger · 2017-07-06T22:43:05Z

src/ui/field_formats_mixin.js

+import { FieldFormatsService } from '../core_plugins/kibana/common/field_formats/field_formats';
+
+export function fieldFormatsMixin(kbnServer, server) {
+  server.decorate('server', 'fieldFormats', new FieldFormatsService());


I think we could improve the ergonomics here a bit:

How about making more instances of the FieldFormatService that are configured with a getConfig() function, rather than having a singleton that taked getConfig as an argument?

If this followed the pattern UiSettings uses, and also depended on UiSettings formally, it could look like this:

// for use in the context of a request, the default context server.decorate('request', 'getFieldFormatService', async function () { return await server.fieldFormatServiceFactory(this.getUiSettingsService()) }) // for use outside of the request context, for special cases server.decorate('server', 'fieldFormatServiceFactory', async function (uiSettingsService) { const configVals = await ...; const getConfig = (key) => configVals[key]; return new FieldFormatService({ getConfig }); });

kobelb

LGTM

nreese added the v6.0.0-alpha3 label Jun 30, 2017

nreese requested review from spalger and kobelb July 5, 2017 12:51

kobelb requested changes Jul 5, 2017

View reviewed changes

nreese added 7 commits July 5, 2017 15:41

move field formatters to common

5a4b1f6

expose fieldFormats on server

f7b5205

export Format class from field_formats/types

d27ed54

remove use of window.atob in StringFormat

06b88d2

fix test and move server register under directory field_formats

2156a0a

cleanup

8d455ff

expose uiSettingDefaults on server so fieldFormats knows how to parse…

8f046c8

… uiSettings

nreese force-pushed the server2 branch from 044c612 to 8f046c8 Compare July 5, 2017 21:41

remove uiSettingDefaults decorator and clean up tests

e655ab3

kobelb reviewed Jul 6, 2017

View reviewed changes

src/core_plugins/kibana/common/field_formats/types/url.js

];

}

UrlFormat.prototype._convert = {

Copy link

Contributor

kobelb Jul 6, 2017

Choose a reason for hiding this comment

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

Same as above.

spalger suggested changes Jul 6, 2017

View reviewed changes

nreese added 3 commits July 6, 2017 19:57

move field_formats_service out of kibana plugin

d5d93c8

make duration test more unstandable

ca79831

prefix internal member with underscore

9de5325

spalger mentioned this pull request Jul 7, 2017

[kbnServer/extensions] formalize request factories with helper #12697

Merged

nreese added 3 commits July 6, 2017 21:36

pass getConfig in constructor instead of methods

6d51b7b

move getParamsDefaults for DurationFormat

929429d

add getInstance method to field_formats_server

729a2b5

jimgoodwin added v6.0.0 v6.0.0-beta1 labels Jul 7, 2017

kobelb approved these changes Jul 7, 2017

View reviewed changes

move FieldFormat class outside of kibana plugin

399006c

spalger approved these changes Jul 7, 2017

View reviewed changes

spalger added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jul 7, 2017

nreese merged commit 363a065 into elastic:master Jul 7, 2017

epixa added v6.0.0-beta1 and removed v6.0.0-beta1 v6.0.0-alpha3 labels Jul 10, 2017

spalger mentioned this pull request Nov 16, 2017

Fix fieldFormat plugins #14984

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose field formatters in kibana server #12625

Expose field formatters in kibana server #12625

nreese commented Jun 30, 2017 •

edited

Loading

kobelb left a comment

kobelb Jul 5, 2017

kobelb Jul 5, 2017

kobelb Jul 5, 2017

spalger Jul 6, 2017

nreese Jul 7, 2017

kobelb Jul 5, 2017

nreese Jul 5, 2017 •

edited

Loading

nreese Jul 5, 2017 •

edited

Loading

kobelb Jul 5, 2017

kobelb Jul 5, 2017

nreese Jul 5, 2017

kobelb Jul 5, 2017

kobelb Jul 6, 2017

nreese Jul 6, 2017

kobelb Jul 7, 2017

kobelb Jul 6, 2017

kobelb Jul 6, 2017

kobelb Jul 6, 2017

spalger left a comment

spalger Jul 6, 2017

spalger Jul 6, 2017

spalger Jul 6, 2017

stacey-gammon Jul 10, 2017

spalger Jul 6, 2017 •

edited

Loading

kobelb left a comment

		@@ -0,0 +1,29 @@
		import { asPrettyString } from '../../utils/as_pretty_string';

Expose field formatters in kibana server #12625

Expose field formatters in kibana server #12625

Conversation

nreese commented Jun 30, 2017 • edited Loading

kobelb 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

nreese Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

nreese Jul 5, 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 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

spalger Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

kobelb left a comment

Choose a reason for hiding this comment

nreese commented Jun 30, 2017 •

edited

Loading

nreese Jul 5, 2017 •

edited

Loading

nreese Jul 5, 2017 •

edited

Loading

spalger Jul 6, 2017 •

edited

Loading