EnxDev commented on code in PR #37625:
URL: https://github.com/apache/superset/pull/37625#discussion_r2767510869
##########
superset-frontend/plugins/legacy-preset-chart-nvd3/test/utils.test.ts:
##########
@@ -128,11 +128,13 @@ describe('nvd3/utils', () => {
});
it('returns a date formatter if format is smart_date', () => {
const time = new Date(Date.UTC(2018, 10, 21, 22, 11));
- expect(getTimeOrNumberFormatter('smart_date')(time)).toBe('10:11');
+ expect(
+ getTimeOrNumberFormatter('smart_date')(time as unknown as number),
Review Comment:
The double cast is needed because `getTimeOrNumberFormatter doesn’t
distinguish return types. `
We could alternatively use `@ts-expect-error` to document the mismatch and
catch future type fixes upstream
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -157,58 +223,76 @@ function convertToArray(
return result;
}
-export class TableRenderer extends Component {
- constructor(props) {
+export class TableRenderer extends Component<
+ TableRendererProps,
+ TableRendererState
+> {
+ sortCache: Map<string, string[][]>;
+ cachedProps: TableRendererProps | null;
+ cachedBasePivotSettings: PivotSettings | null;
+
+ static propTypes: Record<string, unknown>;
+ static defaultProps: Record<string, unknown>;
+
+ constructor(props: TableRendererProps) {
super(props);
// We need state to record which entries are collapsed and which aren't.
// This is an object with flat-keys indicating if the corresponding rows
// should be collapsed.
this.state = { collapsedRows: {}, collapsedCols: {}, sortingOrder: [] };
this.sortCache = new Map();
+ this.cachedProps = null;
+ this.cachedBasePivotSettings = null;
this.clickHeaderHandler = this.clickHeaderHandler.bind(this);
this.clickHandler = this.clickHandler.bind(this);
}
- getBasePivotSettings() {
+ getBasePivotSettings(): PivotSettings {
// One-time extraction of pivot settings that we'll use throughout the
render.
const { props } = this;
const colAttrs = props.cols;
const rowAttrs = props.rows;
- const tableOptions = {
+ const tableOptions: Record<string, unknown> = {
rowTotals: true,
colTotals: true,
...props.tableOptions,
};
- const rowTotals = tableOptions.rowTotals || colAttrs.length === 0;
- const colTotals = tableOptions.colTotals || rowAttrs.length === 0;
+ const rowTotals =
+ (tableOptions.rowTotals as boolean) || colAttrs.length === 0;
+ const colTotals =
+ (tableOptions.colTotals as boolean) || rowAttrs.length === 0;
const namesMapping = props.namesMapping || {};
- const subtotalOptions = {
+ const subtotalOptions: Record<string, unknown> = {
arrowCollapsed: '\u25B2',
arrowExpanded: '\u25BC',
...props.subtotalOptions,
};
const colSubtotalDisplay = {
displayOnTop: false,
- enabled: tableOptions.colSubTotals,
+ enabled: tableOptions.colSubTotals as boolean | undefined,
hideOnExpand: false,
- ...subtotalOptions.colSubtotalDisplay,
+ ...(subtotalOptions.colSubtotalDisplay as
+ | Record<string, unknown>
+ | undefined),
};
const rowSubtotalDisplay = {
displayOnTop: false,
- enabled: tableOptions.rowSubTotals,
+ enabled: tableOptions.rowSubTotals as boolean | undefined,
hideOnExpand: false,
- ...subtotalOptions.rowSubtotalDisplay,
+ ...(subtotalOptions.rowSubtotalDisplay as
+ | Record<string, unknown>
+ | undefined),
};
- const pivotData = new PivotData(props, {
- rowEnabled: rowSubtotalDisplay.enabled,
- colEnabled: colSubtotalDisplay.enabled,
+ const pivotData = new PivotData(props as Record<string, unknown>, {
+ rowEnabled: rowSubtotalDisplay.enabled as boolean | undefined,
+ colEnabled: colSubtotalDisplay.enabled as boolean | undefined,
Review Comment:
here as well for `subtotalOptions`
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -157,58 +223,76 @@ function convertToArray(
return result;
}
-export class TableRenderer extends Component {
- constructor(props) {
+export class TableRenderer extends Component<
+ TableRendererProps,
+ TableRendererState
+> {
+ sortCache: Map<string, string[][]>;
+ cachedProps: TableRendererProps | null;
+ cachedBasePivotSettings: PivotSettings | null;
+
+ static propTypes: Record<string, unknown>;
+ static defaultProps: Record<string, unknown>;
+
+ constructor(props: TableRendererProps) {
super(props);
// We need state to record which entries are collapsed and which aren't.
// This is an object with flat-keys indicating if the corresponding rows
// should be collapsed.
this.state = { collapsedRows: {}, collapsedCols: {}, sortingOrder: [] };
this.sortCache = new Map();
+ this.cachedProps = null;
+ this.cachedBasePivotSettings = null;
this.clickHeaderHandler = this.clickHeaderHandler.bind(this);
this.clickHandler = this.clickHandler.bind(this);
}
- getBasePivotSettings() {
+ getBasePivotSettings(): PivotSettings {
// One-time extraction of pivot settings that we'll use throughout the
render.
const { props } = this;
const colAttrs = props.cols;
const rowAttrs = props.rows;
- const tableOptions = {
+ const tableOptions: Record<string, unknown> = {
rowTotals: true,
colTotals: true,
...props.tableOptions,
};
- const rowTotals = tableOptions.rowTotals || colAttrs.length === 0;
- const colTotals = tableOptions.colTotals || rowAttrs.length === 0;
+ const rowTotals =
+ (tableOptions.rowTotals as boolean) || colAttrs.length === 0;
+ const colTotals =
+ (tableOptions.colTotals as boolean) || rowAttrs.length === 0;
Review Comment:
Would it make sense to define a more specific type here rather than casting
`tableOptions.colTotals` to `boolean`, to avoid unexpected values?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -599,17 +719,17 @@ export class TableRenderer extends Component {
const rowIncrSpan = rowAttrs.length !== 0 ? 1 : 0;
// Iterate through columns. Jump over duplicate values.
let i = 0;
- while (i < visibleColKeys.length) {
- let handleContextMenu;
- const colKey = visibleColKeys[i];
- const colSpan = attrIdx < colKey.length ? colAttrSpans[i][attrIdx] : 1;
+ while (i < visibleColKeys!.length) {
Review Comment:
Can `visibleColKeys`/`colAttrSpans` ever be null here?
A single guard before the loop would eliminate three non-null assertions
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -572,15 +692,15 @@ export class TableRenderer extends Component {
) : null;
const needToggle =
- colSubtotalDisplay.enabled && attrIdx !== colAttrs.length - 1;
+ !!colSubtotalDisplay.enabled && attrIdx !== colAttrs.length - 1;
Review Comment:
Isn’t this supposed to already be a boolean?
```suggestion
colSubtotalDisplay.enabled && attrIdx !== colAttrs.length - 1;
```
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1043,7 +1195,7 @@ export class TableRenderer extends Component {
</th>
);
- const totalValueCells = visibleColKeys.map(colKey => {
+ const totalValueCells = visibleColKeys!.map((colKey: string[]) => {
Review Comment:
here as well (`visibleColKeys`)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -268,8 +355,8 @@ export class TableRenderer extends Component {
rowKeys,
rowTotals,
colTotals,
- arrowCollapsed: subtotalOptions.arrowCollapsed,
- arrowExpanded: subtotalOptions.arrowExpanded,
+ arrowCollapsed: subtotalOptions.arrowCollapsed as string,
+ arrowExpanded: subtotalOptions.arrowExpanded as string,
Review Comment:
Once the appropriate type is defined, we should be able to avoid theese
casts here
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -936,13 +1085,13 @@ export class TableRenderer extends Component {
) : null;
const rowClickHandlers = cellCallbacks[flatRowKey] || {};
- const valueCells = visibleColKeys.map(colKey => {
+ const valueCells = visibleColKeys!.map((colKey: string[]) => {
const flatColKey = flatKey(colKey);
const agg = pivotData.getAggregator(rowKey, colKey);
const aggValue = agg.value();
const keys = [...rowKey, ...colKey];
- let backgroundColor;
+ let backgroundColor: string | undefined;
Review Comment:
I’m wondering if we should have a fallback for `backgroundColor` here
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -936,13 +1085,13 @@ export class TableRenderer extends Component {
) : null;
const rowClickHandlers = cellCallbacks[flatRowKey] || {};
- const valueCells = visibleColKeys.map(colKey => {
+ const valueCells = visibleColKeys!.map((colKey: string[]) => {
Review Comment:
I noticed `visibleColKeys!` is asserted several times in this method.
Would it make sense to add a single null guard at the top instead of
repeating the assertion?
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -773,15 +903,15 @@ export class TableRenderer extends Component {
<tr key="rowHdr">
{rowAttrs.map((r, i) => {
const needLabelToggle =
- rowSubtotalDisplay.enabled && i !== rowAttrs.length - 1;
+ !!rowSubtotalDisplay.enabled && i !== rowAttrs.length - 1;
Review Comment:
Should this already be typed as a boolean?
```suggestion
rowSubtotalDisplay.enabled && i !== rowAttrs.length - 1;
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
