Remove private module dependencies from field formatters #12239
Conversation
…ss getConfig to FieldFormat constructors
nreese
changed the title
| @@ -0,0 +1,7 @@ | |||
| export function ConfigProvider(config) { | |||
| return { | |||
| getConfig: (key, defaultValue) => { | |||
There was a problem hiding this comment.
What value do you see in this service, over just defining const getConfig = (...args) => config.get(...args) above where it's used?
| let TestFormat; | ||
|
|
||
| beforeEach(ngMock.module('kibana')); | ||
| beforeEach(ngMock.inject(function (Private) { | ||
| FieldFormat = Private(IndexPatternsFieldFormatProvider); | ||
| beforeEach(ngMock.inject(function () { |
There was a problem hiding this comment.
You can remove the ngMock stuff now that this doesn't need angular.
| return getHighlightHtml(formatted, hit.highlight[field.name]); | ||
| } | ||
| // format a list of values. In text contexts we just use JSON encoding | ||
| return toJson(value.map(recurse), true); |
There was a problem hiding this comment.
This toJson function behaves like JSON.stringify(), the second argument is a replacer (boolean isn't compatible)
|
|
||
| return Class; | ||
| Class.prototype.getParamDefaults = function () { | ||
| const paramDefaults = _.get(opts, 'paramDefaults', {}); |
There was a problem hiding this comment.
I'm not a huge fan of splitting the param defaults up like this. What about something like this?
Class.prototype.getParamDefaults = function () {
if (_.has(opts, 'getParamDefaults')) {
return opts.getParamDefaults(this.getConfig)
}
return {
pattern: this.getConfig(`format:${opts.id}:defaultPattern`)
}
}Then the instances can specify the getParamDefaults(getConfig) option and read config themselves.
…n because function is only converting an array of strings to into a string
|
Hold off on reviewing. Looks like this PR messed up field format editors. I need to clean that up |
… to instance via getter
|
Field format editors have been fixed. Review away. |
| } | ||
|
|
||
| export const contentTypes = { |
There was a problem hiding this comment.
nitpick:
export const contentTypes = { types, setup }
There was a problem hiding this comment.
Is there a concern of naming collisions? Should setup and types get some kind of prefix to make them more unique?
There was a problem hiding this comment.
Looks like types is never even used by any external modules. I will just export setup as contentTypesSetup.
| if (!value || typeof value.map !== 'function') { | ||
| return convert.call(format, value); | ||
| } | ||
| const subVals = value.map(function (v) { |
There was a problem hiding this comment.
nitpick: Maybe clean up some of these too:
const subVals = value.map(v => recurse(v, field, hit));Same for useMultiLine
| export function IndexPatternsFieldFormatProvider(config, $rootScope, Private) { | ||
| const contentTypes = Private(IndexPatternsFieldFormatContentTypesProvider); | ||
| export function FieldFormat(params) { | ||
| const self = this; |
There was a problem hiding this comment.
we can just remove this and rely on this instead here, I think
|
|
||
| /** | ||
| * Get parameter defaults | ||
| * @param {string} [contentType=text] |
There was a problem hiding this comment.
bug ^? (no param used below)
There was a problem hiding this comment.
Just a copy and paste error
| } | ||
|
|
||
| Source.id = '_source'; | ||
| Source.title = '_source'; | ||
| Source.fieldType = '_source'; | ||
|
|
||
| Source.prototype._convert = { | ||
| text: angular.toJson, | ||
| text: (value) => { | ||
| return toJson(value); |
There was a problem hiding this comment.
why this change? If relevant, maybe we should add a comment that explains it?
Also, this can be: text: (value) => toJson(value). No need for {} here.
There was a problem hiding this comment.
@spalger recommended this to make things more clear
The PR removes all private module dependencies from field formatters. The PR is phase 1 for exposing field formatters on the server.