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.

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

[DataGridPro] Server-side tree data support #12317

Open
wants to merge 45 commits into
base: master
Choose a base branch
from

Conversation

MBilalShafi
Copy link
Member

@MBilalShafi MBilalShafi commented Mar 4, 2024

Part of #8179
Resolves #3377

Previews:
Server side tree data with server-side indexed pagination, sorting, filtering, and caching: https://deploy-preview-12317--material-ui-x.netlify.app/x/react-data-grid/server-side-data/tree-data/
Plain data server-side pagination, filtering, and sorting: https://deploy-preview-12317--material-ui-x.netlify.app/x/react-data-grid/server-side-data/#usage

Action items:

  • A basic data source implementation via a dedicated hook
  • New strategy processor for server-side tree data
  • Allow use of the cache with the data source
  • Make pagination.rowCount a controlled state (extracted to [DataGrid] Make rowCount part of the state #12381)
  • Support x-data-generator to provide server-side filtered and sorted data (mock an actual data source) => (useDemoDataSource)
  • Support server-side sorting, filtering, and (index-based) pagination.
  • Make rows an optional prop (extracted to [DataGrid] Make rows an optional prop #12478)
  • Extract cache into separate plugin + support imperative operations on it
  • Support an internal cache
  • Support unknown and estimated rowCount (extracted to [DataGrid] Support advanced server-side pagination use cases #12474)
  • Support defaultGroupingExpansionDepth and isGroupExpandedByDefault
  • Demos
    • Error handling (plain data)
    • Error handling (tree data)
    • Custom cache on plain data
    • Demos with swr and react-query
  • Discoverability: Make the demos closer to the actual code to make it easy to copy-paste
  • Perf: Optimize the rowTreeCreation and other processes of rows pre-processing
  • Make loading a controlled prop ?
  • Revisit naming conventions (e.g. DataSource vs DataManager vs DataProvider vs ?)
  • Test coverage

Follow-up Items (for subsequent PRs):

@MBilalShafi MBilalShafi added component: data grid This is the name of the generic UI component, not the React module! feature: Server integration Better integration with backends MUI X Pro labels Mar 4, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 10, 2024

This comment has been minimized.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 10, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 12, 2024

This comment has been minimized.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 22, 2024

This comment has been minimized.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 29, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 5, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 16, 2024

This comment was marked as outdated.

@cherniavskii cherniavskii self-requested a review May 13, 2024 13:06
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 17, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Sharing some early feedback, I'll be reviewing it gradually 👍🏻
It looks very promising, looking forward to releasing it to the public! Great work! 🎉

Comment on lines +17 to +19
getGroupKey={getGroupKey}
hasChildren={hasChildren}
getChildrenCount={getChildrenCount}
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we can move these props into the data source itself? Would there be any downsides to this?
It feels like they belong to the data source, and won't be used without data source.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would there be any downsides to this?

Not any particular, I just had an impression that actual row data and metadata should be separate, the datasource interface should just be a representation of the actual datasource (BE/database/etc).

The meta information and other props like columns, getGroupKey, hasChildren, serverSideCache, defaultGroupingExpansionDepth, etc. should be part of grid props, so that if we had to compute them separately (using useMemo for example) we don't have to update the reference of unstable_dataSource (which may result in resetting the whole state and data refetch)

This is my personal opinion though, I'm not sure if we'll ever need to update the references of getGroupKey, hasChildren, getChildrenCount props. So maybe we can make them part of the data source interface. Let me know if you feel it to be better that way and I'll refactor it, I don't think it's a big deal.

Copy link
Member

Choose a reason for hiding this comment

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

I just had an impression that actual row data and metadata should be separate, the datasource interface should just be a representation of the actual datasource (BE/database/etc).

For hasChildren and getChildrenCount specifically, I think they really belong to the data source since you'll be querying your backend/db in those methods, similar to getRows.

When deciding whether something should be part of the dataSource interface, it might be helpful to look at it from an encapsulation point of view – if you extract a reusable data source, what do you need to define there for the data source to be usable?. hasChildren and getChildrenCount seem to fall in this bucket, similar to getGroupKey.

What do you think?


## Error handling

For each row group expansion, the data source is called to fetch the children. If an error occurs during the fetch, the grid will display an error message in the row group cell. `unstable_onServerSideError` is also triggered with the error and the fetch params.
Copy link
Member

Choose a reason for hiding this comment

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

This looks nice! 👍🏻

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 17, 2024
{...data}
unstable_dataSource={dataSource}
getGroupKey={getGroupKey}
hasChildren={hasChildren}
Copy link
Member

Choose a reason for hiding this comment

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

While writing #12317 (comment), I thought about hasChildren and getChildrenCount - do we need to have both of them? Can't we only keep getChildrenCount, and returning 0 would mean that the row has no children?

Imagine you have an endpoint that returns the number of children for the row id.
For the group rows, you'll have to call it twice – once in hasChildren to return a boolean, and once in getChildrenCount to return a number.

Are there use cases I'm missing here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. I think there's a small difference in understanding for this part of the implementation. Thanks @cherniavskii for asking the question.

Props hasChildren, getChildrenCount, getGroupKey are by-design not meant to be Promises, rather they are just like getRowId prop. The data supplied to the getRows method should contain the information which these methods are supposed to extract.

Let's assume this is a response of a getRows call:

{
  rows: [
    {
      id: 1,
      name: 'Parent 1',
      descendantsCount: 10,
    },
    {
      id: 2,
      name: 'Parent 2',
      descendantsCount: 5,
    },
    {
      id: 3,
      name: 'Leaf 1',
      descendantsCount: 0,
    },

    {
      id: 4,
      name: 'Leaf 2',
      descendantsCount: 0,
    },
  ],
  rowCount: 4,
}

For this case, the props hasChildren, getChildrenCount, getGroupKey would look like:

function hasChildren(row) {
  return row.descendantsCount > 0;
}

function getChildrenCount(row) {
  return row.descendantsCount;
}

function getGroupKey(row) {
  return row.name;
}

So considering this, it shouldn't be needed to do separate calls for getChildrenCount and hasChildren since this has to be part of row data itself.

Motivation behind this design choice

1. UX

We need to determine whether or not each row has children on server at the time of rows pre-processing for each specific level to be able to show an expand icon (and schedule expansion if that row has to be expanded as provided by user in defualtGroupingExpansionDepth or isGroupExpandedByDefault props).

If we don't have that information at the time when getRows response is received for a certain level and we have to fetch it afterwards, it may cause a weird UX, (i.e no parent rows initially and as soon as the responses are received, specific rows pop-up to be parents)

For example

Consider a tree data with 100 rows each level. On root level getRows call, i.e. corresponding to the groupKeys value [], there will be 100 rows received, at this point, the Data Grid doesn't know which one of them have children, and if so, how much.

Futher, either of the following could happen:

  1. The Data Grid initially shows all the rows as leaf rows (no expand icon) and calls props.hasChildren in the background for each row (i.e. 100 API calls), not to mention 200 more calls if getGroupKey and getChildrenCount also correspond to some API endpoint. On receiving the responses, the Data Grid keeps updating the rows to be parents and add their descendant counts as we receive the information (and schedule them to be auto fetched if they are warranted to be).
  2. The data grid shows loading indicator until the information of all the rows has been fetched, once all the rows have been fetched, pre-process the row tree, and render it to the users.

And this process will repeat for each group expansion.

To me, 1 is definitely a bad UX, even worse on a slow network. For 2, it will also be weird, for there's always possibility of a failed API call, which might make it difficult to render correct results.

2. Server-fatigue

As per my experience with servers and design of APIs, more API calls mean more parallel threads on server, more time in network I/0, and a worse user experience. In general, we should try to reduce the API calls as much as possible, if I am not wrong, that was one of the main motivations behind the introduction of GraphQL.

So, if we consider the same example as in the UX section, its 1 vs 101-301 API calls per level of grid expansion, which might not be a great API design choice.

3. Proven design

The competitors like AgGrid have been using the similar API and I can't spot any complains on this specific area within the community.

Also, from the draft around the discussion of the data source, I understood that these functions would just be extracting information and not fetching from the server.

4. API Flexibility

If there are some exceptional use-cases, the API is quite flexible to handle them.

For example, to achieve use case no 2 mentioned in the UX section, users can handle this in the getRows method themselves.
The rows APIs like setRows and updateRows are still available, so some advanced use cases should be possible if required.

If I missed some important points or use cases that would not be possible with this design, I'd love to know more about them.

Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, right.
This makes sense to me!

@@ -38,7 +38,8 @@
"@mui/x-data-grid-premium": "workspace:*",
"chance": "^1.1.11",
"clsx": "^2.1.1",
"lru-cache": "^7.18.3"
"lru-cache": "^7.18.3",
"msw": "^2.3.0"
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not so sure about using this, currently using it in 2 demos (the first demo on both overview and tree data sections). Here are a few pros and cons.

Pros:

  • Monitor the data using Network tab
  • Very close to the actual implementation

Cons:

  • Not recommended to be used in Production
  • Not meant for production so not very stable, need to avoid creating multiple instances of its server otherwise it behaves awkwardly

Suggestions are welcome.

@MBilalShafi MBilalShafi marked this pull request as ready for review May 27, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Server integration Better integration with backends plan: Pro Impact at least one Pro user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Implement server-side data source in tree data
5 participants