tbonelee commented on code in PR #5257:
URL: https://github.com/apache/zeppelin/pull/5257#discussion_r3294988842
##########
zeppelin-web-angular/projects/zeppelin-react/src/components/visualizations/TableVisualization.tsx:
##########
@@ -75,72 +74,132 @@ export const TableVisualization = ({ result, config }:
TableVisualizationProps)
y: parseFloat(row[1] || '0') || 0
}));
- let chart: Column | Line | Pie | Scatter | null = null;
+ chartRef.current.innerHTML = '';
+
+ let chart: any = null;
Review Comment:
**issue: `let chart: any` and `let chartConfig: any` violate
`@typescript-eslint/no-explicit-any` (set to `error` in this project)**
`npm run lint` fails on both lines. Beyond the lint rule, the migration also
loses the previous `Column | Line | Pie | Scatter` discrimination. Chart.js 4
ships full generics (`ChartConfiguration<TType>`, `Chart<TType>`), so we can
satisfy the rule and tighten the original type safety in one go.
Concretely: drop the `chartConfig: any` intermediate variable and call `new
Chart<TType>(ctx, {...})` inline per case. The literal `type: 'bar'` narrows
`data.datasets[N]` and `options` to the bar-specific shape, so typos like
`data.map(d => data.value)` would be caught at compile time.
```ts
import type { Chart as ChartJS } from 'chart.js';
// ...
let chart: ChartJS | null = null;
import('chart.js/auto').then(module => {
// ...
switch (currentMode) {
case 'multiBarChart':
chart = new Chart<'bar'>(ctx, {
type: 'bar',
data: {
labels: data.map(d => d.category),
datasets: [{ label: 'Value', data: data.map(d => d.value),
backgroundColor: '#1890ff' }]
},
options: { responsive: true, maintainAspectRatio: false }
});
break;
case 'lineChart':
chart = new Chart<'line'>(ctx, { /* ... */ });
break;
// pieChart -> Chart<'pie'>, scatterChart -> Chart<'scatter'>,
// stackedAreaChart -> Chart<'line'>
}
});
```
Same pattern for the remaining four cases. Tested locally: no runtime
change, both `any` lint warnings go away, and the per-case dataset shape is now
type-checked.
##########
zeppelin-web-angular/projects/zeppelin-react/src/components/visualizations/TableVisualization.tsx:
##########
@@ -75,72 +74,132 @@ export const TableVisualization = ({ result, config }:
TableVisualizationProps)
y: parseFloat(row[1] || '0') || 0
}));
- let chart: Column | Line | Pie | Scatter | null = null;
+ chartRef.current.innerHTML = '';
+
+ let chart: any = null;
let cancelled = false;
- import('@antv/g2plot').then(g2plot => {
+ import('chart.js/auto').then(module => {
if (cancelled || !chartRef.current) return;
+ const Chart = module.Chart || module.default;
+
+ const canvas = document.createElement('canvas');
+ canvas.style.width = '100%';
+ canvas.style.height = '100%';
+ chartRef.current.appendChild(canvas);
+
+ const ctx = canvas.getContext('2d');
+ if (!ctx) return;
+
+ let chartConfig: any = {};
switch (currentMode) {
case 'multiBarChart':
- chart = new g2plot.Column(chartRef.current, {
- data,
- xField: 'category',
- yField: 'value',
- color: '#1890ff',
- columnWidthRatio: 0.8
- });
+ chartConfig = {
+ type: 'bar',
+ data: {
+ labels: data.map(d => d.category),
+ datasets: [{
+ label: 'Value',
+ data: data.map(d => d.value),
+ backgroundColor: '#1890ff'
+ }]
+ },
+ options: {
+ responsive: true,
+ maintainAspectRatio: false
+ }
+ };
break;
case 'lineChart':
- chart = new g2plot.Line(chartRef.current, {
- data,
- xField: 'category',
- yField: 'value',
- color: '#1890ff'
- });
+ chartConfig = {
+ type: 'line',
+ data: {
+ labels: data.map(d => d.category),
+ datasets: [{
+ label: 'Value',
+ data: data.map(d => d.value),
+ borderColor: '#1890ff',
+ backgroundColor: 'rgba(24, 144, 255, 0.1)',
+ tension: 0.1
+ }]
+ },
+ options: {
+ responsive: true,
+ maintainAspectRatio: false
+ }
+ };
break;
case 'pieChart':
- chart = new g2plot.Pie(chartRef.current, {
- data,
- angleField: 'value',
- colorField: 'category'
- });
+ chartConfig = {
+ type: 'pie',
+ data: {
+ labels: data.map(d => d.category),
+ datasets: [{
+ data: data.map(d => d.value),
+ backgroundColor: [
+ '#1890ff', '#2fc25b', '#facc14', '#223273', '#8543e0',
'#13c2c2', '#3436c7', '#f04864'
+ ]
+ }]
+ },
+ options: {
+ responsive: true,
+ maintainAspectRatio: false
+ }
+ };
break;
case 'scatterChart':
- chart = new g2plot.Scatter(chartRef.current, {
- data,
- xField: 'x',
- yField: 'y',
- color: '#1890ff'
- });
+ chartConfig = {
+ type: 'scatter',
+ data: {
+ datasets: [{
+ label: 'Value',
+ data: data.map(d => ({ x: d.x, y: d.y })),
+ backgroundColor: '#1890ff'
+ }]
+ },
+ options: {
+ responsive: true,
+ maintainAspectRatio: false,
+ scales: {
+ x: { type: 'linear', position: 'bottom' }
+ }
+ }
+ };
break;
case 'stackedAreaChart':
- chart = new g2plot.Line(chartRef.current, {
- data,
- xField: 'category',
- yField: 'value',
- color: '#1890ff',
- point: {
- size: 3,
- shape: 'circle'
+ chartConfig = {
+ type: 'line',
+ data: {
+ labels: data.map(d => d.category),
+ datasets: [{
+ label: 'Value',
+ data: data.map(d => data.value),
+ borderColor: '#1890ff',
+ backgroundColor: 'rgba(24, 144, 255, 0.2)',
+ fill: true,
+ tension: 0.1
+ }]
},
- lineStyle: {
- lineWidth: 2
+ options: {
+ responsive: true,
+ maintainAspectRatio: false
}
- });
+ };
break;
}
- if (chart) {
- chart.render();
- }
+ chart = new Chart(ctx, chartConfig);
});
return () => {
cancelled = true;
if (chart) {
chart.destroy();
}
+ if (chartRef.current) {
+ chartRef.current.innerHTML = '';
+ }
Review Comment:
**issue: cleanup references `chartRef.current`, which violates
`react-hooks/exhaustive-deps` (set to `error` in this project)**
`npm run lint` fails on the cleanup line. In this specific component the ref
is unlikely to actually diverge between effect runs (the chart `<div>` is
identity-stable across chart-mode switches, and on switching to the `table`
mode the ref simply goes to `null`, which the existing `if (chartRef.current)`
guard handles), so this is not a runtime bug. But the project has the rule at
`error`, so it still needs to be fixed.
The canonical fix is to capture the ref to a local `const` at effect start
and use it everywhere (initial guard, the dynamic `import().then()` callback,
and the cleanup return). This satisfies the rule without an `eslint-disable`
and makes the lifecycle binding explicit.
```ts
useEffect(() => {
const container = chartRef.current;
if (!container || !tableData || tableData.rows.length === 0 || currentMode
=== 'table') return;
container.innerHTML = '';
let chart: ChartJS | null = null;
let cancelled = false;
import('chart.js/auto').then(module => {
if (cancelled) return;
// ... use `container.appendChild(canvas)` instead of
`chartRef.current.appendChild(...)`
});
return () => {
cancelled = true;
if (chart) chart.destroy();
container.innerHTML = '';
};
}, [currentMode, tableData]);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
