[draft] Lens/line chart renderer

[chrisdavies]

Adds a barebones expression for rendering X/Y charts, and some really simple tests.

Question for reviewers

Where it made sense, I mirrored the naming conventions used in elastic charts. Good idea / bad idea? I kind of think "column" / "columns" might be a better fit for us than "accessor" / "accessors", but consistency is also good... Thoughts?

Notes

• This is a really minimal first step
• Nothing is yet registered w/ the interpreter
• We should add sanitization / validation into the various expression type functions
• Some of the required properties should probably be optional w/ sound defaults
• React snapshot tests don't really work for elastic charts, so robustly testing this is a bit of a challenge, TBD how we want to tackle that

[wylieconlon]

I see that this is a pretty high-level change, so I've tried to leave high-level comments about the way this uses expressions.

Also, I was not finding the various docstring-type comments to be adding value to my review. For example, I would change this:

/**
 * Configuration for the chart's legend.
 */
export interface LegendConfig {}

/**
 * LegendConfig as an expression type.
 */
export const legendConfig = {}

to this:

export interface LensXyLegendExpressionArguments {}

export const lensXyLegendExpression = {};

[wylieconlon]

I was expecting a single function to be registered to handle all xy_chart options, and just to confirm my understanding what this will look like when combined is something like this:

| xy_chart
  legend={legendConfig isVisible=true position=left}
  x={xConfig accessors=[col-1]}
  y={yConfig accessors=[col-0]}
  stackAccessors=[col-0]

I see that there is something similar in canvas today called axisConfig which is a function that configures a single axis. Is there any overlap?

Also, I noticed that you're not using any of the existing typescript support for canvas. If you look at AxisConfig for example, it's defined as ContextFunction<'axisConfig', Datatable, Arguments, AxisConfig>

[chrisdavies]

We don't want to pull in any types from x-pack/plugins/canvas... just because those are really internal Canvas concerns. In fact, I need to change this line import { Datatable } from '../../../canvas/canvas_plugin_src/functions/types';. I wonder if we should move the Datatable type definition into the core interpreter.

Anyway, if we decide that it's worth it, we could pull shared types / functions / modules out of Canvas into some common location such as the core interpreter. In our case, we want to be able to fairly easily move some things from x-pack to OSS, and I think (at least for now), it makes the most sense for us to own our own definitions and types and minimize dependencies.

[wylieconlon]

Given the existing AxisConfig, I'm worried about confusing naming. I assume each visualization will be defining its own expressions and types, so it might be worth it to distinguish in the naming between each visualization and potentially also with a lens prefix.

[chrisdavies]

I don't generally like using a prefix. That's what modules are for. It's OK for there to be similar or even the same name for similar / same concepts across Kibana, as long as they are contained in their own modules. That said, I do like the idea of disambiguating things. Can you think of a good way to do that without prefixes?

[wylieconlon]

I don't currently see a difference in the X and Y expressions, why separate them?

[chrisdavies]

It's a difference in their types. Accessor vs Accessors being the only real difference. (X is singular.)

[wylieconlon]

You're not taking advantage of the canvas support for enums, looking at axisConfig.ts in canvas I see that they are using:

types: ['string'],
options: ['bar', 'line', 'area'],

[chrisdavies]

Nice. Yeah. I was hoping to get this kind of feedback. I was having a hard time finding things like this in any documentation.

[wylieconlon]

Looks like you're not taking advantage of having either default values or restricted options in the expression definition here. See axisConfig.ts

[chrisdavies]

That's true. Defaults were on my TODO list for a later PR, but I'm OK with adding them now.

[wylieconlon]

Not sure if you've seen this case yet, but elastic-charts requires some preprocessing of data such as for date types.

[chrisdavies]

Yeap. That's all stuff that I think will be easier to hammer out when we actually have this rendering. It's kind of hard to nail things like that down until then. Most of this PR was tested in a standalone project, where I had a formatter for each axis. Formatters are something we need to support, but I think it makes sense as its own PR.

[chrisdavies]

@wylieconlon I agree that the doc comments are low value. I'm OK with getting rid of them. I don't like the really long names like LensXyLegendExpressionArguments, though. The one thing I want to be really clear when someone sees the code is that we have type definitions and expression definitions which are describing essentially the same thing and need to be kept in sync. So, I'm putting them one after the other in code. I like the idea of the names suggesting what they are. Let's kick some naming ideas around.

[flash1293]

any types shouldn't work because of the updated eslint rule - maybe that's fixed automatically with the enhanced types from Canvas. If not, we can just use unknown here, it's unused anyway

[flash1293]

Prompt to discuss: Do we want to have sensible defaults for legend, seriesType, x and y config? We don't have to generate these from within Lens, but it probably makes this function easier reusable

[chrisdavies]

We should discuss this, and I think we should have defaults. We can always go less strict easily, but the reverse is not true, so I'm leaving defaults for a future PR.

[flash1293]

Same here - any should be unknown or even better an existing type for render functions (but I'm not sure whether there is one yet).

[flash1293]

Can we also test the actual chart? I vaguely remember something from the charts channel that there is a problem with that.

[flash1293]

I have to look it up, but I think kibana_datatable is not identical with the Datatable type from Canvas.

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💔 Build Failed

• continuous-integration/kibana-ci/pull-request

[elasticmachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request

[flash1293]

LGTM

[wylieconlon]

LGTM