-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[TreeView] Add lazy loading for children of tree items #15308
base: master
Are you sure you want to change the base?
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.
Cool exploration 👍
waiAria: https://www.w3.org/WAI/ARIA/apg/patterns/treeview/ | ||
--- | ||
|
||
# Rich Tree View - Lazy Loading Children |
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.
# Rich Tree View - Lazy Loading Children | |
# Rich Tree View - Lazy loading children [<span class="plan-pro"></span>](/x/introduction/licensing/#pro-plan 'Pro plan') |
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.
Yep I agree, it should be part of the pro plan
I saw the grid discussing moving some lazy loading to the community package, we should just check with them before moving ours to the pro that we aren't doing a mistake, but AFAIK the feature should be rpo
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 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.
So why have data grid data source MIT but tree view data source Pro?
In the case of the data grid data: data source allows to maximize the open source adoption (get to a fully working prototype sooner, the fastest we get there, the least we give people time to try something else, I don't think people say I have 10h, I will try as many grids as possible, but instead, give a 2hrs budget to the first grid, if they feel too slow, kill it, try the next one, and so one) and even if you have the feature, you can't do much with it, you still need the Pro plan for their features. So MIT.
In the case of the Tree View: I would expect developers to be able to implement lazy loading using React Query (TanStack/query#413) with our MIT plan and not to have a too terrible experience. I assumed that our new docs page is about doing more than this, but looking closer, it's not so clear. So maybe the right move is:
- Make the page MIT, since we likely want to have an example using TanStack Query https://npm-stat.com/charts.html?package=react-dom&package=%40tanstack%2Freact-query&from=2023-11-12&to=2024-11-12?
- Add that example
- Rely on the fact that once people integrate lazy loading, they are more likely to hit a point where they need virtualization (Pro) or need drag & drop (Pro), and we need Lazy loading to be competitive in the MIT space, e.g. https://ant.design/components/tree#tree-demo-dynamic. This is a better 📈 ramp.
- We see if there is space for a more complex data fetching logic. Maybe we don't need a data source, if we do, it goes Pro.
Guesses our where a data source could truly shine for cases where:
- the simple lazy loading API isn't enough
- you are using a data grid data source, you are used to it, and you want the same abstraction
- you are going to use virtualization, and you need to be smart about loading
- SWR or React Query have too many limitations (we need to be careful to not rebuild those libraries but to be truly complementary). A quick research on what SWR does that fetch doesn't: https://www.reddit.com/r/nextjs/comments/17603jy/comment/k4j3stp/.
docs/data/tree-view/rich-tree-view/lazy-loading/lazy-loading.md
Outdated
Show resolved
Hide resolved
docs/data/tree-view/rich-tree-view/lazy-loading/lazy-loading.md
Outdated
Show resolved
Hide resolved
import { RichTreeView } from '@mui/x-tree-view/RichTreeView'; | ||
import { TreeViewBaseItem } from '@mui/x-tree-view/models'; | ||
|
||
const fetchData = async (): Promise< |
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.
const fetchData = async (): Promise< | |
async function fetchData(): Promise< |
|
||
export default function BasicLazyLoading() { | ||
return ( | ||
<Box sx={{ width: '300px' }}> |
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.
<Box sx={{ width: '300px' }}> | |
<Box sx={{ minHeight: 320, minWidth: 300 }}> |
treeViewDataSource={{ | ||
getChildrenCount: (item) => item?.childrenCount, | ||
getTreeItems: fetchData, | ||
}} |
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.
Not super clear to me if:
- This should be part of MUI X and not Toolpad. But maybe [data grid] Provide ready to use data sources #14731 is another layer of abstraction, on top of this one
- How much code should be shared with the data grid
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.
For now we are following a DX that is close to the one of the grid.
Once we have an initial version fully working, we will see with the grid and the charts if there are things to share with the grid. Sharing DX is obvious (to me at least), sharing code might be more complex since it's very tight to the internals of each set of components.
@@ -9,6 +9,12 @@ export interface TreeViewItemToRenderProps { | |||
children?: TreeViewItemToRenderProps[]; | |||
} | |||
|
|||
export type AddItemsParams<R> = { |
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.
export type AddItemsParams<R> = { | |
export type AddItemsParameters<R> = { |
@@ -172,6 +172,7 @@ RichTreeView.propTypes = { | |||
experimentalFeatures: PropTypes.shape({ | |||
indentationAtItemLevel: PropTypes.bool, | |||
labelEditing: PropTypes.bool, | |||
lazyLoading: PropTypes.bool, |
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.
I don't understand this one. If lazing loading can be detected with the prop. What's the advantage of this? We could be doing:
lazyLoading: PropTypes.bool, |
and then: unstable_treeViewDataSource
, unstable_treeViewDataSourceCache
. I thought the point of experimentalFeatures
is for things that we can't change with a prop.
I have the same confusion with Next.js: https://nextjs.org/docs/app/building-your-application/upgrading/version-15#serverexternalpackages, I don't understand why they handle this this way.
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.
We went for consistency here, each new major feature is hidden under an experimentalFeature
.
For me this has two main benefits:
- If you have several props, you don't need to prefix each one of them (plus the API methods, plus the utility hooks etc...)
- Users don't have a breaking change when the feature becomes stable. They can remove there
experimentalFeature
but there app will work fine even if they keep it. With theunstable_
prefix, there is a breaking change that we have to care about (by exporting two version with and without the prefix for some time) and that is still a cost for our users. - Some feature have to be flagged as
experimentalFeatures
because as you said we can't do it with props (because they are opt-out, like the aggregation). So I prefer to be consistent and not have the user learn a new way of enabling an experimental feature for each new feature.
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.
@flaviendelangle The limits I see:
- It's not clear what's stable and what's unstable. This would be clearer
<TreeView
experimentalFeatures={{
treeViewDataSource:
treeViewDataSourceCache:
}}
- The terminology is confusing, we call the rest unstable. "experimental" sounds likely that it could get dropped, "unstable" feels like a higher degree of commitment from MUI, more likely to land as stable.
<TreeView
unstableFeatures={{
treeViewDataSource:
treeViewDataSourceCache:
}}
Users don't have a breaking change when the feature becomes stable
The API could change when it becomes stable though. We also don't have to make it a breaking change, we can have deprecations.
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 terminology is confusing, we call the rest unstable. "experimental" sounds likely that it could get dropped, "unstable" feels like a higher degree of commitment.
On the other hand, our assumption was that "unstable" was more repelling than "experimental" which is why we called this prop "experimentalFeatures" instead of "unstableFeatures" on the grid.
Both have benefits for sure.
The API could change when it becomes stable though.
But most of the time it doesn't on the experimental features we had in the past.
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.
It's not clear what's stable and what's unstable. This would be clearer
experimentalFeatures={{
treeViewDataSource:
treeViewDataSourceCache:
}}
I agree, we should add both props that are introduced to the experimentalFeatures list (or unstableFeatures if we decide to rename) 👌.
We weren't sure about the exact dx we wanted here and how similar it should be to the one the grid has when I flagged this as experimental (did it right at the beginning of my exploration). So the inconsistency between what's in experimental features and the names of the props is due to the fact that this is still WIP. 🙈
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.
If we think experimentalFeatures={{ treeViewDataSource: true }}
is not clear enough to tell that the cache is experimental as well, I'm fine splitting the two.
I felt like both were close enough to keep them together (which is what the grid did I think) but that's debatable.
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.
React uses both, unstable and experimental https://github.com/facebook/react/blob/main/packages/react/index.js#L63. Confusing perceptions in https://www.linuxquestions.org/questions/debian-26/unstable-vs-experimental-331158/.
No a strong view, but unless there are major cons with this, going like this seems good enough:
- stick to a single term in code, and in docs, it feels unlikely that we can reliability communicate: unstable > experimental or unstable < experimental.
- we could introduce a pre GA prefix, effective what we use unstable_ for today so that we can then use unstable for really unstable things like AI integration (we might kill it). But do we really need this level of precision/complexity? Until we have too many examples where critical feedback isn't left on the features during the non stable phase and is only left after stable, no, let's be lazy.
- with a single term, use unstable because it's shorter, a bit closer to what React does?
- prefer to flatten unstable props rather than grouping them, our API docs generation struggles in documenting nested props. Not sure about the value of grouping in the first place, I don't see why Next.js group them.
- have deprecations when promoting a unstable API to stable.
- make all components follow the same policy, Base UI, etc.
waiAria: https://www.w3.org/WAI/ARIA/apg/patterns/treeview/ | ||
--- | ||
|
||
# Rich Tree View - Lazy Loading Children |
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.
So why have data grid data source MIT but tree view data source Pro?
In the case of the data grid data: data source allows to maximize the open source adoption (get to a fully working prototype sooner, the fastest we get there, the least we give people time to try something else, I don't think people say I have 10h, I will try as many grids as possible, but instead, give a 2hrs budget to the first grid, if they feel too slow, kill it, try the next one, and so one) and even if you have the feature, you can't do much with it, you still need the Pro plan for their features. So MIT.
In the case of the Tree View: I would expect developers to be able to implement lazy loading using React Query (TanStack/query#413) with our MIT plan and not to have a too terrible experience. I assumed that our new docs page is about doing more than this, but looking closer, it's not so clear. So maybe the right move is:
- Make the page MIT, since we likely want to have an example using TanStack Query https://npm-stat.com/charts.html?package=react-dom&package=%40tanstack%2Freact-query&from=2023-11-12&to=2024-11-12?
- Add that example
- Rely on the fact that once people integrate lazy loading, they are more likely to hit a point where they need virtualization (Pro) or need drag & drop (Pro), and we need Lazy loading to be competitive in the MIT space, e.g. https://ant.design/components/tree#tree-demo-dynamic. This is a better 📈 ramp.
- We see if there is space for a more complex data fetching logic. Maybe we don't need a data source, if we do, it goes Pro.
Guesses our where a data source could truly shine for cases where:
- the simple lazy loading API isn't enough
- you are using a data grid data source, you are used to it, and you want the same abstraction
- you are going to use virtualization, and you need to be smart about loading
- SWR or React Query have too many limitations (we need to be careful to not rebuild those libraries but to be truly complementary). A quick research on what SWR does that fetch doesn't: https://www.reddit.com/r/nextjs/comments/17603jy/comment/k4j3stp/.
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
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.
Sorry for the huge delay 😢
|
||
## Editing lazy loaded children | ||
|
||
In order to register changes for item labels on your server or update them in the cache use the `onItemLabelChange` callback function. Visit the dedicated page for [lazy loading](/x/react-tree-view/rich-tree-view/lazy-loading/#lazy-loading-and-label-editing) to read more. |
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.
In order to register changes for item labels on your server or update them in the cache use the `onItemLabelChange` callback function. Visit the dedicated page for [lazy loading](/x/react-tree-view/rich-tree-view/lazy-loading/#lazy-loading-and-label-editing) to read more. | |
In order to register changes for item labels on your server or update them in the cache use the `onItemLabelChange()` callback function. | |
See [Rich Tree View Pro—Lazy Loading Children]((/x/react-tree-view/rich-tree-view/lazy-loading/#lazy-loading-and-label-editing) for more details. |
And I would just add a small code snippet between the two sentences of the prop usage.
Something super basic to make this section sufficient for basic use cases
childrenCount?: number; | ||
}>; | ||
|
||
function valuetext(value: 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.
function valuetext(value: number) { | |
function getSliderAriaValueText(value: number) { |
loading: { | ||
...state.dataSource.loading, | ||
...loadingIds, | ||
this.api.setState((state) => { |
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.
Not related to this PR
itemMetaLookup[id] = { | ||
id, | ||
label, | ||
parentId, | ||
idAttribute: undefined, | ||
expandable: !!item.children?.length, | ||
expandable: !!item.children?.length || isItemExpandable, |
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.
Would it make sense to do the following?
expandable: !!item.children?.length || isItemExpandable, | |
expandable: getChildrenCount ? (getChildrenCount(item) > 0) : !!item.children?.length, |
[store], | ||
); | ||
|
||
const setTreeViewLoading = (isLoading: boolean) => { |
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.
useEventCallback
on all the new API methods 🙏
getItemId: params.getItemId, | ||
getItemLabel: params.getItemLabel, | ||
}), | ||
loading: false, |
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.
Can we move loading
and error
inside updateItemsState
so that it handles the full state?
getChildrenCount, | ||
}: AddItemsParameters<TreeViewBaseItem>) => { | ||
if (items) { | ||
const newState = updateItemsState({ |
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.
I feel like we should slit updateItemsState
into 2:
- A new method that handles the shared logic that you use in
updateItemsState
and inaddItems
updateItemsState
that is only used for the already existing use cases.
Because the method stats to be quite complex 😬
"include": ["src/**/*"] | ||
"include": [ | ||
"src/**/*", | ||
"../x-tree-view-pro/src/internals/plugins/useTreeViewLazyLoading/useTreeViewLazyLoading.types.ts" |
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.
😬 This dependency is really not great
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Fix #9687
Preview: https://deploy-preview-15308--material-ui-x.netlify.app/x/react-tree-view/rich-tree-view/lazy-loading/
TO DO: