Expression datatable formatting hints#43091
Conversation
flash1293
added
v8.0.0
release_note:skip
💚 Build Succeeded |
lukeelmers
left a comment
lukeelmers
left a comment
There was a problem hiding this comment.
Overall LGTM, just added a few questions & comments.
src/legacy/ui/public/visualize/loader/pipeline_helpers/utilities.ts
Outdated
Show resolved
Hide resolved
|
|
||
| const name = 'kibana_datatable'; | ||
|
|
||
| export interface SerializedFieldFormat<TParams = object> { |
There was a problem hiding this comment.
Since this is public/exported, maybe it would make sense to co-locate it with getFormat & the pipeline utilities, and import it into here instead? Since it doesn't have anything to do with the kibana_datatable type specifically.
To some extent it doesn't matter a lot right now since all of the ui/visualize/loader stuff is going away or moving eventually anyway, but IMO this is the only thing in the PR that feels like maybe it should be moved.
There was a problem hiding this comment.
That's how I started out over here (5548579), but somehow the type check fails in completely unrelated places if I do that - something about importing legacy stuff from the new world. Then I thought it's maybe even cleaner that way because the types of the data table are all defined in one place.
Maybe this was a configuration error already fixed, I'll move it over and check.
There was a problem hiding this comment.
Ah I didn't think about that @flash1293 -- you are totally right. You can import new platform into legacy, but you cannot import legacy into new platform.
Not sure what makes the most then, since we definitely won't want to keep this in with the datatable type longer term. Maybe instead we should stick it in the types/common.ts file for now? I just don't want it to get lost in expression_types/kibana_datatable.ts
There was a problem hiding this comment.
Moved it over to common
src/legacy/ui/public/visualize/loader/pipeline_helpers/build_pipeline.ts
Outdated
Show resolved
Hide resolved
| name: column.name, | ||
| })), | ||
| columns: response.columns.map((column: any) => { | ||
| const cleanedColumn: KibanaDatatable['columns'][0] = { |
There was a problem hiding this comment.
Do you think enough folks will have a use case for this that it'd be worth exporting Column separately, perhaps as KibanaDatatableColumn, so that you don't have to do stuff like this?
There was a problem hiding this comment.
why do we always take the first column ?
There was a problem hiding this comment.
Because all of the columns in the array have the same type, this is a hack. I agree with @lukeelmers here, it makes sense to expose KibanaDatatableColumn to be able to cleanly reference this type.
Current solution works like this:
KibanaDatatable is of type { columns: Array<Column> }
KibanaDatatable['columns] is of type Array<Column>
now the question is how do I get to the type of the Column itself? I could do a proper type inference like this: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-8.html#type-inference-in-conditional-types or I could do a dirty hack and get the type of the 0 property of KibanaDatatable['columns] which happens to be Column
There was a problem hiding this comment.
A bit orthogonal to the discussion here, but in cases where magic like this is necessary, I find the syntax KibanaDatatable['columns'][number] to be slightly easier to grok, mostly because the index number can be distracting as @ppisljar points out.
But agreed that exporting separately feels like the way to go here.
|
Pinging @elastic/kibana-app-arch |
lukeelmers
added
review
Feature:ExpressionLanguage
| name: column.name, | ||
| })), | ||
| columns: response.columns.map((column: any) => { | ||
| const cleanedColumn: KibanaDatatable['columns'][0] = { |
There was a problem hiding this comment.
why do we always take the first column ?
| timeRange: get(context, 'timeRange', null), | ||
| query: get(context, 'query', null), | ||
| filters: get(context, 'filters', null), | ||
| timeRange: get(context, 'timeRange', undefined), |
There was a problem hiding this comment.
this went in because the existing RequestHandler types don't allow null here (but they allow undefined)
| } | ||
|
|
||
| export type RequestHandler = <T>(params: RequestHandlerParams) => T; | ||
| export type RequestHandler<T = unknown> = (params: RequestHandlerParams) => T; |
There was a problem hiding this comment.
moving the type parameter to the outer type to be able to specify it in courier-specific type. Inferred types in other usages should work like before.
💔 Build Failed |
|
@lukeelmers :this: is the error I meant - no idea why this happens but if we don't import legacy from within NP world, it works. |
This reverts commit 4738f82.
💚 Build Succeeded |
💚 Build Succeeded |
4 participants
This PR adds an optional property to the column of expression data tables:
formatHint. It carries the JSON representation of a field formatter config that can be used by expression renderers to format the values of the column correctly.This is a design decision with trade-offs: Conceptually formatting belongs to the presentation of the data and shouldn't be part of the "data source"-ey expression functions. But correctly formatting something is very strongly coupled to the source of a given data table - the data source (in this case
esaggs) knows about source-specific formatting information (e.g. user defined field formatters on the given index pattern or the interval of a date_histogram aggregation to format dates accordingly). The render functions shouldn't know or care about these concepts and passing in the formatting information as sub-expressions of the renderer function would cause redundant information on the expression that can get out of sync. Providing the data source with a way to pass formatting hints along with the data itself together with a way to overwrite/ignore this formatting offers the benefits of both ways - a clean and robust expression that integrates with existing formatting sources of the system in the standard case, but still full control over the formatting if this is necessary.The
esaggsfunction is extended with an optional boolean argumentincludeFormatHintsthat can be set to add formatting information to the data table returned byesaggs. It uses the same implementation as thebuild_pipelinehelper for the expression loader for Visualize.Theoretically the Visualize renderers and
build_pipelinecould be rewritten to not include the formatting information in the arguments of the render function but to use the format hints from esaggs, but that's out of scope for this PR IMHO.Not 100% sure about the files the various helpers and types should reside, open for suggestions here.