[Lens] Code quality improvements

[markov00]

Here is a collection of possible improvements in our code, and when I have time I also like to propose a viable alternative.
They are in random order, as I found them/remember them.
I want to add a comment per issue/improvement so that everybody can contribute to it by commenting right below each comment in a thread.

[markov00]

Avoid unnecessary types guards and undefined/null fallbacks

kibana/x-pack/plugins/lens/public/visualizations/xy/visualization_helpers.tsx

Lines 131 to 150 in 5c0f034

	export const isDataLayer = (layer: XYLayerConfig): layer is XYDataLayerConfig =>
	layer.layerType === layerTypes.DATA || !layer.layerType;
	export const getDataLayers = (layers: XYLayerConfig[]) =>
	(layers || []).filter((layer): layer is XYDataLayerConfig => isDataLayer(layer));
	export const getFirstDataLayer = (layers: XYLayerConfig[]) =>
	(layers || []).find((layer): layer is XYDataLayerConfig => isDataLayer(layer));
	export const isReferenceLayer = (
	layer: Pick<XYLayerConfig, 'layerType'>
	): layer is XYReferenceLineLayerConfig => layer.layerType === layerTypes.REFERENCELINE;
	export const getReferenceLayers = (layers: Array<Pick<XYLayerConfig, 'layerType'>>) =>
	(layers || []).filter((layer): layer is XYReferenceLineLayerConfig => isReferenceLayer(layer));
	export const isAnnotationsLayer = (
	layer: Pick<XYLayerConfig, 'layerType'>
	): layer is XYAnnotationLayerConfig =>
	layer.layerType === layerTypes.ANNOTATIONS && 'indexPatternId' in layer;

1. When filtering we are using a type guard function, there is no need to add another type guard in the filter itself.

2. The layers array, as written and described in TS, is always an array: it is described as an array in this function argument but also the object that is usually passed to that function state.layers. There is no need to fall back to an empty array if it is not an array.
   If that is a possible situation, I feel that is better to type the argument as optional or possibly undefined/null to make it explicit.

3. I've noticed in multiple places that we use logical OR operator || in cases where probably the nullish coalescing operator ?? is probably a better choice, or better, it avoids some edge cases that are probably unwanted, like if we pass an empty string, a zero or a false boolean value. I feel in most cases we are looking to fallback null and/or undefined values so ?? is a better and clear choice on what we are looking for.

Possible cleanup

// all type guards semantically related are moved close together

export const isDataLayer = (layer: XYLayerConfig): layer is XYDataLayerConfig =>
  layer.layerType === layerTypes.DATA || !layer.layerType;

export const isReferenceLayer = (layer: XYLayerConfig): layer is XYReferenceLineLayerConfig =>
  layer.layerType === layerTypes.REFERENCELINE;

export const isAnnotationsLayer = (layer: XYLayerConfig): layer is XYAnnotationLayerConfig =>
  layer.layerType === layerTypes.ANNOTATIONS && 'indexPatternId' in layer;

// no need for the logical OR as the argument is always an array as the argument type expect
// no need for the typed type guard inside the filter function
// we can also pass directly the typeguard to the filter instead of extracting more variables

export const getDataLayers = (layers: XYLayerConfig[]) => layers.filter(isDataLayer);

export const getFirstDataLayer = (layers: XYLayerConfig[]) => layers.find(isDataLayer);

export const getReferenceLayers = (layers: XYLayerConfig[]) => layers.filter(isReferenceLayer);

export const getAnnotationsLayers = (layers: XYLayerConfig[]) => layers.filter(isAnnotationsLayer);

[markov00]

Write meaningful type guards

In the code below we are returning an array of DataLayers with at least 1 object in the layer.accessors property.
A type guard is also applied: (layer): layer is ValidXYDataLayerConfig => Boolean(layer.accessors.length) but if you check the type ValidXYDataLayerConfig it doesn't mention a non-empty accessors array, just the fact that the xAccessor exist and remark (because the type was the same also in the inherited type) that a layerId property exists.
I see this as a tech debt because the way it is written, there is no connection between having at least 1 element in the accessors array and the xAccessor.

kibana/x-pack/plugins/lens/public/visualizations/xy/to_expression.ts

Lines 198 to 205 in fde9539

	const validDataLayers: ValidXYDataLayerConfig[] = getDataLayers(state.layers)
	.filter<ValidXYDataLayerConfig>((layer): layer is ValidXYDataLayerConfig =>
	Boolean(layer.accessors.length)
	)
	.map((layer) => ({
	...layer,
	accessors: getSortedAccessors(datasourceLayers[layer.layerId], layer),
	}));

kibana/x-pack/plugins/lens/public/visualizations/xy/types.ts

Lines 95 to 109 in fde9539

	export interface XYDataLayerConfig {
	layerId: string;
	accessors: string[];
	layerType: 'data';
	seriesType: SeriesType;
	xAccessor?: string;
	simpleView?: boolean;
	yConfig?: YConfig[];
	splitAccessor?: string;
	palette?: PaletteOutput;
	collapseFn?: CollapseFunction;
	xScaleType?: XScaleType;
	isHistogram?: boolean;
	columnToLabel?: string;
	}

kibana/x-pack/plugins/lens/public/visualizations/xy/types.ts

Lines 183 to 186 in fde9539

	export interface ValidXYDataLayerConfig extends XYDataLayerConfig {
	xAccessor: NonNullable<XYDataLayerConfig['xAccessor']>;
	layerId: string;
	}

My suggestions here are:

• reconsider the type-guard applied and checks for the xAccessor too
• add a NonEmpty array type to the accessors in ValidXYDataLayerConfig if this constrain is required later in the code
• remove the redundant typing of layerId in ValidXYDataLayerConfig as it is already available in XYDataLayerConfig
• I also believe that there should be a better name to the type that adding a Valid prefix. The word Valid doesn't give much information on what is going on, it could also imply that the XYDataLayerConfig is a non-valid type too. I believe there can be a better naming for that

[markov00]

Replace Record<string,...> with Map<string,...> when the key can be modified by the user in some way

To avoid JS prototype poisoning or just to avoid issues with reserved JS keywords as labels, is always safer to use a Map instead of a Record.
Consider for example if a user specifies labels like: toString hasOwnProperty etc causing some subsequent call to these functions to fail.

Using Map also gives a bit more safety if our typescript is not configured with noUncheckedIndexedAccess. This can lead to errors like the following:

const data: Record<string, number> = { a: 1, b: 2 };
const sum = data['x'] + 10;

In this case data['x'] is typed as string even if it's not available. If data was a Map then x should be checked before use.

This is a general approach, I see various places where Records are used to store labels keys

[markov00]

Strengthen type earlier

I've noticed that we don't straighten our type when we apply some default values like in the code sample below.

In the code below for example we are adding some default values to 8 parameters that originally are optional. But typing the array with the same type as it was before (so with all the optional parameters) creates 2 problems:

• nullifies the effect of adding defaults, because a subsequent use of that array/object needs to check again and apply a fallback even if this does not happen
• is not clear to the reader what is the source of truth for the default values. Is here where we set the default showGridLines value or is it somewhere else? This could happen because I'm not sure if that is the final shape of the yAxis configurations that is used on every other waterfall component, code that picks from state configuration and applies similar or different defaults could exist down the stream anytime.

There is also a third issue here: we are using 2 times the same type name AxisConfig. Each type is different and they come from 2 different packages.

kibana/x-pack/plugins/lens/public/visualizations/xy/to_expression.ts

Lines 237 to 259 in 94e850d

	const yAxisConfigs: AxisConfig[] = [
	{
	position: Position.Left,
	extent: state?.yLeftExtent,
	showTitle: state?.axisTitlesVisibilitySettings?.yLeft ?? true,
	title: state.yTitle || '',
	showLabels: state?.tickLabelsVisibilitySettings?.yLeft ?? true,
	showGridLines: state?.gridlinesVisibilitySettings?.yLeft ?? true,
	labelsOrientation: state?.labelsOrientation?.yLeft ?? 0,
	scaleType: state.yLeftScale || 'linear',
	},
	{
	position: Position.Right,
	extent: state?.yRightExtent,
	showTitle: state?.axisTitlesVisibilitySettings?.yRight ?? true,
	title: state.yRightTitle || '',
	showLabels: state?.tickLabelsVisibilitySettings?.yRight ?? true,
	showGridLines: state?.gridlinesVisibilitySettings?.yRight ?? true,
	labelsOrientation: state?.labelsOrientation?.yRight ?? 0,
	scaleType: state.yRightScale || 'linear',
	},
	];

kibana/src/plugins/chart_expressions/expression_xy/common/types/expression_functions.ts

Lines 83 to 103 in 94e850d

	export interface AxisConfig {
	title?: string;
	hide?: boolean;
	id?: string;
	position?: Position;
	labelColor?: string;
	showOverlappingLabels?: boolean;
	showDuplicates?: boolean;
	labelsOrientation?: number;
	truncate?: number;
	showLabels?: boolean;
	showTitle?: boolean;
	showGridLines?: boolean;
	extent?: AxisExtentConfigResult;
	}
	export interface YAxisConfig extends AxisConfig {
	mode?: AxisMode;
	boundsMargin?: number;
	scaleType?: YScaleType;
	}

kibana/x-pack/plugins/lens/public/visualizations/xy/types.ts

Lines 73 to 75 in 94e850d

	export interface AxisConfig extends Omit<YAxisConfig, 'extent'> {
	extent?: AxisExtentConfig;
	}

[stratoula]

This function returns the first property from the existing datasources on the doc as the active datasource. If there are 2, it doesnt guarantee that the correct datasourceId will be returned. We should refactor it and check if the datasources in the state have layers. But for now I fixed that on our builder

function getActiveDatasourceIdFromDoc(doc?: Document) {
  if (!doc) {
    return null;
  }

  const [firstDatasourceFromDoc] = Object.keys(doc.state.datasourceStates);
  return firstDatasourceFromDoc || null;
}

[stratoula]

This interface should be updated. Lens state has the kql/lucene query (Query) if it is on dataview mode or the ES|QL query (AggregateQuery) if it is on the textbased mode. This needs to be updated to query: Query | AggregateQuery; It is not a simple change. A bunch of files and functionality should change to accomodate this change.

export interface Document {
  savedObjectId?: string;
  type?: string;
  visualizationType: string | null;
  title: string;
  description?: string;
  state: {
    datasourceStates: Record<string, unknown>;
    visualization: unknown;
    query: Query;
    globalPalette?: {
      activePaletteId: string;
      state?: unknown;
    };
    filters: Filter[];
    adHocDataViews?: Record<string, DataViewSpec>;
    internalReferences?: SavedObjectReference[];
  };
  references: SavedObjectReference[];
}

When this is done the LensConfigOptions on the builder should also be updated.

interface LensConfigOptions {
  /** if true the output will be embeddable input, else lens attributes */
  embeddable?: boolean;
  /** optional time range override */
  timeRange?: TimeRange;
  filters?: Filter[];
  query?: Query;
}

[drewdaemon]

Our type for datasourceStates in the Document interface is currently very loose:

kibana/x-pack/plugins/lens/public/persistence/saved_object_store.ts

Line 24 in e926602

datasourceStates: Record<string, unknown>;

We should update this type to be more helpful. The first step is to figure out what is actually going on with our saved objects so that we craft a type that is accurate both to what is really happening and to how our application should be viewing the state.

For example, if formBased and textBased are mutually exclusive we might consider something like the following

datasourceStates: { formBased: FormBasedPersistedState, textBased?: never } | { formBased?: never, textBased: TextBasedPersistedState };