From ed49bbbb5afd69a5d771fff29e9fc7742153d94e Mon Sep 17 00:00:00 2001 From: Nick Partridge Date: Mon, 15 Jun 2020 10:50:24 -0500 Subject: [PATCH] fix: graceful scale fallbacks and warnings (#704) fix #678 --- .../renderer/canvas/partition.tsx | 7 +- .../xy_chart/domains/x_domain.test.ts | 55 ++++++++++++++++ src/chart_types/xy_chart/domains/x_domain.ts | 22 ++++--- src/chart_types/xy_chart/state/utils/utils.ts | 10 +-- src/chart_types/xy_chart/utils/series.test.ts | 10 +-- src/chart_types/xy_chart/utils/series.ts | 41 ++++++++++-- src/state/chart_state.ts | 4 +- src/utils/commons.ts | 5 -- src/utils/logger.ts | 66 +++++++++++++++++++ 9 files changed, 183 insertions(+), 37 deletions(-) create mode 100644 src/utils/logger.ts diff --git a/src/chart_types/partition_chart/renderer/canvas/partition.tsx b/src/chart_types/partition_chart/renderer/canvas/partition.tsx index e92b2d7234..7a55fceae4 100644 --- a/src/chart_types/partition_chart/renderer/canvas/partition.tsx +++ b/src/chart_types/partition_chart/renderer/canvas/partition.tsx @@ -120,12 +120,7 @@ class PartitionComponent extends React.Component { indices.forEach((i) => datumIndices.add(i)); } }); - /* - *console.log( - * pickedShapes.map((s) => s.value), - * [...datumIndices.values()], - *); - */ + return pickedShapes; // placeholder } diff --git a/src/chart_types/xy_chart/domains/x_domain.test.ts b/src/chart_types/xy_chart/domains/x_domain.test.ts index c40e540611..aab0d54452 100644 --- a/src/chart_types/xy_chart/domains/x_domain.test.ts +++ b/src/chart_types/xy_chart/domains/x_domain.test.ts @@ -443,6 +443,61 @@ describe('X Domain', () => { ); expect(mergedDomain.domain).toEqual([0, 1, 2, 5, 7]); }); + + test('Should fallback to ordinal scale if not array of numbers', () => { + const ds1: BasicSeriesSpec = { + chartType: ChartTypes.XYAxis, + specType: SpecTypes.Series, + id: 'ds1', + groupId: 'g1', + seriesType: SeriesTypes.Bar, + xAccessor: 'x', + yAccessors: ['y'], + xScaleType: ScaleType.Linear, + yScaleType: ScaleType.Linear, + yScaleToDataExtent: false, + data: [ + { x: 0, y: 0 }, + { x: 'a', y: 0 }, + { x: 2, y: 0 }, + { x: 5, y: 0 }, + ], + }; + const ds2: BasicSeriesSpec = { + chartType: ChartTypes.XYAxis, + specType: SpecTypes.Series, + id: 'ds2', + groupId: 'g2', + seriesType: SeriesTypes.Bar, + xAccessor: 'x', + yAccessors: ['y'], + xScaleType: ScaleType.Linear, + yScaleType: ScaleType.Linear, + yScaleToDataExtent: false, + data: [ + { x: 0, y: 0 }, + { x: 7, y: 0 }, + ], + }; + const specDataSeries = [ds1, ds2]; + + const { xValues } = getSplittedSeries(specDataSeries); + const mergedDomain = mergeXDomain( + [ + { + seriesType: SeriesTypes.Bar, + xScaleType: ScaleType.Linear, + }, + { + seriesType: SeriesTypes.Bar, + xScaleType: ScaleType.Ordinal, + }, + ], + xValues, + ); + expect(mergedDomain.domain).toEqual([0, 'a', 2, 5, 7]); + expect(mergedDomain.scaleType).toEqual(ScaleType.Ordinal); + }); test('Should merge multi bar/line ordinal series correctly', () => { const ds1: BasicSeriesSpec = { chartType: ChartTypes.XYAxis, diff --git a/src/chart_types/xy_chart/domains/x_domain.ts b/src/chart_types/xy_chart/domains/x_domain.ts index 598c819de2..c7502bcc35 100644 --- a/src/chart_types/xy_chart/domains/x_domain.ts +++ b/src/chart_types/xy_chart/domains/x_domain.ts @@ -18,8 +18,9 @@ */ import { ScaleType } from '../../../scales/constants'; -import { compareByValueAsc, identity, isNumberArray } from '../../../utils/commons'; +import { compareByValueAsc, identity } from '../../../utils/commons'; import { computeContinuousDataDomain, computeOrdinalDataDomain, Domain } from '../../../utils/domain'; +import { Logger } from '../../../utils/logger'; import { isCompleteBound, isLowerBound, isUpperBound } from '../utils/axis_type_utils'; import { BasicSeriesSpec, DomainRange, SeriesTypes } from '../utils/specs'; import { XDomain } from './types'; @@ -36,6 +37,7 @@ export function mergeXDomain( specs: Pick[], xValues: Set, customXDomain?: DomainRange | Domain, + fallbackScale?: ScaleType, ): XDomain { const mainXScaleType = convertXScaleTypes(specs); if (!mainXScaleType) { @@ -46,7 +48,13 @@ export function mergeXDomain( let seriesXComputedDomains; let minInterval = 0; - if (mainXScaleType.scaleType === ScaleType.Ordinal) { + if (mainXScaleType.scaleType === ScaleType.Ordinal || fallbackScale === ScaleType.Ordinal) { + if (mainXScaleType.scaleType !== ScaleType.Ordinal) { + Logger.warn( + `Each X value in a ${mainXScaleType.scaleType} x scale needs be be a number. Using ordinal x scale as fallback.`, + ); + } + seriesXComputedDomains = computeOrdinalDataDomain(values, identity, false, true); if (customXDomain) { if (Array.isArray(customXDomain)) { @@ -58,11 +66,7 @@ export function mergeXDomain( } else { seriesXComputedDomains = computeContinuousDataDomain(values, identity, true); let customMinInterval: undefined | number; - if (!isNumberArray(values)) { - throw new Error( - `Each X value in a ${mainXScaleType.scaleType} x scale needs be be a number. String or objects are not allowed`, - ); - } + if (customXDomain) { if (Array.isArray(customXDomain)) { throw new TypeError('xDomain for continuous scale should be a DomainRange object, not an array'); @@ -91,7 +95,7 @@ export function mergeXDomain( seriesXComputedDomains = [computedDomainMin, customXDomain.max]; } } - const computedMinInterval = findMinInterval(values); + const computedMinInterval = findMinInterval(values as number[]); if (customMinInterval != null) { // Allow greater custom min iff xValues has 1 member. if (xValues.size > 1 && customMinInterval > computedMinInterval) { @@ -107,7 +111,7 @@ export function mergeXDomain( return { type: 'xDomain', - scaleType: mainXScaleType.scaleType, + scaleType: fallbackScale ?? mainXScaleType.scaleType, isBandScale: mainXScaleType.isBandScale, domain: seriesXComputedDomains, minInterval, diff --git a/src/chart_types/xy_chart/state/utils/utils.ts b/src/chart_types/xy_chart/state/utils/utils.ts index 12df423235..d8eb585365 100644 --- a/src/chart_types/xy_chart/state/utils/utils.ts +++ b/src/chart_types/xy_chart/state/utils/utils.ts @@ -180,13 +180,15 @@ export function computeSeriesDomains( deselectedDataSeries: SeriesIdentifier[] = [], customXDomain?: DomainRange | Domain, ): SeriesDomainsAndData { - const { splittedSeries, xValues, seriesCollection } = deselectedDataSeries - ? getSplittedSeries(seriesSpecs, deselectedDataSeries) - : getSplittedSeries(seriesSpecs, []); + const { + splittedSeries, + xValues, seriesCollection, + fallbackScale, + } = getSplittedSeries(seriesSpecs, deselectedDataSeries); const splittedDataSeries = [...splittedSeries.values()]; const specsArray = [...seriesSpecs.values()]; - const xDomain = mergeXDomain(specsArray, xValues, customXDomain); + const xDomain = mergeXDomain(specsArray, xValues, customXDomain, fallbackScale); const yDomain = mergeYDomain(splittedSeries, specsArray, customYDomainsByGroupId); const formattedDataSeries = getFormattedDataseries( diff --git a/src/chart_types/xy_chart/utils/series.test.ts b/src/chart_types/xy_chart/utils/series.test.ts index c1654aa1bd..89fe3eba1a 100644 --- a/src/chart_types/xy_chart/utils/series.test.ts +++ b/src/chart_types/xy_chart/utils/series.test.ts @@ -683,26 +683,26 @@ describe('Series', () => { expect(getSortedDataSeriesColorsValuesMap(seriesCollection)).toEqual(undefinedSortedColorValues); }); test('clean datum shall parse string as number for y values', () => { - let datum = cleanDatum([0, 1, 2], 0, 1, 2); + let datum = cleanDatum([0, 1, 2], 0, 1, [], 2); expect(datum).toBeDefined(); expect(datum?.y1).toBe(1); expect(datum?.y0).toBe(2); - datum = cleanDatum([0, '1', 2], 0, 1, 2); + datum = cleanDatum([0, '1', 2], 0, 1, [], 2); expect(datum).toBeDefined(); expect(datum?.y1).toBe(1); expect(datum?.y0).toBe(2); - datum = cleanDatum([0, '1', '2'], 0, 1, 2); + datum = cleanDatum([0, '1', '2'], 0, 1, [], 2); expect(datum).toBeDefined(); expect(datum?.y1).toBe(1); expect(datum?.y0).toBe(2); - datum = cleanDatum([0, 1, '2'], 0, 1, 2); + datum = cleanDatum([0, 1, '2'], 0, 1, [], 2); expect(datum).toBeDefined(); expect(datum?.y1).toBe(1); expect(datum?.y0).toBe(2); - datum = cleanDatum([0, 'invalid', 'invalid'], 0, 1, 2); + datum = cleanDatum([0, 'invalid', 'invalid'], 0, 1, [], 2); expect(datum).toBeDefined(); expect(datum?.y1).toBe(null); expect(datum?.y0).toBe(null); diff --git a/src/chart_types/xy_chart/utils/series.ts b/src/chart_types/xy_chart/utils/series.ts index 4a62aaa9cf..6666228274 100644 --- a/src/chart_types/xy_chart/utils/series.ts +++ b/src/chart_types/xy_chart/utils/series.ts @@ -23,6 +23,7 @@ import { ColorOverrides } from '../../../state/chart_state'; import { Accessor, AccessorFn, getAccessorValue } from '../../../utils/accessor'; import { Datum, Color } from '../../../utils/commons'; import { GroupId, SpecId } from '../../../utils/ids'; +import { Logger } from '../../../utils/logger'; import { ColorConfig } from '../../../utils/themes/theme'; import { splitSpecsByGroupId, YBasicSeriesSpec } from '../domains/y_domain'; import { LastValues } from '../state/utils/types'; @@ -150,6 +151,7 @@ export function splitSeries({ const series = new Map(); const colorsValues = new Set(); const xValues = new Set(); + const nonNumericValues: any[] = []; data.forEach((datum) => { const splitAccessors = getSplitAccessors(datum, splitSeriesAccessors); @@ -164,6 +166,7 @@ export function splitSeries({ datum, xAccessor, accessor, + nonNumericValues, y0Accessors && y0Accessors[index], markSizeAccessor, ); @@ -175,7 +178,14 @@ export function splitSeries({ } }); } else { - const cleanedDatum = cleanDatum(datum, xAccessor, yAccessors[0], y0Accessors && y0Accessors[0], markSizeAccessor); + const cleanedDatum = cleanDatum( + datum, + xAccessor, + yAccessors[0], + nonNumericValues, + y0Accessors && y0Accessors[0], + markSizeAccessor, + ); if (cleanedDatum !== null && cleanedDatum.x !== null && cleanedDatum.x !== undefined) { xValues.add(cleanedDatum.x); const seriesKey = updateSeriesMap(series, splitAccessors, yAccessors[0], cleanedDatum, specId); @@ -184,6 +194,12 @@ export function splitSeries({ } }); + if (nonNumericValues.length > 0) { + Logger.warn(`Found non-numeric y value${nonNumericValues.length > 1 ? 's' : ''} in dataset for spec "${specId}"`, + `(${nonNumericValues.map((v) => JSON.stringify(v)).join(', ')})` + ); + } + return { rawDataSeries: [...series.values()], colorsValues, @@ -266,6 +282,7 @@ export function cleanDatum( datum: Datum, xAccessor: Accessor | AccessorFn, yAccessor: Accessor, + nonNumericValues: any[], y0Accessor?: Accessor, markSizeAccessor?: Accessor | AccessorFn, ): RawDataSeriesDatum | null { @@ -280,20 +297,26 @@ export function cleanDatum( } const mark = markSizeAccessor === undefined ? null : getAccessorValue(datum, markSizeAccessor); - const y1 = castToNumber(datum[yAccessor]); + const y1 = castToNumber(datum[yAccessor], nonNumericValues); const cleanedDatum: RawDataSeriesDatum = { x, y1, datum, y0: null, mark }; if (y0Accessor) { - cleanedDatum.y0 = castToNumber(datum[y0Accessor as keyof typeof datum]); + cleanedDatum.y0 = castToNumber(datum[y0Accessor as keyof typeof datum], nonNumericValues); } return cleanedDatum; } -function castToNumber(value: any): number | null { +function castToNumber(value: any, nonNumericValues: any[]): number | null { if (value === null || value === undefined) { return null; } const num = Number(value); - return isNaN(num) ? null : num; + + if (isNaN(num)) { + nonNumericValues.push(value); + + return null; + } + return num; } /** @internal */ @@ -406,10 +429,12 @@ export function getSplittedSeries( splittedSeries: Map; seriesCollection: Map; xValues: Set; + fallbackScale?: ScaleType; } { const splittedSeries = new Map(); const seriesCollection = new Map(); const xValues: Set = new Set(); + let isNumberArray = true; let isOrdinalScale = false; // eslint-disable-next-line no-restricted-syntax for (const spec of seriesSpecs) { @@ -437,6 +462,9 @@ export function getSplittedSeries( // eslint-disable-next-line no-restricted-syntax for (const xValue of dataSeries.xValues) { + if (isNumberArray && typeof xValue !== 'number') { + isNumberArray = false; + } xValues.add(xValue); } } @@ -445,7 +473,8 @@ export function getSplittedSeries( splittedSeries, seriesCollection, // keep the user order for ordinal scales - xValues: isOrdinalScale ? xValues : new Set([...xValues].sort()), + xValues: (isOrdinalScale || !isNumberArray) ? xValues : new Set([...xValues].sort()), + fallbackScale: (!isOrdinalScale && !isNumberArray) ? ScaleType.Ordinal : undefined, }; } diff --git a/src/state/chart_state.ts b/src/state/chart_state.ts index 8922ceed1d..603eeb33ad 100644 --- a/src/state/chart_state.ts +++ b/src/state/chart_state.ts @@ -30,6 +30,7 @@ import { Spec, PointerEvent } from '../specs'; import { DEFAULT_SETTINGS_SPEC } from '../specs/constants'; import { Color } from '../utils/commons'; import { Dimensions } from '../utils/dimensions'; +import { Logger } from '../utils/logger'; import { Point } from '../utils/point'; import { StateActions } from './actions'; import { CHART_RENDERED } from './actions/chart'; @@ -408,8 +409,7 @@ function findMainChartType(specs: SpecList): ChartTypes | null { // https://stackoverflow.com/questions/55012174/why-doesnt-object-keys-return-a-keyof-type-in-typescript const chartTypes = Object.keys(types).filter((type) => type !== ChartTypes.Global); if (chartTypes.length > 1) { - // eslint-disable-next-line no-console - console.warn('Multiple chart type on the same configuration'); + Logger.warn('Multiple chart type on the same configuration'); return null; } return chartTypes[0] as ChartTypes; diff --git a/src/utils/commons.ts b/src/utils/commons.ts index 5fac858252..49bfce8d84 100644 --- a/src/utils/commons.ts +++ b/src/utils/commons.ts @@ -326,11 +326,6 @@ export function mergePartial( return getPartialValue(baseClone, partial, additionalPartials); } -/** @internal */ -export function isNumberArray(value: unknown): value is number[] { - return Array.isArray(value) && value.every((element) => typeof element === 'number'); -} - /** @internal */ export function getUniqueValues(fullArray: T[], uniqueProperty: keyof T): T[] { return fullArray.reduce<{ diff --git a/src/utils/logger.ts b/src/utils/logger.ts new file mode 100644 index 0000000000..89366b00a9 --- /dev/null +++ b/src/utils/logger.ts @@ -0,0 +1,66 @@ +/* + * 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. + */ + +/* eslint-disable no-console */ + +/** + * Helper class to assist with logging warnings + * + * @internal + * @todo Add more helpful messages in dev for configuration errors + */ +export class Logger { + static namespace = '[@elastic/chart]'; + + /** + * Log warning to console + * + * @param message + * @param optionalParams + */ + static warn(message?: any, ...optionalParams: any[]) { + if (Logger.isDevelopment()) { + console.warn(`${Logger.namespace} ${message}`, ...optionalParams); + } + } + + /** + * Log error to console + * + * @param message + * @param optionalParams + */ + static error(message?: any, ...optionalParams: any[]) { + if (Logger.isDevelopment()) { + console.warn(`${Logger.namespace} ${message}`, ...optionalParams); + } + } + + /** + * Determined development env + * + * @todo confirm this logic works + * @todo add more robust logic + */ + private static isDevelopment(): boolean { + return process.env.NODE_ENV !== 'production'; + } +} + +/* eslint-enable */