Skip to content

[DataGridPro] Ignore missing rowCount response when new children are fetched with the data source #17711

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

Merged
merged 2 commits into from
Jun 19, 2025
Merged

[DataGridPro] Ignore missing rowCount response when new children are fetched with the data source #17711

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8c54e9
Do not update row count when loading new children with the data source
arminmeh May 6, 2025
d81d8f9
Ignore undefined row count while loading children
arminmeh Jun 16, 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
10 changes: 6 additions & 4 deletions packages/x-data-grid-pro/src/hooks/features/dataSource/useGridDataSourceBasePro.ts
View file
Open in desktop
Copy link
Member

@MBilalShafi MBilalShafi May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, there are a couple of reasons why updating the rowCount during child data fetches would make sense:

  1. Consistency in function signatures: Our data fetching function (getRows) is utilized for both root and nested data. Its return type includes rowCount, so from a TypeScript perspective, it should accept and process rowCount regardless of the data’s nesting level. Changing this without any clear indication may cause slightly unpleasant DX.
  2. Dynamic data use-case: In scenarios involving dynamic data, the root rowCount might change in the background. Allowing updates to the root rowCount alongside nested data fetches can eliminate the need for an additional API call solely to retrieve the updated row count. It may particularly help specific cases where API calls are expensive.

How about reverting to the v7 behavior—where rowCount updates are ignored if passed as undefined (except during the initial getRows call)? Do you foresee some potential issues with this approach?

Copy link

@mannam95 mannam95 May 14, 2025

Choose a reason for hiding this comment

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

How about giving users an option to control this behavior—perhaps something like an ignoreRowCountOnChildFetch parameter? It doesn't have to be that exact name, but something along those lines to provide flexibility.

Copy link
Contributor Author

@arminmeh arminmeh May 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, there are a couple of reasons why updating the rowCount during child data fetches would make sense:

  1. Consistency in function signatures: Our data fetching function (getRows) is utilized for both root and nested data. Its return type includes rowCount, so from a TypeScript perspective, it should accept and process rowCount regardless of the data’s nesting level. Changing this without any clear indication may cause slightly unpleasant DX.
  2. Dynamic data use-case: In scenarios involving dynamic data, the root rowCount might change in the background. Allowing updates to the root rowCount alongside nested data fetches can eliminate the need for an additional API call solely to retrieve the updated row count. It may particularly help specific cases where API calls are expensive.

How about reverting to the v7 behavior—where rowCount updates are ignored if passed as undefined (except during the initial getRows call)? Do you foresee some potential issues with this approach?

Agree with the point about the consistency.

Taking all of this into the consideration, I think that the right behavior then would be to consider the whole response to carry the information related to the request context.
For root calls both are related to the top level. For children request, both are related to that nested level.
We do this for rows, but we consider rowCount to always be related to the top level.

If we would adopt the new behavior, we would need to use rowCount to update parent row descendants count and this looks like something you might need to do.

To prevent breaking change, new behavior can be behind an experimental flag and for now we do

How about reverting to the v7 behavior—where rowCount updates are ignored if passed as undefined (except during the initial getRows call)? Do you foresee some potential issues with this approach?

Copy link
Contributor Author

@arminmeh arminmeh May 14, 2025

Choose a reason for hiding this comment

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

How about giving users an option to control this behavior—perhaps something like an ignoreRowCountOnChildFetch parameter? It doesn't have to be that exact name, but something along those lines to provide flexibility.

This can also easily be done by caching the last row count on your own and returning it if the new response does not have this value

Copy link
Member

@MBilalShafi MBilalShafi May 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

For root calls both are related to the top level. For children request, both are related to that nested level.
We do this for rows, but we consider rowCount to always be related to the top level.

If we would adopt the new behavior, we would need to use rowCount to update parent row descendants count and this looks like something you might need to do.

Exactly, we'll have to do it with #14527 anyways. Though I had in my mind a slightly different interface, to allow passing both the values at any nested level.

Something like:

type GetRowsResponse = {
  rows: GridValidRowModel[];
  /**
    Row count for the current nested level. For the root level, it can be omitted, or `rowCount` can be repeated.
  */
  rowCount?: number;
  /**
    Row count for the root level. It can be omitted for a particular nested level, in which case the previously set value will be used. If it's not defined in the state, it will be initialized as `-1` to represent unknown.
  */
  rootRowCount?: number;
}

It will provide a predictable structure with the flexibility to:

  1. Update the rootRowCount at any time if needed, or skip if not.
  2. Update the nested level row count if needed, to be used with cases like [data grid] Implement server-side data source with nested data lazy loading #14527
  3. Optional types make the behavior controllable by prop/API, ignoring undefined values.

To me, the current behavior of having the previous rowCount replaced by -1 if no row count is provided with any API call is unexpected. I'd advocate for only keeping it on the very first data source call.

To prevent breaking change, new behavior can be behind an experimental flag

We could possibly avoid the breaking change by adjusting the naming, so we keep the rowCount property and add a new one for the current nested level, such as currentRowCount.
The in v9, we do a breaking change rowCount => rootRowCount and currentRowCount => rowCount, wdyt?

Copy link
Contributor Author

@arminmeh arminmeh Jun 16, 2025

Choose a reason for hiding this comment

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

I have updated the behavior to be the same as in v7 to avoid breaking change.

We could possibly avoid the breaking change by adjusting the naming, so we keep the rowCount property and add a new one for the current nested level, such as currentRowCount.
The in v9, we do a breaking change rowCount => rootRowCount and currentRowCount => rowCount, wdyt?

I agree that we will need a new property when #14527 is implemented, but I will leave that to be added in the related PR.

From v9 I think that we should do the replacement currentRowCount => rowCount (once it is added) and not have root row count processed with the children data fetch (not doing rowCount => rootRowCount).

For me it is weird to get back information that is not related to my request. I have asked for the children of a specific parent. I don't expect to get back the root row count. Can you find an example of an API that does something like this?

In addition to this, providing a root row count without root rows is just half of the data I need to actually refresh my tree.

Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,9 @@ export const useGridDataSourceBasePro = <Api extends GridPrivateApiPro>(
const rows = cachedData.rows;
nestedDataManager.setRequestSettled(id);
apiRef.current.updateNestedRows(rows, rowNode.path);
apiRef.current.setRowCount(cachedData.rowCount === undefined ? -1 : cachedData.rowCount);
if (cachedData.rowCount !== undefined) {
apiRef.current.setRowCount(cachedData.rowCount);
}
apiRef.current.setRowChildrenExpansion(id, true);
apiRef.current.dataSource.setChildrenLoading(id, false);
return;
Expand Down Expand Up @@ -158,9 +160,9 @@ export const useGridDataSourceBasePro = <Api extends GridPrivateApiPro>(
cache.set(key, response);
});

apiRef.current.setRowCount(
getRowsResponse.rowCount === undefined ? -1 : getRowsResponse.rowCount,
);
if (getRowsResponse.rowCount !== undefined) {
apiRef.current.setRowCount(getRowsResponse.rowCount);
}
// Remove existing outdated rows before setting the new ones
const rowsToDelete: GridRowModelUpdate[] = [];
getRowsResponse.rows.forEach((row) => {
Expand Down