Browse Source

ref(colors) make color pallete readonly (#82922)

Use readonly so we dont end up mutating the colors by mistake
(dangerously close in two places). Using a const arr means we can also
drop the non null assertions and guard from out of bounds access.

The getColorPallete should also return a T | undefined as there is
nothing enforcing access inside the array indices
Jonas 2 months ago
parent
commit
328cad0432

+ 12 - 5
static/app/chartcuterie/discover.tsx

@@ -173,7 +173,10 @@ discoverCharts.push({
 
     const stats = Object.values(data.stats);
     const hasOther = Object.keys(data.stats).includes('Other');
-    const color = theme.charts.getColorPalette(stats.length - 2 - (hasOther ? 1 : 0));
+    const color = theme.charts
+      .getColorPalette(stats.length - 2 - (hasOther ? 1 : 0))
+      ?.slice() as string[];
+
     if (hasOther) {
       color.push(theme.chartOther);
     }
@@ -232,7 +235,9 @@ discoverCharts.push({
 
     const stats = Object.values(data.stats);
     const hasOther = Object.keys(data.stats).includes('Other');
-    const color = theme.charts.getColorPalette(stats.length - 2 - (hasOther ? 1 : 0));
+    const color = theme.charts
+      .getColorPalette(stats.length - 2 - (hasOther ? 1 : 0))
+      ?.slice() as string[];
     if (hasOther) {
       color.push(theme.chartOther);
     }
@@ -290,9 +295,11 @@ discoverCharts.push({
 
     const stats = Object.values(data.stats);
     const hasOther = Object.keys(data.stats).includes('Other');
-    const color = theme.charts.getColorPalette(stats.length - 2 - (hasOther ? 1 : 0));
+    const color = theme.charts
+      .getColorPalette(stats.length - 2 - (hasOther ? 1 : 0))
+      ?.slice() as string[] | undefined;
     if (hasOther) {
-      color.push(theme.chartOther);
+      color?.push(theme.chartOther);
     }
 
     const series = stats
@@ -364,7 +371,7 @@ discoverCharts.push({
     const stats = Object.keys(data.stats).map(key =>
       Object.assign({}, {key}, data.stats[key])
     );
-    const color = theme.charts.getColorPalette(stats.length - 2);
+    const color = theme.charts.getColorPalette(stats.length - 2) ?? [];
     const previousPeriodColor = lightenHexToRgb(color);
 
     const areaSeries: SeriesOption[] = [];

+ 2 - 2
static/app/components/charts/baseChart.tsx

@@ -152,7 +152,7 @@ export interface BaseChartProps {
    * Array of color codes to use in charts. May also take a function which is
    * provided with the current theme
    */
-  colors?: string[] | ((theme: Theme) => string[]);
+  colors?: string[] | readonly string[] | ((theme: Theme) => string[]);
   'data-test-id'?: string;
   /**
    * DataZoom (allows for zooming of chart)
@@ -390,7 +390,7 @@ function BaseChartUnwrapped({
   const theme = useTheme();
 
   const resolveColors =
-    colors !== undefined ? (Array.isArray(colors) ? colors : colors(theme)) : null;
+    colors !== undefined ? (typeof colors === 'function' ? colors(theme) : colors) : null;
 
   const color =
     resolveColors ||

+ 3 - 1
static/app/components/charts/eventsChart.tsx

@@ -266,7 +266,9 @@ class Chart extends Component<ChartProps, State> {
     }
     const chartColors = timeseriesData.length
       ? colors?.slice(0, series.length) ?? [
-          ...theme.charts.getColorPalette(timeseriesData.length - 2 - (hasOther ? 1 : 0)),
+          ...(theme.charts.getColorPalette(
+            timeseriesData.length - 2 - (hasOther ? 1 : 0)
+          ) ?? []),
         ]
       : undefined;
     if (chartColors?.length && hasOther) {

+ 1 - 1
static/app/components/charts/miniBarChart.tsx

@@ -141,7 +141,7 @@ interface Props extends Omit<BaseChartProps, 'css' | 'colors' | 'series' | 'heig
   /**
    * Colors to use on the chart.
    */
-  colors?: string[];
+  colors?: ReadonlyArray<string>;
 
   /**
    * A list of colors to use on hover.

+ 3 - 1
static/app/components/charts/pieChart.tsx

@@ -101,7 +101,9 @@ class PieChart extends Component<Props> {
       <BaseChart
         ref={this.chart}
         colors={
-          firstSeries?.data && [...theme.charts.getColorPalette(firstSeries.data.length)]
+          firstSeries?.data && [
+            ...(theme.charts.getColorPalette(firstSeries.data.length) ?? []),
+          ]
         }
         // when legend highlights it does NOT pass dataIndex :(
         onHighlight={({name}) => {

+ 1 - 1
static/app/components/performance/waterfall/utils.spec.tsx

@@ -16,7 +16,7 @@ describe('pickBarColor()', function () {
   });
 
   it('returns a random color when no predefined option is available', function () {
-    const colorsAsArray = Object.keys(CHART_PALETTE).map(key => CHART_PALETTE[17]![key]);
+    const colorsAsArray = Object.keys(CHART_PALETTE).map(key => CHART_PALETTE[17][key]);
 
     let randomColor = pickBarColor('a normal string');
     expect(colorsAsArray).toContain(randomColor);

+ 6 - 6
static/app/components/performance/waterfall/utils.tsx

@@ -238,13 +238,13 @@ const getLetterIndex = (letter: string): number => {
   return index === -1 ? 0 : index;
 };
 
-const colorsAsArray = Object.keys(CHART_PALETTE).map(key => CHART_PALETTE[17]![key]);
+const colorsAsArray = Object.keys(CHART_PALETTE).map(key => CHART_PALETTE[17][key]);
 
 export const barColors = {
-  default: CHART_PALETTE[17]![4],
-  transaction: CHART_PALETTE[17]![8],
-  http: CHART_PALETTE[17]![10],
-  db: CHART_PALETTE[17]![17],
+  default: CHART_PALETTE[17][4],
+  transaction: CHART_PALETTE[17][8],
+  http: CHART_PALETTE[17][10],
+  db: CHART_PALETTE[17][17],
 };
 
 export const pickBarColor = (input: string | undefined): string => {
@@ -252,7 +252,7 @@ export const pickBarColor = (input: string | undefined): string => {
   // That way colors stay consistent between transactions.
 
   if (!input || input.length < 3) {
-    return CHART_PALETTE[17]![4]!;
+    return CHART_PALETTE[17][4];
   }
 
   if (barColors[input]) {

+ 1 - 1
static/app/components/userMisery.tsx

@@ -21,7 +21,7 @@ function UserMisery(props: Props) {
   // 0 User Misery while still preserving the actual value for sorting purposes.
   const adjustedMisery = userMisery > 0.05 ? userMisery : 0;
 
-  const palette = new Array(bars).fill([CHART_PALETTE[0]![0]]);
+  const palette = new Array(bars).fill([CHART_PALETTE[0][0]]);
   const score = Math.round(adjustedMisery * palette.length);
 
   let title: React.ReactNode;

+ 1 - 1
static/app/constants/chartPalette.tsx

@@ -176,4 +176,4 @@ export const CHART_PALETTE = [
     '#f4aa27',
     '#f2b712',
   ],
-];
+] as const;

+ 6 - 6
static/app/utils/profiling/flamegraph/flamegraphTheme.tsx

@@ -203,10 +203,10 @@ export const LightFlamegraphTheme: FlamegraphTheme = {
       'by frequency': makeColorMapByFrequency,
       'by system vs application frame': makeColorMapBySystemVsApplicationFrame,
     },
-    CPU_CHART_COLORS: CHART_PALETTE[12]!.map(c => hexToColorChannels(c, 0.8)),
+    CPU_CHART_COLORS: CHART_PALETTE[12].map(c => hexToColorChannels(c, 0.8)),
     MEMORY_CHART_COLORS: [
-      hexToColorChannels(CHART_PALETTE[4]![2]!, 0.8),
-      hexToColorChannels(CHART_PALETTE[4]![3]!, 0.8),
+      hexToColorChannels(CHART_PALETTE[4][2], 0.8),
+      hexToColorChannels(CHART_PALETTE[4][3], 0.8),
     ],
     CHART_CURSOR_INDICATOR: 'rgba(31,35,58,.75)',
     CHART_LABEL_COLOR: 'rgba(31,35,58,.75)',
@@ -257,10 +257,10 @@ export const DarkFlamegraphTheme: FlamegraphTheme = {
       'by frequency': makeColorMapByFrequency,
       'by system vs application frame': makeColorMapBySystemVsApplicationFrame,
     },
-    CPU_CHART_COLORS: CHART_PALETTE[12]!.map(c => hexToColorChannels(c, 0.8)),
+    CPU_CHART_COLORS: CHART_PALETTE[12].map(c => hexToColorChannels(c, 0.8)),
     MEMORY_CHART_COLORS: [
-      hexToColorChannels(CHART_PALETTE[4]![2]!, 0.5),
-      hexToColorChannels(CHART_PALETTE[4]![3]!, 0.5),
+      hexToColorChannels(CHART_PALETTE[4][2], 0.5),
+      hexToColorChannels(CHART_PALETTE[4][3], 0.5),
     ],
     CHART_CURSOR_INDICATOR: 'rgba(255, 255, 255, 0.5)',
     CHART_LABEL_COLOR: 'rgba(255, 255, 255, 0.5)',

Some files were not shown because too many files changed in this diff