Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: chart state and series functions cleanup #989

Merged
merged 27 commits into from
Jan 25, 2021
Merged

fix: chart state and series functions cleanup #989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
8a910c1
test: heterogeneous cartesian story
monfera Jan 20, 2021
e62a4b8
test: stories
monfera Jan 20, 2021
ef613c9
refactor: simplify findMainChartType and cover the possibility of zer…
monfera Jan 20, 2021
644c1d0
refactor: simplify spec parsed case
monfera Jan 20, 2021
d930263
refactor: remove double return from upsert spec due to commonality ex…
monfera Jan 20, 2021
ff1d169
refactor: remove double return from ext pointer event due to commonal…
monfera Jan 20, 2021
5f4f7a1
refactor: reduce other bifurcations
monfera Jan 20, 2021
092a2b1
chore: type comment and moving away from naming a type as if it were …
monfera Jan 20, 2021
b18594a
refactor: replace control flow with a straightforward linear preceden…
monfera Jan 20, 2021
2e1c958
chore: remove @param
monfera Jan 20, 2021
02858be
refactor: simplify series name determination 1
monfera Jan 20, 2021
a050312
refactor: simplify series name determination 2
monfera Jan 20, 2021
cd11ecc
refactor: simplify series name determination 3
monfera Jan 20, 2021
07c7509
refactor: simplify series name determination 4 - always string
monfera Jan 20, 2021
e157b16
refactor: simplify series name determination 4 - narrowing types
monfera Jan 20, 2021
1cec05e
refactor: simplify series name determination 5 - customLabel is const
monfera Jan 20, 2021
181ba19
refactor: simplify series name determination 6 - delimiter is const
monfera Jan 20, 2021
0c72f25
refactor: simplify series name determination 7
monfera Jan 20, 2021
878b2d2
refactor: simplify series name determination 8
monfera Jan 20, 2021
e06d62f
refactor: simplify series name determination 9
monfera Jan 20, 2021
11091b0
refactor: simplify series name determination a
monfera Jan 20, 2021
9058c10
refactor: simplify series name determination b - all nameKeys are non…
monfera Jan 21, 2021
95fb4e1
fix: sort comparefn must yield finite numbers
monfera Jan 21, 2021
3e2d1c7
test: baseline mock before adding partition small multiples
monfera Jan 21, 2021
76dfdf6
test: image
monfera Jan 21, 2021
3545cd8
test: image rotation
monfera Jan 21, 2021
17e1356
chore: pr feedback ht Nick
monfera Jan 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 1 addition & 20 deletions .playground/playground.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,28 +17,9 @@
* under the License.
*/

/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏼

* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import React from 'react';

import { Example } from '../stories/icicle/01_unix_icicle';
import { Example } from '../stories/small_multiples/6_heterogeneous_cartesians';

export class Playground extends React.Component {
render() {
Expand Down
Binary file added ...l-stories-small-multiples-alpha-heterogeneous-visually-looks-correct-1-snap.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
118 changes: 34 additions & 84 deletions src/chart_types/xy_chart/utils/series.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,20 +19,20 @@

import { SeriesIdentifier, SeriesKey } from '../../../common/series_id';
import { ScaleType } from '../../../scales/constants';
import { GroupBySpec, BinAgg, Direction, XScaleType } from '../../../specs';
import { BinAgg, Direction, GroupBySpec, XScaleType } from '../../../specs';
import { OrderBy } from '../../../specs/settings';
import { ColorOverrides } from '../../../state/chart_state';
import { Accessor, AccessorFn, getAccessorValue } from '../../../utils/accessor';
import { Datum, Color, isNil } from '../../../utils/common';
import { Color, Datum, isNil } from '../../../utils/common';
import { GroupId } from '../../../utils/ids';
import { Logger } from '../../../utils/logger';
import { ColorConfig } from '../../../utils/themes/theme';
import { groupSeriesByYGroup, isHistogramEnabled, isStackedSpec } from '../domains/y_domain';
import { LastValues } from '../state/utils/types';
import { applyFitFunctionToDataSeries } from './fit_function_utils';
import { groupBy } from './group_data_series';
import { BasicSeriesSpec, SeriesTypes, SeriesSpecs, SeriesNameConfigOptions, StackMode } from './specs';
import { formatStackedDataSeriesValues, datumXSortPredicate } from './stacked_series_utils';
import { BasicSeriesSpec, SeriesNameConfigOptions, SeriesSpecs, SeriesTypes, StackMode } from './specs';
import { datumXSortPredicate, formatStackedDataSeriesValues } from './stacked_series_utils';
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved

/** @internal */
export const SERIES_DELIMITER = ' - ';
Expand Down Expand Up @@ -115,9 +115,6 @@ export function getSeriesIndex(series: SeriesIdentifier[], target: SeriesIdentif

/**
* Returns string form of accessor. Uses index when accessor is a function.
*
* @param accessor
* @param index
* @internal
*/
export function getAccessorFieldName(accessor: Accessor | AccessorFn, index: number) {
Expand All @@ -126,7 +123,7 @@ export function getAccessorFieldName(accessor: Accessor | AccessorFn, index: num

/**
* Split a dataset into multiple series depending on the accessors.
* Each series is then associated with a key thats belong to its configuration.
* Each series is then associated with a key that belongs to its configuration.
* This method removes every data with an invalid x: a string or number value is required
* `y` values and `mark` values are casted to number or null.
* @internal
Expand Down Expand Up @@ -329,12 +326,7 @@ function castToNumber(value: any, nonNumericValues: any[]): number | null {
return num;
}

/**
* Sorts data based on order of xValues
* @param dataSeries
* @param xValues
* @param xScaleType
*/
/** Sorts data based on order of xValues */
const getSortedDataSeries = (
dataSeries: DataSeries[],
xValues: Set<string | number>,
Expand Down Expand Up @@ -380,14 +372,7 @@ export function getFormattedDataSeries(
return [...fittedAndStackedDataSeries, ...nonStackedDataSeries];
}

/**
*
* @param seriesSpecs the map for all the series spec
* @param deselectedDataSeries the array of deselected/hidden data series
* @param enableVislibSeriesSort is optional; if not specified in <Settings />,
* @param smallMultiples
* @internal
*/
/** @internal */
export function getDataSeriesFromSpecs(
seriesSpecs: BasicSeriesSpec[],
deselectedDataSeries: SeriesIdentifier[] = [],
Expand Down Expand Up @@ -518,6 +503,8 @@ function getSortedOrdinalXValues(
}
}

const BIG_NUMBER = Number.MAX_SAFE_INTEGER; // the sort comparator must yield finite results, can't use infinities
markov00 marked this conversation as resolved.
Show resolved Hide resolved

function getSeriesNameFromOptions(
options: SeriesNameConfigOptions,
{ yAccessor, splitAccessors }: XYChartSeriesIdentifier,
Expand All @@ -530,7 +517,7 @@ function getSeriesNameFromOptions(
return (
options.names
.slice()
.sort(({ sortIndex: a = Infinity }, { sortIndex: b = Infinity }) => a - b)
.sort(({ sortIndex: a = BIG_NUMBER }, { sortIndex: b = BIG_NUMBER }) => a - b)
.map(({ accessor, value, name }) => {
const accessorValue = splitAccessors.get(accessor) ?? null;
if (accessorValue === value) {
Expand All @@ -557,41 +544,28 @@ export function getSeriesName(
isTooltip: boolean,
spec?: BasicSeriesSpec,
): string {
let delimiter = SERIES_DELIMITER;
if (spec && spec.name && typeof spec.name !== 'string') {
let customLabel: string | number | null = null;
if (typeof spec.name === 'function') {
customLabel = spec.name(seriesIdentifier, isTooltip);
} else {
delimiter = spec.name.delimiter ?? delimiter;
customLabel = getSeriesNameFromOptions(spec.name, seriesIdentifier, delimiter);
}

if (customLabel !== null) {
return customLabel.toString();
}
const customLabel =
typeof spec?.name === 'function'
? spec.name(seriesIdentifier, isTooltip)
: typeof spec?.name === 'object' // extract booleans once https://github.com/microsoft/TypeScript/issues/12184 is fixed
? getSeriesNameFromOptions(spec.name, seriesIdentifier, spec.name.delimiter ?? SERIES_DELIMITER)
: null;
Comment on lines +547 to +552
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems harder to read for me TBH, not sure why the nested ternary lines are not indented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's not nesting b/c it's a linear fallthrough case, ie. no structural nesting applies, more like a case sequence in a switch. Would work with indented formatting, would still preserve ability to linearly read through

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a ternary inside another ternary, that's nested no?

As far as the indenting, this is what I'd like to see that delineates the exprIfTrue and exprIfTrue code blocks.

const customLabel =
    typeof spec?.name === 'function'
      ? spec.name(seriesIdentifier, isTooltip)
      : typeof spec?.name === 'object' // extract booleans once https://github.com/microsoft/TypeScript/issues/12184 is fixed
        ? getSeriesNameFromOptions(spec.name, seriesIdentifier, spec.name.delimiter ?? SERIES_DELIMITER)
        : null;

Copy link
Contributor Author

@monfera monfera Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This formatting would be OK too, maybe in some formatting PR we can test drive how the rule would shape the ternaries here and elsewhere.
You are right, the syntax AST is a right-deep tree which is conceptually still linear (the reason it was born), as in, you could substitute it with a reduce, or switch/case alternative, or it can be read as

Condition1
Result 1
Condition2
Result2
Condition3
Result3
...

Especially with code I didn't write, I find it way easier to linearly step through, "OK this condition still not true, let's look whether the next applies" compared to needing to simulate ascending and descending into (semantically, not just AST-wise) nested ifs. So this is what I meant by rectangular code, linear readability. I know I'm in trouble when the left edge of the code is meandering, I might have some weaknesses there, usually, it takes less time to simplify data flow and control flow paths and making the code shorter (all of which I think also happened) than to eyeball it long enough to grok what's going on.

Languages:

JS doesn't offer a true cond, and the equivalent switch(true) pattern is not only a yoda condition but also a bit unwieldy (b/c our linter requires curlies for even single-row returns), though often better than nested if. A reduce with the options would work too, but it'd eagerly evaluate never taken options, expensive and would need a lot of defensive optional chaining etc.
The best support in this case is provided by those that have pattern matching constructs. Eg, a Fibonacci in haskell would be

fib n | n == 0 = 1
      | n == 1 = 1
      | n >= 2 = fib (n-1) + fib (n-2)

again, linear/rectangular code, easy to read top to bottom, and a single pass reading gives a good picture, no need to ascend and descend into conditional valleys. JS don't have no such pattern matching...

Bigger picture:

Both code (internal view, our DX) and API (external view, Kibana DX) benefit from a strive toward simplicity. One way for achieving it is to reduce the number of conditional paths, nesting variety and one-off handling. It's always straightforward to add to the code; "on this condition X let's do something different before/after/instead of the current thing", which is how code bases become tangled.

So my feel is that the real solution is not to turn into ternary all such bushy if trees, though it has some positives too here, and also, not to introduce code level constraints.

Instead, we could be be a bit slower to add features; let's brainstorm if

  • we actually need a feature (eg. this function is made this complex in part by the null return of the callback we discussed today; meanwhile nobody uses it, and even our test cases don't lock in the promising yet speculative flexibility)
  • can it be achieved without added complexity, or how can we minimize added complexity
  • can we even remove branchy code while solving a need by going a bit more general; eg. avoiding singleton cases

That's the drawback with broadening types into union types (discriminated or not), and/or optional props, and the error prone falsey and fallthrough cases that require mental juggling and exponentially growing conditional branches like | null | undefined. This is why I feel it was good to chat about lending the user overriding and other config options in ways that retain as much homogeneity as possible, and type clarity helps the user too, fewer types, tighter doc etc. This is all I'm learning along the way so quote me on this when running into my subpar code, past or future 😸

We talked about eventually farming out a humane layer for somewhat hairy rules, defaults and fallbacks, so our core isn't penetrated by API use brevity or other architecture wise superficially manageable reasons; ie. core charting, wrapped around with defaults etc. driven by best practices, folks' expectations, customs etc. We had a chat with @markov00 today about an approach toward layered, priority/fallback driven configuration, which doesn't bake in what the user can and cannot override along some preconceived directions. So either of these options could help avoid expoding conditionals.

So ultimately I agree, and work toward not needing this many layers of "pattern matching", ternaries or not. Future PRs from all of us FTW!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks as always for the thoughtful reply. I just don't want to get to a point where we are forming super complex ternaries to remove complex nested conditionals. Replacing some makes sense while others would be hard to understand, but yes reducing the bushyness would be great.

As for the null option and other non-used functionality. I think we could go through the code sometime and maybe identify features in the code that are any of the following:

  • Not used and does not have a practicable use case.
  • Features that are not used but have a good use case. (documentation is to blame)
  • Features or options that have multiple (>2) options to use but only a simple and complex option would suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @nickofthyme!

Great idea for hunting for disused functionality (esp. that's not foreseen to be used; maybe this null or some alternative would have great use). Maybe we could look out for the replaceability of optional or nullable/undefinable properties too, at least for alternatives discussion. Though both take a good bunch of time.

I agree the ternary chaining is less than optimal, there's room for improving on it, hopefully by simplification rather than eg. if/return or let/if/assign chaining or nesting the next time one of us touches that part of the code, so it's likely not for eternity.


if (customLabel !== null) {
return customLabel.toString();
}

let name = '';
const nameKeys =
spec && spec.yAccessors.length > 1 ? seriesIdentifier.seriesKeys : seriesIdentifier.seriesKeys.slice(0, -1);

// there is one series, the is only one yAccessor, the first part is not null
if (hasSingleSeries || nameKeys.length === 0 || nameKeys[0] == null) {
if (!spec) {
return '';
}

if (spec.splitSeriesAccessors && nameKeys.length > 0 && nameKeys[0] != null) {
name = nameKeys.join(delimiter);
} else {
name = typeof spec.name === 'string' ? spec.name : `${spec.id}`;
}
} else {
name = nameKeys.join(delimiter);
}

return name;
const multipleYAccessors = spec && spec.yAccessors.length > 1;
const nameKeys = multipleYAccessors ? seriesIdentifier.seriesKeys : seriesIdentifier.seriesKeys.slice(0, -1);
const nonZeroLength = nameKeys.length > 0;

Copy link
Contributor Author

@monfera monfera Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The block below (return nonZeroLength...) could be extracted into a separate function, though not much benefit right now. And flat, linear ternary chaining for the inherently fall-through logic, we might need to chat about it 😃

return nonZeroLength && (spec?.splitSeriesAccessors || !hasSingleSeries)
? nameKeys.join(typeof spec?.name === 'object' ? spec.name.delimiter ?? SERIES_DELIMITER : SERIES_DELIMITER)
: spec === undefined
? ''
: typeof spec.name === 'string'
? spec.name
: spec.id;
}

function getSortIndex({ specSortIndex }: SeriesCollectionValue, total: number): number {
Expand All @@ -612,45 +586,21 @@ export function getSortedDataSeriesColorsValuesMap(

/**
* Helper function to get highest override color.
*
* from highest to lowest: `temporary`, `seriesSpec.color` then `persisted`
*
* @param key
* @param customColors
* @param overrides
* From highest to lowest: `temporary`, `seriesSpec.color` then, unless `temporary` is set to `null`, `persisted`
*/
function getHighestOverride(
key: string,
customColors: Map<SeriesKey, Color>,
overrides: ColorOverrides,
): Color | undefined {
const tempColor: Color | undefined | null = overrides.temporary[key];

if (tempColor) {
return tempColor;
}

const customColor: Color | undefined | null = customColors.get(key);

if (customColor) {
return customColor;
}

if (tempColor === null) {
// Use default color when temporary and custom colors are null
return;
}

return overrides.persisted[key];
// Unexpected empty `tempColor` string is falsy and falls through, see comment in `export type Color = ...`
// Use default color when temporary and custom colors are null
return tempColor || customColors.get(key) || (tempColor === null ? undefined : overrides.persisted[key]);
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Returns color for a series given all color hierarchies
*
* @param seriesCollection
* @param chartColors
* @param customColors
* @param overrides
* @internal
*/
export function getSeriesColors(
Expand Down
Loading