From 654af9a48d119688f7955be80dd8afd21cadf6ee Mon Sep 17 00:00:00 2001 From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com> Date: Wed, 1 Feb 2023 17:02:20 +0000 Subject: [PATCH] [v9.4.x] Explore: Fix graph not updating when changing config (#62706) Explore: Fix graph not updating when changing config (#62473) * Explore: Fix graph not updating when changing config * move useStructureRev to a seprate hook and add specific tests (cherry picked from commit 6b6b7332299732d364b0fadce0f176698178f5f0) Co-authored-by: Giordano Ricci --- .../features/explore/Graph/ExploreGraph.tsx | 38 +++--- .../explore/Graph/useStructureRev.test.ts | 117 ++++++++++++++++++ .../features/explore/Graph/useStructureRev.ts | 20 +++ 3 files changed, 153 insertions(+), 22 deletions(-) create mode 100644 public/app/features/explore/Graph/useStructureRev.test.ts create mode 100644 public/app/features/explore/Graph/useStructureRev.ts diff --git a/public/app/features/explore/Graph/ExploreGraph.tsx b/public/app/features/explore/Graph/ExploreGraph.tsx index 97fff0f6134..86af63d3d4e 100644 --- a/public/app/features/explore/Graph/ExploreGraph.tsx +++ b/public/app/features/explore/Graph/ExploreGraph.tsx @@ -1,7 +1,6 @@ import { css, cx } from '@emotion/css'; import { identity } from 'lodash'; import React, { useEffect, useMemo, useState } from 'react'; -import { useCounter } from 'react-use'; import { AbsoluteTimeRange, @@ -37,6 +36,7 @@ import { ExploreGraphStyle } from 'app/types'; import { seriesVisibilityConfigFactory } from '../../dashboard/dashgrid/SeriesVisibilityConfigFactory'; import { applyGraphStyle } from './exploreGraphStyleUtils'; +import { useStructureRev } from './useStructureRev'; const MAX_NUMBER_OF_TIME_SERIES = 20; @@ -76,7 +76,15 @@ export function ExploreGraph({ const theme = useTheme2(); const style = useStyles2(getStyles); const [showAllTimeSeries, setShowAllTimeSeries] = useState(false); - const [structureRev, { inc }] = useCounter(0); + + const timeRange = { + from: dateTime(absoluteRange.from), + to: dateTime(absoluteRange.to), + raw: { + from: dateTime(absoluteRange.from), + to: dateTime(absoluteRange.to), + }, + }; const fieldConfigRegistry = useMemo( () => createFieldConfigRegistry(getGraphFieldConfig(defaultGraphConfig), 'Explore'), @@ -98,34 +106,20 @@ export function ExploreGraph({ overrides: [], }); - const timeRange = { - from: dateTime(absoluteRange.from), - to: dateTime(absoluteRange.to), - raw: { - from: dateTime(absoluteRange.from), - to: dateTime(absoluteRange.to), - }, - }; - const styledFieldConfig = useMemo(() => applyGraphStyle(fieldConfig, graphStyle), [fieldConfig, graphStyle]); const dataWithConfig = useMemo(() => { return applyFieldOverrides({ fieldConfig: styledFieldConfig, - data, + data: showAllTimeSeries ? data : data.slice(0, MAX_NUMBER_OF_TIME_SERIES), timeZone, replaceVariables: (value) => value, // We don't need proper replace here as it is only used in getLinks and we use getFieldLinks theme, fieldConfigRegistry, }); - }, [fieldConfigRegistry, data, timeZone, theme, styledFieldConfig]); + }, [fieldConfigRegistry, data, timeZone, theme, styledFieldConfig, showAllTimeSeries]); - const seriesToShow = showAllTimeSeries ? dataWithConfig : dataWithConfig.slice(0, MAX_NUMBER_OF_TIME_SERIES); - - // We need to increment structureRev when the number of series changes. - // the function passed to useMemo runs during rendering, so when we get a different - // amount of data, structureRev is incremented before we render it - useMemo(inc, [dataWithConfig.length, styledFieldConfig, seriesToShow.length, inc]); + const structureRev = useStructureRev(dataWithConfig); useEffect(() => { if (onHiddenSeriesChanged) { @@ -164,7 +158,7 @@ export function ExploreGraph({ return ( - {dataWithConfig.length > MAX_NUMBER_OF_TIME_SERIES && !showAllTimeSeries && ( + {data.length > MAX_NUMBER_OF_TIME_SERIES && !showAllTimeSeries && (
Showing only {MAX_NUMBER_OF_TIME_SERIES} time series. @@ -174,12 +168,12 @@ export function ExploreGraph({ onClick={() => setShowAllTimeSeries(true)} className={style.showAllButton} > - Show all {dataWithConfig.length} + Show all {data.length}
)} { + lastResult = Number.MIN_SAFE_INTEGER; +}; + +const startCounters = (start: number | Error) => { + // The if is only to make TypeScript happy + if (start instanceof Error) { + expect(start).not.toBeInstanceOf(Error); + return; + } + + lastResult = start; +}; + +beforeAll(() => { + expect.extend({ + toHaveIncremented(received: number[]) { + if (received.length < 2) { + return { + message: () => `expected at least 2 elements, got ${received.length}`, + pass: false, + }; + } + + const pass = received[received.length - 1] > lastResult; + const r = lastResult; + + const message = () => + this.isNot + ? `expected ${received[received.length - 1]} to be equal or lesser than ${r}` + : `expected ${received[received.length - 1]} to be greater than ${r}`; + + lastResult = received[received.length - 1]; + + return { + message, + pass, + }; + }, + }); +}); + +describe('useStructureRev', () => { + afterEach(() => resetCounters()); + + it('should increment only when relevant fields in frame change', () => { + let frames: DataFrame[] = [toDataFrame({ fields: [{ name: 'time', type: FieldType.time, values: [1, 2, 3] }] })]; + const { result, rerender } = renderHook((frames) => useStructureRev(frames), { initialProps: frames }); + startCounters(result.current); + + // When changing number of frames, the structureRev should increment + frames = [...frames, toDataFrame({ fields: [{ name: 'time', type: FieldType.time, values: [1, 2, 3] }] })]; + rerender(frames); + expect(result.all).toHaveIncremented(); + + // Changing RefId should not increment the structure revision + frames[0] = toDataFrame({ + refId: 'A', + fields: [{ name: 'time', type: FieldType.time, values: [1, 2, 3] }], + }); + rerender([...frames]); + expect(result.all).not.toHaveIncremented(); + + // Changing frame name should increment the structure revision + frames[0] = toDataFrame({ + refId: 'A', + name: 'Some Name', + fields: [{ name: 'time', type: FieldType.time, values: [1, 2, 3] }], + }); + rerender([...frames]); + expect(result.all).toHaveIncremented(); + + // Changing frame's fields number should increment the structure revision + frames[0] = toDataFrame({ + refId: 'A', + name: 'Some Name', + fields: [ + { name: 'time', type: FieldType.time, values: [1, 2, 3] }, + { name: 'value', type: FieldType.number, values: [1, 2, 3] }, + ], + }); + rerender([...frames]); + expect(result.all).toHaveIncremented(); + + // Changing a frame's field's config should increment the structure revision + frames[0] = toDataFrame({ + refId: 'A', + name: 'Some Name', + fields: [ + { name: 'time', type: FieldType.time, values: [1, 2, 3] }, + { name: 'value', type: FieldType.number, values: [1, 2, 3], config: { unit: 'ms' } }, + ], + }); + rerender([...frames]); + expect(result.all).toHaveIncremented(); + + // Changing a frame's field's name should increment the structure revision + frames[0] = toDataFrame({ + refId: 'A', + name: 'Some Name', + fields: [ + { name: 'time', type: FieldType.time, values: [1, 2, 3] }, + { name: 'value, but with a different name', type: FieldType.number, values: [1, 2, 3], config: { unit: 'ms' } }, + ], + }); + rerender([...frames]); + expect(result.all).toHaveIncremented(); + }); +}); diff --git a/public/app/features/explore/Graph/useStructureRev.ts b/public/app/features/explore/Graph/useStructureRev.ts new file mode 100644 index 00000000000..23490e4d170 --- /dev/null +++ b/public/app/features/explore/Graph/useStructureRev.ts @@ -0,0 +1,20 @@ +import { useMemo } from 'react'; +import { useCounter, usePrevious } from 'react-use'; + +import { DataFrame, compareArrayValues, compareDataFrameStructures } from '@grafana/data'; + +export function useStructureRev(frames: DataFrame[]) { + const [structureRev, { inc }] = useCounter(0); + const previousFrames = usePrevious(frames); + + // We need to increment structureRev when the number of series changes. + // the function passed to useMemo runs during rendering, so when we get a different + // amount of data, structureRev is incremented before we render it + useMemo(() => { + if (previousFrames && !compareArrayValues(frames, previousFrames, compareDataFrameStructures)) { + inc(); + } + }, [frames, previousFrames, inc]); + + return structureRev; +}