Skip to content

Commit 86a32c8

Browse files
authored
fix(chart) :: use typed Map instead of Object.prototype
Before loosly typed `Object.protoype` was used to map user inputs to built in properties. This is what a `Map` is supposed to do, so we use that with stong types.
1 parent b5d8ee7 commit 86a32c8

4 files changed

Lines changed: 34 additions & 21 deletions

File tree

‎frontend/src/apexcharts.ts‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -39,23 +39,27 @@ const sqlpage_chart = (() => {
3939
["black", "#000000", "#000000"],
4040
["white", "#ffffff", "#f8f9fa"],
4141
];
42-
const colorNames = Object.fromEntries(
43-
tblrColors.flatMap(([name, dark, light]) => [
42+
const colorNames = new Map(
43+
tblrColors.flatMap(([name, dark, light]): [string, string][] => [
4444
[name, dark],
4545
[`${name}-lt`, light],
4646
]),
4747
);
4848
const isDarkTheme = document.body?.dataset?.bsTheme === "dark";
4949

5050
const STACKABLE_CHART_TYPES = ["line", "area", "bar"];
51-
const APEXCHARTS_TYPE_ALIASES = { column: "bar" };
51+
const STROKE_WIDTHS = new Map([
52+
["area", 3],
53+
["line", 2],
54+
]);
55+
const APEXCHARTS_TYPE_ALIASES = new Map([["column", "bar"]]);
5256

53-
const referenceColor = colorNames[isDarkTheme ? "gray-lt" : "gray"];
57+
const referenceColor = colorNames.get(isDarkTheme ? "gray-lt" : "gray");
5458

5559
type ReferenceLine = { [property: string]: string | number | null };
5660

5761
const named_color = (name: unknown): string | undefined =>
58-
typeof name === "string" ? colorNames[name] : undefined;
62+
typeof name === "string" ? colorNames.get(name) : undefined;
5963

6064
const reference_color = (name: string | number | null) =>
6165
named_color(name) || referenceColor;
@@ -100,7 +104,7 @@ const sqlpage_chart = (() => {
100104
chartContainer.innerHTML = "";
101105
const is_timeseries = !!data.time;
102106
const chart_type =
103-
APEXCHARTS_TYPE_ALIASES[data.type] || data.type || "line";
107+
APEXCHARTS_TYPE_ALIASES.get(data.type) || data.type || "line";
104108
const is_stacked =
105109
!!data.stacked && STACKABLE_CHART_TYPES.includes(chart_type);
106110
const points = data.points.filter(Array.isArray);
@@ -219,11 +223,7 @@ const sqlpage_chart = (() => {
219223
type: chart_type === "area" ? "gradient" : "solid",
220224
},
221225
stroke: {
222-
width:
223-
{
224-
area: 3,
225-
line: 2,
226-
}[chart_type] || 0,
226+
width: STROKE_WIDTHS.get(chart_type) ?? 0,
227227
lineCap: "round",
228228
curve: "smooth",
229229
},

‎frontend/src/chart_series.ts‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,14 @@ export type Series = Map<string, ChartSeries>;
1010

1111
const NUMERIC_X_CHART_TYPES = ["line", "area", "bar", "scatter", "bubble"];
1212

13-
const Y_WHEN_A_SERIES_SKIPS_A_LABEL: Record<string, number | null> = {
14-
bar: 0,
15-
line: null,
16-
area: null,
17-
scatter: null,
18-
bubble: null,
19-
heatmap: null,
20-
};
13+
const Y_WHEN_A_SERIES_SKIPS_A_LABEL = new Map<string, number | null>([
14+
["bar", 0],
15+
["line", null],
16+
["area", null],
17+
["scatter", null],
18+
["bubble", null],
19+
["heatmap", null],
20+
]);
2121

2222
/** equal x values share a key */
2323
const x_key = (x: XValue): number | string =>
@@ -91,7 +91,8 @@ export function align_series_for(
9191
is_stacked: boolean,
9292
): ChartSeries[] {
9393
if (is_stacked) return align_series(series, 0);
94-
if (x_is_text(series) && chart_type in Y_WHEN_A_SERIES_SKIPS_A_LABEL)
95-
return align_series(series, Y_WHEN_A_SERIES_SKIPS_A_LABEL[chart_type]);
94+
const y_when_missing = Y_WHEN_A_SERIES_SKIPS_A_LABEL.get(chart_type);
95+
if (x_is_text(series) && y_when_missing !== undefined)
96+
return align_series(series, y_when_missing);
9697
return series;
9798
}
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
SELECT 'chart' AS component, 'test-chart' AS id, 'Chart test fixture' AS title, 'bar' AS type, 'toString' AS color, 4 AS marker;
2+
WITH points(series, x, y) AS (VALUES ('A', 'Q1', 1), ('A', 'Q2', 2)) SELECT * FROM points;

‎tests/end-to-end/fixtures/chart/test.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -497,6 +497,16 @@ test("keeps the default palette when the chart names a color SQLPage does not kn
497497
expect(fills(unknown)).toEqual(fills(plain));
498498
});
499499

500+
test("draws a chart whose color names a built-in JavaScript property", async ({
501+
page,
502+
}) => {
503+
const plain = await renderChart(page, "uncolored-bar");
504+
const chart = await renderChart(page, "builtin-chart-color");
505+
506+
expect(chart.failures).toEqual([]);
507+
expect(fills(chart)).toEqual(fills(plain));
508+
});
509+
500510
test("renders series named after built-in JavaScript properties", async ({
501511
page,
502512
}) => {

0 commit comments

Comments
 (0)