Skip to content

refactor: improve typing for useRowGrouping #1082

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

refactor: improve typing for useRowGrouping #1082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
84902b7
refactor: improve typing for useRowGrouping
ImmortalRabbit Aug 9, 2025
c8842d6
improve typing with type guard
ImmortalRabbit Aug 9, 2025
6a517ff
Small renaming
ImmortalRabbit Aug 9, 2025
f751e71
fix lint error
ImmortalRabbit Aug 9, 2025
347bcd6
use union type to avoid casing any
ImmortalRabbit Aug 9, 2025
32af20f
final version
ImmortalRabbit Aug 9, 2025
5b0f420
fix lint
ImmortalRabbit Aug 9, 2025
6c30726
Merge branch 'main' into remove-usage-of-any-onuseRowGrouping
ImmortalRabbit Aug 14, 2025
818913b
Merge branch 'main' into remove-usage-of-any-onuseRowGrouping
ImmortalRabbit Aug 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 21 additions & 17 deletions packages/core/src/data-editor/row-grouping-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,31 @@
import React from "react";
import type { Item } from "../internal/data-grid/data-grid-types.js";
import { flattenRowGroups, mapRowIndexToPath, type RowGroup, type RowGroupingOptions, type MapResult } from "./row-grouping.js";
import { flattenRowGroups, mapRowIndexToPath, type FlattenedRowGroup, type RowGroup, type RowGroupingOptions } from "./row-grouping.js";

export interface RowGroupingMapperResult<T> extends Omit<MapResult, 'originalIndex'> {
export interface RowGroupingMapperResult<T> {
path: readonly number[];
originalIndex: T;
isGroupHeader: boolean;
groupRows: number;
originalIndex: T;
readonly groupIndex: number;
readonly contentIndex: number;
}

export type RowGroupingMapper = {
(itemOrRow: number): RowGroupingMapperResult<number>;
(itemOrRow: Item): RowGroupingMapperResult<Item>;
};
export type RowGroupingMapper = (itemOrRow: number | Item) => RowGroupingMapperResult<Item | number>;
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RowGroupingMapper type loses type safety by returning a union type RowGroupingMapperResult<Item | number>. This contradicts the purpose of the type overloads in rowGroupingMapper. Consider using function overloads for this type as well to maintain type safety.

Suggested change
export type RowGroupingMapper = (itemOrRow: number | Item) => RowGroupingMapperResult<Item | number>;
export type RowGroupingMapper = {
(itemOrRow: number, flattenedRowGroups?: readonly FlattenedRowGroup[] | undefined): RowGroupingMapperResult<number>;
(itemOrRow: Item, flattenedRowGroups?: readonly FlattenedRowGroup[] | undefined): RowGroupingMapperResult<Item>;
};

Copilot uses AI. Check for mistakes.

export function rowGroupingMapper(itemOrRow: number, flattenedRowGroups?: readonly FlattenedRowGroup[] | undefined): RowGroupingMapperResult<number>;
export function rowGroupingMapper(itemOrRow: Item, flattenedRowGroups?: readonly FlattenedRowGroup[] | undefined): RowGroupingMapperResult<Item>;
export function rowGroupingMapper(itemOrRow: number | Item, flattenedRowGroups?: readonly FlattenedRowGroup[] | undefined): RowGroupingMapperResult<number> | RowGroupingMapperResult<Item>;
export function rowGroupingMapper(itemOrRow: number | Item, flattenedRowGroups?: readonly FlattenedRowGroup[] | undefined): RowGroupingMapperResult<number> | RowGroupingMapperResult<Item> {
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation signature includes redundant | undefined since the parameter is already marked as optional with ?. Remove | undefined from the parameter type.

Suggested change
export function rowGroupingMapper(itemOrRow: number | Item, flattenedRowGroups?: readonly FlattenedRowGroup[] | undefined): RowGroupingMapperResult<number> | RowGroupingMapperResult<Item> {
export function rowGroupingMapper(itemOrRow: number, flattenedRowGroups?: readonly FlattenedRowGroup[]): RowGroupingMapperResult<number>;
export function rowGroupingMapper(itemOrRow: Item, flattenedRowGroups?: readonly FlattenedRowGroup[]): RowGroupingMapperResult<Item>;
export function rowGroupingMapper(itemOrRow: number | Item, flattenedRowGroups?: readonly FlattenedRowGroup[]): RowGroupingMapperResult<number> | RowGroupingMapperResult<Item>;
export function rowGroupingMapper(itemOrRow: number | Item, flattenedRowGroups?: readonly FlattenedRowGroup[]): RowGroupingMapperResult<number> | RowGroupingMapperResult<Item> {

Copilot uses AI. Check for mistakes.
if (typeof itemOrRow === "number") {
return mapRowIndexToPath(itemOrRow, flattenedRowGroups);
}
const r = mapRowIndexToPath(itemOrRow[1], flattenedRowGroups);
return {
...r,
originalIndex: [itemOrRow[0], r.originalIndex],
} as RowGroupingMapperResult<Item>;
}

export interface UseRowGroupingResult {
readonly mapper: RowGroupingMapper;
Expand All @@ -34,16 +47,7 @@ export function useRowGrouping(options: RowGroupingOptions | undefined, rows: nu
getRowGroupingForPath,
updateRowGroupingByPath,
mapper: React.useCallback<UseRowGroupingResult["mapper"]>(
(itemOrRow: number | Item) => {
if (typeof itemOrRow === "number") {
return mapRowIndexToPath(itemOrRow, flattenedRowGroups);
}
const r = mapRowIndexToPath(itemOrRow[1], flattenedRowGroups);
return {
...r,
originalIndex: [itemOrRow[0], r.originalIndex],
} as any; // FIXME
},
(itemOrRow: number | Item) => rowGroupingMapper(itemOrRow, flattenedRowGroups),
[flattenedRowGroups]
),
};
Expand Down
11 changes: 2 additions & 9 deletions packages/core/src/data-editor/row-grouping.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { Theme } from "../common/styles.js";
import type { DataEditorProps } from "./data-editor.js";
import type { DataGridProps } from "../internal/data-grid/data-grid.js";
import { whenDefined } from "../common/utils.js";
import type { RowGroupingMapperResult } from "./row-grouping-api.js";

type Mutable<T> = {
-readonly [K in keyof T]: T[K];
Expand Down Expand Up @@ -177,17 +178,9 @@ export function flattenRowGroups(rowGrouping: RowGroupingOptions, rows: number):
});
}

export interface MapResult {
readonly path: readonly number[];
readonly isGroupHeader: boolean;
readonly originalIndex: number;
readonly groupIndex: number;
readonly groupRows: number;
readonly contentIndex: number;
}

// grid relative index to path and other details
export function mapRowIndexToPath(row: number, flattenedRowGroups?: readonly FlattenedRowGroup[]): MapResult {
export function mapRowIndexToPath(row: number, flattenedRowGroups?: readonly FlattenedRowGroup[]): RowGroupingMapperResult<number> {
if (flattenedRowGroups === undefined || flattenRowGroups.length === 0)
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a typo in the variable name. It should be flattenedRowGroups.length instead of flattenRowGroups.length. The function parameter is flattenedRowGroups but the condition checks flattenRowGroups (missing 'd').

Suggested change
if (flattenedRowGroups === undefined || flattenRowGroups.length === 0)
if (flattenedRowGroups === undefined || flattenedRowGroups.length === 0)

Copilot uses AI. Check for mistakes.
return {
path: [row],
Expand Down