-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts] Use store from @mui/x-internals
#20121
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
Conversation
|
Deploy preview: https://deploy-preview-20121--material-ui-x.netlify.app/ Bundle size report
|
CodSpeed Performance ReportMerging #20121 will improve performances by 18.11%Comparing Summary
Benchmarks breakdown
|
packages/x-charts-pro/src/FunnelChart/funnelAxisPlugin/useChartFunnelAxis.ts
Outdated
Show resolved
Hide resolved
| React.useEffect(() => { | ||
| store.update((prev) => { | ||
| const width = params.width ?? prev.dimensions.width; | ||
| const height = params.height ?? prev.dimensions.height; | ||
|
|
||
| return { | ||
| ...prev, | ||
| dimensions: { | ||
| margin: { | ||
| top: params.margin.top, | ||
| right: params.margin.right, | ||
| bottom: params.margin.bottom, | ||
| left: params.margin.left, | ||
| }, | ||
| width, | ||
| height, | ||
| propsHeight: params.height, | ||
| propsWidth: params.width, | ||
| }, | ||
| }; | ||
| if (isFirstRender.current) { | ||
| isFirstRender.current = false; | ||
| return; |
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 noticed this was causing an additional rendering for no reason
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 got a hook you can use for this now useEffectAfterFirstRender
| if (!onHighlightedAxisChange) { | ||
| return; | ||
| } |
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 is an early return to replace the skip parametter that does not exist on useStoreEffect()
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 the use of useStoreEffect, like, what is it for? 🤔
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 suppose it's for causing side-effects from store updates. This seems like a good use case: if the axis interaction changed, call the onChange callback
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.
Ah ok, so listening to a update in the store.
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.
Exactly. When a selector value get updated, the effect gets triggered
| zoomInteractionConfig: initializeZoomInteractionConfig(zoomInteractionConfig), | ||
| }, | ||
| }; | ||
| store.set('zoom', { |
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 have a "partial set" where we only provide the changed props and the behaviour provides the unchanged ones?
// current
set<T>(key: keyof State, value: T) {
if (!Object.is(this.state[key], value)) {
this.setState({ ...this.state, [key]: value });
}
}
// partial
partial<T>(key: keyof State, value: Partial<T>) {
this.setState({
...this.state,
[key]: { ...this.state[key], ...value }
});
}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 instead to implement support for nested keys? This would become store.set('zoom.zoomInteractionConfig', etc).
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.
Wouldn't that break the selectors? If the root object doesn't change how will siblings know that the parent updated? 🤔
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 implementation would mutate every parent object, just like it happens in your partial() example.
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.
Ah, yeah we could do that 😅
Then I believe that API is better indeed
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 might still need to update more than one prop though, so partial could still be useful 🙃
| React.useEffect(() => { | ||
| store.update((prev) => { | ||
| const width = params.width ?? prev.dimensions.width; | ||
| const height = params.height ?? prev.dimensions.height; | ||
|
|
||
| return { | ||
| ...prev, | ||
| dimensions: { | ||
| margin: { | ||
| top: params.margin.top, | ||
| right: params.margin.right, | ||
| bottom: params.margin.bottom, | ||
| left: params.margin.left, | ||
| }, | ||
| width, | ||
| height, | ||
| propsHeight: params.height, | ||
| propsWidth: params.width, | ||
| }, | ||
| }; | ||
| if (isFirstRender.current) { | ||
| isFirstRender.current = false; | ||
| return; |
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 got a hook you can use for this now useEffectAfterFirstRender
| if (!onHighlightedAxisChange) { | ||
| return; | ||
| } |
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 the use of useStoreEffect, like, what is it for? 🤔
Extraction from #20052
Just replaced the custom store by it's common version, and adapted the codebase to it.
Selectors remain untouched. Only the
useLazySelectorEffectgot replaced byuseStoreEffect