[DataGridPro] Ignore missing rowCount response when new children are fetched with the data source
#17711
Conversation
arminmeh
added
type: bug
|
Deploy preview: https://deploy-preview-17711--material-ui-x.netlify.app/ Bundle size reportTotal Size Change: ▼-16B(0.00%) - Total Gzip Change: ▼-29B(0.00%) Show details for 100 more bundles (22 more not shown)@mui/x-data-grid-premium parsed: ▼-4B(0.00%) gzip: ▼-6B(0.00%) |
|
Would this change impact the infinite loading feature? https://mui.com/x/react-data-grid/server-side-data/lazy-loading/#infinite-loading |
There was a problem hiding this comment.
IMO, there are a couple of reasons why updating the rowCount during child data fetches would make sense:
- Consistency in function signatures: Our data fetching function (
getRows) is utilized for both root and nested data. Its return type includesrowCount, so from a TypeScript perspective, it should accept and processrowCountregardless of the data’s nesting level. Changing this without any clear indication may cause slightly unpleasant DX. - Dynamic data use-case: In scenarios involving dynamic data, the root
rowCountmight change in the background. Allowing updates to the rootrowCountalongside 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
IMO, there are a couple of reasons why updating the
rowCountduring child data fetches would make sense:
- Consistency in function signatures: Our data fetching function (
getRows) is utilized for both root and nested data. Its return type includesrowCount, so from a TypeScript perspective, it should accept and processrowCountregardless of the data’s nesting level. Changing this without any clear indication may cause slightly unpleasant DX.- Dynamic data use-case: In scenarios involving dynamic data, the root
rowCountmight change in the background. Allowing updates to the rootrowCountalongside 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
rowCountupdates are ignored if passed asundefined(except during the initialgetRowscall)? 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?
There was a problem hiding this comment.
How about giving users an option to control this behavior—perhaps something like an
ignoreRowCountOnChildFetchparameter? 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
There was a problem hiding this comment.
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:
- Update the
rootRowCountat any time if needed, or skip if not. - 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
- Optional types make the behavior controllable by prop/API, ignoring
undefinedvalues.
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?
There was a problem hiding this comment.
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.
|
This pull request has been inactive for 30 days. Please remove the stale label or leave a comment to keep it open. Otherwise, it will be closed in 15 days. |
github-actions
bot
added
the
stale
arminmeh
removed
the
stale
arminmeh
force-pushed
the
ignore-row-count-while-loading-children
branch
from
87790dd to
e8c54e9
Compare
arminmeh
changed the title
rowCount response when new children are fetched with the data sourcerowCount response when new children are fetched with the data source
5 participants
Closes #17626
v7 ignored
undefinedvalues for therowCountwhile fetching childrenhttps://github.com/mui/mui-x/blob/v7.x/packages/x-data-grid-pro/src/hooks/features/dataSource/useGridDataSource.ts#L185
v8 sets it to -1
I think that we can remove this altogether as it does not make sense to update root row count while updating children.
If you do, you don't have a mean to update the data via the same response, so your count cannot be correct.