LG-4803: refactor echarts option configurations and fix exhaustive dependencies (#2812)

* Destructure chart and list exhaustive dependencies for Chart child components

* Add and install @leafygreen-ui/hooks to charts core package

* Refactor echarts options configuration and list exhaustive deps in useEchart and useChart

* Changeset

* Address feedback

Author: stephl3

.changeset/tasty-trainers-brake.md

@@ -0,0 +1,5 @@
+---
+'@lg-charts/core': patch
+---
+
+[LG-4803](https://jira.mongodb.org/browse/LG-4803): fix exhaustive dependencies issues and refactor echarts options configuration 

charts/core/package.json

@@ -18,6 +18,7 @@
     "@dnd-kit/sortable": "^10.0.0",
     "@dnd-kit/utilities": "^3.2.2",
     "@leafygreen-ui/emotion": "workspace:^",
+    "@leafygreen-ui/hooks": "workspace:^",
     "@leafygreen-ui/lib": "workspace:^",
     "@leafygreen-ui/palette": "workspace:^",
     "@leafygreen-ui/tokens": "workspace:^",

charts/core/src/Chart/config/getDefaultChartOptions.ts

@@ -10,6 +10,7 @@ import {
 } from '@leafygreen-ui/tokens';
 
 import { ChartOptions } from '../Chart.types';
+import { TOOLBOX_ID, X_AXIS_ID, Y_AXIS_ID } from '../constants';
 
 const commonAxisOptions = {
   /**
@@ -66,17 +67,20 @@ export const getDefaultChartOptions = (
   },
 
   xAxis: {
+    id: X_AXIS_ID,
     type: 'time',
     ...commonAxisOptions,
   },
 
   yAxis: {
+    id: Y_AXIS_ID,
     type: 'value',
     ...commonAxisOptions,
   },
 
   // Sets up zooming
   toolbox: {
+    id: TOOLBOX_ID,
     feature: {
       dataZoom: {
         show: true,

charts/core/src/Chart/constants.ts

@@ -0,0 +1,3 @@
+export const TOOLBOX_ID = 'toolbox';
+export const X_AXIS_ID = 'x-axis';
+export const Y_AXIS_ID = 'y-axis';

charts/core/src/Chart/hooks/updateUtils.spec.ts

@@ -1,66 +1,50 @@
 import { ChartOptions } from '../Chart.types';
 
-import { addSeries, removeSeries, updateOptions } from './updateUtils';
+import {
+  getOptionsToUpdateWithAddedSeries,
+  getOptionsToUpdateWithRemovedSeries,
+} from './updateUtils';
 
 describe('@lg-charts/core/Chart/hooks/updateUtils', () => {
-  test('addSeries should add a series to the chart options', () => {
+  test('getOptionsToUpdateWithAddedSeries should return options with an added series', () => {
     const currentOptions: Partial<ChartOptions> = {
-      series: [{ name: 'series1' }],
+      series: [{ id: 'series-1' }],
     };
-    const newSeriesName = 'series2';
-    const data = { name: newSeriesName };
-    const updatedOptions = addSeries(currentOptions, data);
+    const newSeriesId = 'series-2';
+    const data = { id: newSeriesId };
+    const updatedOptions = getOptionsToUpdateWithAddedSeries(
+      currentOptions,
+      data,
+    );
     expect(updatedOptions.series).toHaveLength(2);
-    expect(updatedOptions.series?.[1].name).toBe(newSeriesName);
+    expect(updatedOptions.series?.[1].id).toBe(newSeriesId);
   });
 
-  test('addSeries should not add a series if a chart with the same name exists', () => {
+  test('getOptionsToUpdateWithAddedSeries should return options without an added series if a series with the same id exists', () => {
     const currentOptions: Partial<ChartOptions> = {
-      series: [{ name: 'series1' }],
+      series: [{ id: 'series-1' }],
     };
-    const newSeriesName = 'series1';
-    const data = { name: newSeriesName };
-    const updatedOptions = addSeries(currentOptions, data);
+    const existingSeriesId = 'series-1';
+    const data = { id: existingSeriesId };
+    const updatedOptions = getOptionsToUpdateWithAddedSeries(
+      currentOptions,
+      data,
+    );
     expect(updatedOptions.series).toHaveLength(1);
-    expect(updatedOptions.series?.[0].name).toBe(newSeriesName);
+    expect(updatedOptions.series?.[0].id).toBe(existingSeriesId);
   });
 
-  test('removeSeries should remove a series from the chart options', () => {
+  test('getOptionsToUpdateWithRemovedSeries should return options with a removed series', () => {
     const currentOptions: Partial<ChartOptions> = {
-      series: [{ name: 'series1' }, { name: 'series2' }],
+      series: [{ id: 'series-1' }, { id: 'series-2' }],
     };
-    const seriesName1 = 'series1';
-    const seriesName2 = 'series2';
-    const updatedOptions = removeSeries(currentOptions, seriesName1);
+    const seriesId1 = 'series-1';
+    const seriesId2 = 'series-2';
+    const updatedOptions = getOptionsToUpdateWithRemovedSeries(
+      currentOptions,
+      seriesId1,
+    );
     expect(updatedOptions.series).toHaveLength(1);
-    expect(updatedOptions.series?.[0].name).toBe(seriesName2);
-  });
-
-  /**
-   * Tests that option updates don't overwrite the entire chart options object.
-   */
-  test('updateOptions should merge chart options non-destructively', () => {
-    const currentOptions: Partial<ChartOptions> = {
-      xAxis: {
-        show: true,
-        splitLine: {
-          show: true,
-        },
-      },
-    };
-    const updatedOptions = updateOptions(currentOptions, {
-      xAxis: {
-        show: false, // This should only update the show property and not other properties
-      },
-      grid: {
-        show: true,
-      },
-    });
-    // @ts-ignore: Property 'show' does not exist on type 'Arrayable<AriaOption>'.
-    expect(updatedOptions?.xAxis?.show).toBe(false);
-    // @ts-ignore: Property 'show' does not exist on type 'Arrayable<AriaOption>'.
-    expect(updatedOptions?.xAxis?.splitLine?.show).toBe(true);
-    // @ts-ignore: Property 'show' does not exist on type 'Arrayable<GridOption>'.
-    expect(updatedOptions?.grid?.show).toBe(true);
+    expect(updatedOptions.series?.[0].id).toBe(seriesId2);
   });
 });

charts/core/src/Chart/hooks/updateUtils.ts

@@ -1,69 +1,49 @@
+import { ECBasicOption } from 'echarts/types/dist/shared';
+
 import { ChartOptions, SeriesOption } from '../Chart.types';
 
-export function addSeries(
-  currentOptions: ChartOptions,
+export function getOptionsToUpdateWithAddedSeries(
+  currentOptions: ECBasicOption | undefined,
   data: SeriesOption,
 ): Partial<ChartOptions> {
-  const updatedOptions = { ...currentOptions };
-
-  if (!updatedOptions.series) {
-    updatedOptions.series = [data];
-  } else {
-    if (!updatedOptions.series.some(series => series.name === data.name)) {
-      updatedOptions.series.push(data);
-    }
-  }
-
-  return updatedOptions;
-}
-
-export function removeSeries(
-  currentOptions: ChartOptions,
-  name: string,
-): Partial<ChartOptions> {
-  const updatedOptions = { ...currentOptions };
-
-  if (updatedOptions.series) {
-    updatedOptions.series = [
-      ...updatedOptions.series.filter(series => series.name !== name),
-    ];
-  }
-
-  return updatedOptions;
-}
-
-/**
- * Method to recursively merge two objects. It should update keys if they
- * already exist and add them if they don't. However, it shouldn't completely
- * overwrite a key it's an already existing object.
- *
- * They goal is to allow for partial updates to the chart options object.
- */
-function recursiveMerge(
-  target: { [key: string]: any },
-  source: { [key: string]: any },
-) {
-  const updatedObj = { ...target };
-
-  for (const key in source) {
-    if (
-      typeof source[key] === 'object' &&
-      typeof updatedObj[key] === 'object'
-    ) {
-      // Recursively update nested objects
-      updatedObj[key] = recursiveMerge(updatedObj[key], source[key]);
-    } else {
-      // Update or add the value for the key
-      updatedObj[key] = source[key];
-    }
+  const prevSeries =
+    currentOptions && Array.isArray(currentOptions?.series)
+      ? currentOptions.series
+      : [];
+
+  const hasSeriesData = prevSeries.some(
+    series => series.id && series.id === data.id,
+  );
+
+  if (hasSeriesData) {
+    return {
+      series: prevSeries,
+    };
   }
 
-  return updatedObj;
+  return {
+    series: [...prevSeries, data],
+  };
 }
 
-export function updateOptions(
-  currentOptions: ChartOptions,
-  options: Partial<ChartOptions>,
+export function getOptionsToUpdateWithRemovedSeries(
+  currentOptions: ECBasicOption | undefined,
+  id: string,
 ): Partial<ChartOptions> {
-  return recursiveMerge(currentOptions, options);
+  const prevSeries =
+    currentOptions &&
+    currentOptions.series &&
+    Array.isArray(currentOptions?.series)
+      ? currentOptions.series
+      : [];
+
+  const filteredSeries = prevSeries.filter(series => {
+    if (!series) return true;
+
+    return series.id && series.id !== id;
+  });
+
+  return {
+    series: filteredSeries,
+  };
 }