Skip to content
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

Jump to bottom

[DataGrid] Fix row id bug #10051

Merged
merged 4 commits into from
Aug 16, 2023
Merged

[DataGrid] Fix row id bug #10051

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4677705
fix: row id bug
romgrk Aug 16, 2023
ebb9dbb
lint
romgrk Aug 16, 2023
c7d01b5
lint
romgrk Aug 16, 2023
530964b
ci: run
romgrk Aug 16, 2023
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
1 change: 1 addition & 0 deletions docs/pages/x/api/data-grid/grid-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import { GridApi } from '@mui/x-data-grid';
| <span class="prop-name">getRow</span> | <span class="prop-type">&lt;R extends GridValidRowModel = any&gt;(id: GridRowId) =&gt; R \| null</span> | Gets the row data with a given id. |
| <span class="prop-name">getRowElement</span> | <span class="prop-type">(id: GridRowId) =&gt; HTMLDivElement \| null</span> | Gets the underlying DOM element for a row at the given `id`. |
| <span class="prop-name">getRowGroupChildren [<span class="plan-pro" title="Pro plan"></span>](/x/introduction/licensing/#pro-plan)</span> | <span class="prop-type">(params: GridRowGroupChildrenGetterParams) =&gt; GridRowId[]</span> | Gets the rows of a grouping criteria.<br />Only contains the rows provided to the grid, not the rows automatically generated by it. |
| <span class="prop-name">getRowId</span> | <span class="prop-type">&lt;R extends GridValidRowModel = any&gt;(row: R) =&gt; GridRowId</span> | Gets the ID of a row given its data. |
| <span class="prop-name">getRowIdFromRowIndex</span> | <span class="prop-type">(index: number) =&gt; GridRowId</span> | Gets the `GridRowId` of a row at a specific index.<br />The index is based on the sorted but unfiltered row list. |
| <span class="prop-name">getRowIndexRelativeToVisibleRows</span> | <span class="prop-type">(id: GridRowId) =&gt; number</span> | Gets the index of a row relative to the rows that are reachable by scroll. |
| <span class="prop-name">getRowMode</span> | <span class="prop-type">(id: GridRowId) =&gt; GridRowMode</span> | Gets the mode of a row. |
Expand Down
8 changes: 3 additions & 5 deletions ...ges/grid/x-data-grid-premium/src/hooks/features/aggregation/wrapColumnWithAggregation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ const getAggregationValueWrappedRenderCell: ColumnPropertyWrapper<'renderCell'>
*/
const getWrappedFilterOperators: ColumnPropertyWrapper<'filterOperators'> = ({
value: filterOperators,
apiRef,
getCellAggregationResult,
}) =>
filterOperators!.map((operator) => {
Expand Down Expand Up @@ -154,7 +155,7 @@ const getWrappedFilterOperators: ColumnPropertyWrapper<'filterOperators'> = ({
return null;
}
return (value, row, column, api) => {
if (getCellAggregationResult(row.id, column.field) != null) {
if (getCellAggregationResult(apiRef.current.getRowId(row), column.field) != null) {
return true;
}
return filterFn(value, row, column, api);
Expand Down Expand Up @@ -203,10 +204,7 @@ export const wrapColumnWithAggregationValue = ({
field: string,
): GridAggregationLookup[GridRowId][string] | null => {
let cellAggregationPosition: GridAggregationPosition | null = null;
const rowNode = apiRef.current.getRowNode(id);
if (!rowNode) {
return null;
}
const rowNode = apiRef.current.getRowNode(id)!;

if (rowNode.type === 'group') {
cellAggregationPosition = 'inline';
Expand Down
14 changes: 14 additions & 0 deletions packages/grid/x-data-grid/src/hooks/features/rows/useGridRows.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,19 @@ export const useGridRows = (
[apiRef],
);

const getRowId = React.useCallback<GridRowApi['getRowId']>(
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering about the performance of this.

And if like AG Grid, it can make sense to have an intermediary data structure for caching https://www.ag-grid.com/javascript-data-grid/client-side-row-stages/#state-2-all-rows

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which kind of performance hit are you worried about?

And I'm not sure how that structure applies here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The apiRef.current indirection might prevent optimization, but I think if we destructure the API method into a const before usage like const getRowId = apiRef.current.getRowId it becomes very very easy for the JITs to optimize it away with inlining, and the if (getRowIdProp) branch is also optimized away in most cases due to getRowIdProp being a const undefined.

But we can keep that for cases where it matters. In the case for this PR it doesn't matter enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh actually it's for filtering, so it might matter. I'll benchmark at some point and decide from there.

Copy link
Member

@oliviertassinari oliviertassinari Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Which kind of performance hit are you worried about?

I was wondering of the time it take to access row.id vs. calling getRowId(row) when we do it a lot of time, e.g. sorting 100,000 rows client side, but also filtering, grouping, pivoting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice to avoid, but let's start with correctness and we can benchmark performance again soon, I'm completing one more perf change and I'll be done with the work from #9120. Anyway I think we've gained enough performance lately to afford one more fcall in there :)

(row) => {
if (GRID_ID_AUTOGENERATED in row) {
return row[GRID_ID_AUTOGENERATED];
}
if (props.getRowId) {
return props.getRowId(row);
}
return row.id;
},
[apiRef, props.getRowId],
);

const lookup = React.useMemo(
() =>
currentPage.rows.reduce<Record<GridRowId, number>>((acc, { id }, index) => {
Expand Down Expand Up @@ -451,6 +464,7 @@ export const useGridRows = (

const rowApi: GridRowApi = {
getRow,
getRowId,
getRowModels,
getRowsCount,
getAllRowIds,
Expand Down
6 changes: 6 additions & 0 deletions packages/grid/x-data-grid/src/models/api/gridRowApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ export interface GridRowApi {
* @returns {GridRowModel} The row data.
*/
getRow: <R extends GridValidRowModel = any>(id: GridRowId) => R | null;
/**
* Gets the ID of a row given its data.
* @param {GridRowModel} row The row data.
* @returns {GridRowId} The id of the row.
*/
getRowId: <R extends GridValidRowModel = any>(row: R) => GridRowId;
/**
* Gets the row node from the internal tree structure.
* TODO rows v6: Rename `getTreeNode`
Expand Down
Loading