Skip to content

Commit f4bd4ec

Browse files
authored
refactor(frontend) :: enable noImplicitAny
`noImplicitAny` is now enabled. Most of the remaining implicit workarounds rooted from `JSON.parse(data_element.textContent)`. This found 3 bugs due to `as ChartPoint` :: 1. `ChartPoint.x` excluded null, `z` was a number, and `y` excluded the `string[]` two `value` columns return. 2. `ChartSeries.name` was a string, and the test pinning a numeric series name has passed all along. 3. `merged_x_values` compares x values that may be null, which JavaScript reads as zero. Most remaining work arounds we should step back and think big about if we need to upstreaming patches along with the fixes.
1 parent b04d715 commit f4bd4ec

13 files changed

Lines changed: 344 additions & 238 deletions

File tree

‎frontend/src/apexcharts.ts‎

Lines changed: 157 additions & 165 deletions
Large diffs are not rendered by default.

‎frontend/src/chart_data.ts‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
import type { PlotValue } from "./chart_series.ts";
2+
3+
export type DataPoint = {
4+
name: string | number;
5+
x: PlotValue;
6+
y: PlotValue | PlotValue[];
7+
color: PlotValue;
8+
z: PlotValue | undefined;
9+
link: string | undefined;
10+
};
11+
12+
/** A row that draws a line across the chart instead of plotting a point. */
13+
export type ReferenceLine = Record<
14+
"xline" | "xline_end" | "yline" | "yline_end" | "label" | "color",
15+
PlotValue
16+
>;
17+
18+
/** One chart's properties, as `chart.handlebars` encodes them. */
19+
export type ChartData = {
20+
type: string;
21+
time: boolean;
22+
labels: boolean;
23+
marker: number | undefined;
24+
xtitle: string | undefined;
25+
ytitle: string | undefined;
26+
ztitle: string | undefined;
27+
xticks: number | undefined;
28+
yticks: number | undefined;
29+
ystep: number | undefined;
30+
xmin: number | undefined;
31+
ymin: number | undefined;
32+
xmax: number | undefined;
33+
ymax: number | undefined;
34+
toolbar: boolean;
35+
show_legend: boolean;
36+
logarithmic: boolean;
37+
horizontal: boolean;
38+
stacked: boolean;
39+
colors: PlotValue[];
40+
points: DataPoint[];
41+
reference_lines: ReferenceLine[];
42+
};
43+
44+
const plot_value = (value: unknown): PlotValue => {
45+
if (value == null) return null;
46+
if (typeof value === "number" || typeof value === "string") return value;
47+
if (typeof value === "boolean") return Number(value);
48+
return String(value);
49+
};
50+
51+
const text_value = (value: unknown) =>
52+
String(plot_value(value) ?? "") || undefined;
53+
54+
const number_value = (value: unknown) => {
55+
const text = plot_value(value);
56+
const number = text == null || text === "" ? Number.NaN : Number(text);
57+
return Number.isFinite(number) ? number : undefined;
58+
};
59+
60+
const data_point = ([name, x, y, color, z, link]: unknown[]): DataPoint => ({
61+
name: plot_value(name) ?? "",
62+
x: plot_value(x),
63+
y: Array.isArray(y) ? y.map(plot_value) : plot_value(y),
64+
color: plot_value(color),
65+
// ApexCharts reads a point as three-dimensional as soon as it carries a z,
66+
// so a row that never mentioned one must not carry a null.
67+
z: z === undefined ? undefined : plot_value(z),
68+
link: text_value(link),
69+
});
70+
71+
const reference_line = (row: Record<string, unknown>): ReferenceLine => ({
72+
xline: plot_value(row.xline),
73+
xline_end: plot_value(row.xline_end),
74+
yline: plot_value(row.yline),
75+
yline_end: plot_value(row.yline_end),
76+
label: plot_value(row.label),
77+
color: plot_value(row.color),
78+
});
79+
80+
export function read_chart_data(json: string | null): ChartData {
81+
const data = JSON.parse(json ?? "");
82+
const rows = Array.isArray(data.points) ? data.points : [];
83+
return {
84+
type: text_value(data.type) ?? "",
85+
time: !!data.time,
86+
labels: !!data.labels,
87+
marker: number_value(data.marker),
88+
xtitle: text_value(data.xtitle),
89+
ytitle: text_value(data.ytitle),
90+
ztitle: text_value(data.ztitle),
91+
xticks: number_value(data.xticks),
92+
yticks: number_value(data.yticks),
93+
ystep: number_value(data.ystep),
94+
xmin: number_value(data.xmin),
95+
ymin: number_value(data.ymin),
96+
xmax: number_value(data.xmax),
97+
ymax: number_value(data.ymax),
98+
toolbar: !!data.toolbar,
99+
show_legend: data.show_legend == null || !!data.show_legend,
100+
logarithmic: !!data.logarithmic,
101+
horizontal: !!data.horizontal,
102+
stacked: !!data.stacked,
103+
colors: Array.isArray(data.colors) ? data.colors.map(plot_value) : [],
104+
points: rows.filter(Array.isArray).map(data_point),
105+
reference_lines: rows
106+
.filter((row: unknown) => !Array.isArray(row))
107+
.map(reference_line),
108+
};
109+
}

‎frontend/src/chart_series.ts‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
1-
export type XValue = number | string | Date;
1+
export type PlotValue = string | number | null;
2+
export type XValue = PlotValue | Date;
23
export type ChartPoint = {
34
x: XValue;
4-
y: number | string | number[] | null;
5-
z?: number;
5+
y: PlotValue | PlotValue[];
6+
z?: PlotValue;
67
fillColor?: string;
78
link?: string;
89
};
9-
export type ChartSeries = { name: string; data: ChartPoint[] };
10-
export type Series = Map<string, ChartSeries>;
10+
export type ChartSeries = { name: string | number; data: ChartPoint[] };
11+
export type Series = Map<ChartSeries["name"], ChartSeries>;
1112

1213
const NUMERIC_X_CHART_TYPES = ["line", "area", "bar", "scatter", "bubble"];
1314

@@ -21,8 +22,10 @@ const Y_WHEN_A_SERIES_SKIPS_A_LABEL = new Map<string, number | null>([
2122
]);
2223

2324
/** equal x values share a key */
24-
const x_key = (x: XValue): number | string =>
25-
x instanceof Date ? x.getTime() : x;
25+
const x_key = (x: XValue): PlotValue => (x instanceof Date ? x.getTime() : x);
26+
27+
/** A missing x sorts as zero, which is how JavaScript compares it. */
28+
const is_lower = (x: XValue, than: XValue) => (x ?? 0) < (than ?? 0);
2629

2730
const x_is_text = (series: ChartSeries[]) =>
2831
typeof series[0]?.data?.[0]?.x === "string";
@@ -50,11 +53,11 @@ export function xaxis_type_for(
5053
*/
5154
export function merged_x_values(series: ChartSeries[]): XValue[] {
5255
const unread = series.map(({ data }) => data.map(({ x }) => x));
53-
const merged = new Map();
56+
const merged = new Map<PlotValue, XValue>();
5457
while (unread.some((xs) => xs.length > 0)) {
5558
const with_lowest_x = unread
5659
.filter((xs) => xs.length > 0)
57-
.reduce((a, b) => (b[0] < a[0] ? b : a));
60+
.reduce((a, b) => (is_lower(b[0], a[0]) ? b : a));
5861
const x = with_lowest_x.shift() as XValue;
5962
merged.set(x_key(x), x);
6063
}

‎frontend/src/globals.d.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,14 @@
1-
// Names the browser bundle relies on at runtime rather than through an import.
1+
// What the bundle installs on the page at runtime rather than through an
2+
// import: the page's own scripts and the browser tests read these back.
23

34
interface Window {
45
/** Every chart rendered on the page, in the order they were built. */
5-
charts?: unknown[];
6+
charts?: import("./apexcharts.ts").RenderedChart[];
67
/** A Bootstrap a page loaded for itself, preferred over the bundled copy. */
78
bootstrap?: typeof import("@tabler/core").bootstrap;
89
}
10+
11+
interface HTMLElement {
12+
/** Attached by sqlpage_select_dropdown to every select it takes over. */
13+
tomselect?: import("tom-select/popular").default;
14+
}

‎tests/end-to-end/fixture.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,5 +30,5 @@ export const test = base.extend({
3030
},
3131
});
3232

33-
export type { Page } from "@playwright/test";
33+
export type { ConsoleMessage, Page } from "@playwright/test";
3434
export { expect };
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, 'pie' AS type, TRUE AS labels;
2+
WITH points(label, value) AS (VALUES ('Yes', 65), ('No', 35)) SELECT * FROM points;
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
SELECT 'chart' AS component, 'test-chart' AS id, 'Chart test fixture' AS title, 'rangeBar' AS type, TRUE AS time, TRUE AS labels;
2+
SELECT 'Design' AS series, 'Alice' AS label, '2024-03-01' AS value, '2024-03-05' AS value;
3+
SELECT 'Build' AS series, 'Bob' AS label, '2024-03-04' AS value, '2024-03-09' AS value;

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

Lines changed: 42 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,4 @@
1-
import { expect, type Page, test } from "../../fixture.ts";
2-
3-
type ChartPoint = { x: string | number | Date; y: number | null };
4-
5-
declare global {
6-
interface Window {
7-
charts?: {
8-
w: {
9-
config: {
10-
chart: { type: string; stacked: boolean };
11-
xaxis: { type?: string; tickAmount?: number };
12-
series: { name: string | number; data?: ChartPoint[] }[];
13-
tooltip: {
14-
custom?: (args: {
15-
seriesIndex: number;
16-
dataPointIndex: number;
17-
w: unknown;
18-
}) => string;
19-
};
20-
};
21-
globals: { labels: (string | number)[] };
22-
};
23-
}[];
24-
}
25-
}
1+
import { type ConsoleMessage, expect, type Page, test } from "../../fixture.ts";
262

273
const MARKS =
284
".apexcharts-bar-area, .apexcharts-rangebar-area, .apexcharts-treemap-rect, .apexcharts-pie-area, .apexcharts-heatmap-rect, .apexcharts-series .apexcharts-marker";
@@ -32,7 +8,7 @@ const ORANGE = "#f76707";
328
const GREEN = "#37b24d";
339
async function renderChart(page: Page, fixture: string) {
3410
const failures: string[] = [];
35-
const recordError = (message: { type(): string; text(): string }) => {
11+
const recordError = (message: ConsoleMessage) => {
3612
if (message.type() === "error") failures.push(message.text());
3713
};
3814
page.on("console", recordError);
@@ -46,13 +22,15 @@ async function renderChart(page: Page, fixture: string) {
4622
const container = document.getElementById("test-chart");
4723
if (!container) throw new Error("Chart fixture did not render");
4824
const rendered = window.charts?.[0];
49-
const series = (rendered?.w.config.series ?? []).map((s) => ({
50-
name: s.name,
51-
points: (s.data ?? []).map((p) => [
52-
p.x instanceof Date ? p.x.toISOString() : p.x,
53-
p.y,
54-
]),
55-
}));
25+
const series = (rendered?.w.config.series ?? [])
26+
.filter((s) => typeof s !== "number")
27+
.map((s) => ({
28+
name: s.name,
29+
points: s.data.map((p) => [
30+
p.x instanceof Date ? p.x.toISOString() : p.x,
31+
p.y,
32+
]),
33+
}));
5634
const drawnPerSeries = series.map(({ name }) => {
5735
const markers = [
5836
...container.querySelectorAll<SVGGraphicsElement>(
@@ -122,9 +100,12 @@ async function renderChart(page: Page, fixture: string) {
122100
tickAmount: rendered?.w.config.xaxis.tickAmount ?? null,
123101
},
124102
generatedLabels: rendered?.w.globals.labels ?? [],
103+
threeDimensional: rendered?.w.globals.isDataXYZ ?? null,
125104
axisLabels,
126105
dataLabels: [
127-
...container.querySelectorAll(".apexcharts-datalabel"),
106+
...container.querySelectorAll(
107+
".apexcharts-datalabel, .apexcharts-pie-label",
108+
),
128109
].map((label) => label.textContent),
129110
barGroups,
130111
series,
@@ -407,6 +388,16 @@ for (const type of ["area", "scatter", "heatmap"]) {
407388
});
408389
}
409390

391+
test("counts a third dimension only where the rows carried one", async ({
392+
page,
393+
}) => {
394+
const flat = await renderChart(page, "index");
395+
const bubbles = await renderChart(page, "bubble-categories");
396+
397+
expect(flat.threeDimensional).toBe(false);
398+
expect(bubbles.threeDimensional).toBe(true);
399+
});
400+
410401
test("keeps the bubble size of the points it lined up", async ({ page }) => {
411402
const chart = await renderChart(page, "bubble-categories");
412403

@@ -455,6 +446,22 @@ test("gives the tooltip title the color of the tooltip around it", async ({
455446
expect(colors.title).toBe(colors.tooltip);
456447
});
457448

449+
test("names the series of every bar of a range bar chart", async ({ page }) => {
450+
const chart = await renderChart(page, "labeled-range-bar");
451+
452+
expect(chart.failures).toEqual([]);
453+
expect(chart.dataLabels).toEqual(["Design", "Build"]);
454+
});
455+
456+
test("gives every slice of a pie chart its label and its share", async ({
457+
page,
458+
}) => {
459+
const chart = await renderChart(page, "labeled-pie");
460+
461+
expect(chart.failures).toEqual([]);
462+
expect(chart.dataLabels).toEqual(["Yes: 65%", "No: 35%"]);
463+
});
464+
458465
test("draws a reference line that carries no label", async ({ page }) => {
459466
const chart = await renderChart(page, "unlabeled-reference-lines");
460467

@@ -601,11 +608,7 @@ test("labels each axis of a bubble tooltip with its own title", async ({
601608
const custom = chart.w.config.tooltip.custom;
602609
if (!custom) throw new Error("A bubble chart needs the custom tooltip");
603610
const holder = document.createElement("div");
604-
holder.innerHTML = custom({
605-
seriesIndex: 0,
606-
dataPointIndex: 1,
607-
w: chart.w,
608-
});
611+
holder.innerHTML = custom({ seriesIndex: 0, dataPointIndex: 1 });
609612
const values = holder.querySelectorAll(".apexcharts-tooltip-text-y-value");
610613
return [...holder.querySelectorAll(".apexcharts-tooltip-text-y-label")].map(
611614
(label, i): [string, string] => [

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { expect, type Page, test } from "../../fixture.ts";
1+
import { type ConsoleMessage, expect, type Page, test } from "../../fixture.ts";
22

33
const PARIS_WITHOUT_ITS_LONGITUDE = "48.85,";
44
const NOT_COORDINATES = "somewhere nice";
@@ -7,7 +7,7 @@ async function renderMap(page: Page, fixture: string, markerCount = 0) {
77
const errors: string[] = [];
88
const logged: string[] = [];
99
const recordPageError = (error: Error) => errors.push(error.message);
10-
const recordConsoleError = (message: { type(): string; text(): string }) => {
10+
const recordConsoleError = (message: ConsoleMessage) => {
1111
if (message.type() === "error") logged.push(message.text());
1212
};
1313
page.on("pageerror", recordPageError);

‎tests/end-to-end/globals.d.ts‎

Lines changed: 0 additions & 7 deletions
This file was deleted.

0 commit comments

Comments
 (0)