-
Notifications
You must be signed in to change notification settings - Fork 353
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.
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
base: main
Are you sure you want to change the base?
refactor: improve typing for useRowGrouping #1082
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the typing for the useRowGrouping
hook by introducing a new rowGroupingMapper
function with proper type overloads to ensure type safety when working with row grouping operations.
Key changes:
- Introduced
rowGroupingMapper
function with type overloads fornumber
andItem
parameters - Removed the deprecated
MapResult
interface in favor ofRowGroupingMapperResult<T>
- Simplified the
RowGroupingMapper
type definition
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
packages/core/src/data-editor/row-grouping.ts | Removed MapResult interface and updated mapRowIndexToPath return type |
packages/core/src/data-editor/row-grouping-api.ts | Added rowGroupingMapper function with type overloads and refactored the mapper implementation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
||
// 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) |
There was a problem hiding this comment.
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').
if (flattenedRowGroups === undefined || flattenRowGroups.length === 0) | |
if (flattenedRowGroups === undefined || flattenedRowGroups.length === 0) |
Copilot uses AI. Check for mistakes.
(itemOrRow: number): RowGroupingMapperResult<number>; | ||
(itemOrRow: Item): RowGroupingMapperResult<Item>; | ||
}; | ||
export type RowGroupingMapper = (itemOrRow: number | Item) => RowGroupingMapperResult<Item | number>; |
There was a problem hiding this comment.
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.
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> { |
There was a problem hiding this comment.
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.
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.
Introduced rowGroupingMapper with type overload to make sure when itemOrRow is type of
number
it should returnRowGroupingMapperResult<number>
and when type ifItem
it should returnRowGroupingMapperResult<Item>