-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts] Use selectors from @mui/x-internals
#20052
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-20052--material-ui-x.netlify.app/ Bundle size report
|
CodSpeed Performance ReportMerging #20052 will improve performances by 10%Comparing Summary
Benchmarks breakdown
Footnotes |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
| const propsWidth = useSelector(store, selectorChartPropsWidth); | ||
| const propsHeight = useSelector(store, selectorChartPropsHeight); |
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.
Use selectors that return number to avoid having to memoize those
| store, | ||
| selectorSeriesWithIds, | ||
| ids, | ||
| // fastArrayCompare |
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 be nice but not urgent to bring that logic.
The issue it solves is the following one:
If any processesd series get updated, the follwoing hook will trigger a re-render, because it returns a new array of series. Event if the objects are exactly the same
const specialSeries = useBarSeries(['id1', 'id2'])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.
Could we spread the IDs? useBarSeries('id1', 'id2')?
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 really because selectors are limited to 3 args. so 3 ids in that case.
But the more I think about it, the more I consider we could remove the array option
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.
But the more I think about it, the more I consider we could remove the array option
It would be a breaking change, wouldn't it?
I guess it can make sense if you have programmatically generated series, though. Otherwise you need N hooks for N series.
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 guess it can make sense if you have programmatically generated series, though. Otherwise you need N hooks for N series.
you can always provide no array and get all series, albeit the result shape is different
It would be a breaking change, wouldn't it?
yeah
we could do something like useBarChart([1,2,3]) => arg0.map(v => series[v]) though
| : acc, | ||
| 0, | ||
| ), | ||
| export const selectorChartAxisSizes = createSelectorMemoized( |
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 grouped all the axis sizes in a single memoized object to reduce the number of selectors for drawing area
| [ | ||
| selectorChartRawXAxis, | ||
| selectorChartSeriesProcessed, | ||
| selectorChartSeriesConfig, | ||
| selectorChartZoomOptionsLookup, | ||
| selectorChartDrawingArea, | ||
| selectorChartPreviewXScales, | ||
| selectorChartXDomains, | ||
| (_, axisId: AxisId) => axisId, | ||
| selectorChartXAxisWithDomains, | ||
| ], | ||
|
|
||
| ( |
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 selector was exceeding the number of allowed selectors.
Since selectorChartXDomains has selectorChartRawXAxis as an input, I modified selectorChartXDomains to return both the domains and the raw axes
| // { | ||
| // memoizeOptions: { | ||
| // // Keep the same reference if array content is the same. | ||
| // // If possible, avoid this pattern by creating selectors that | ||
| // // uses string/number as arguments. | ||
| // resultEqualityCheck: isDeepEqual, | ||
| // }, | ||
| // }, |
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 should bring that back
The selectors get the pointer x-coordinate, and the x-axes. From that it derives axes data indexes associated to the pointer.
This isDeepEqual was here to update the value only when the pointer moves from one value to another.
@romgrk Woudl you be ok if we add an option in createSelectorMemoized to modify the memoizeOptions?
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 it's not possible to inline the arguments to avoid the array, then yes we could, but that sounds like a hack so I would avoid it if possible.
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've inlined the one I could, but this one does not look feasible
| }, | ||
| }, | ||
| ); | ||
| store.set('voronoi', { isVoronoiEnabled: !disableVoronoi }); |
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 voronoi state only have isVoronoiEnabled property
| // { | ||
| // memoizeOptions: { | ||
| // // Keep the same reference if array content is the same. | ||
| // // If possible, avoid this pattern by creating selectors that | ||
| // // uses string/number as arguments. | ||
| // resultEqualityCheck: isDeepEqual, | ||
| // }, | ||
| // }, | ||
| ); |
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.
Same issue as in the cartesian axes
|
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. |
@mui/x-internals
| }, | ||
| ); | ||
|
|
||
| const noop: AxisItemIdentifier[] = []; |
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.
Might be more descriptive:
| const noop: AxisItemIdentifier[] = []; | |
| const EMPTY_ARRAY: AxisItemIdentifier[] = []; |
| /** | ||
| * Method wrapping reselect's createChartSelector to provide caching for chart instances. | ||
| */ | ||
| export const createSelector = < |
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 this is meant as a stepping stone to remove later, right? What's preventing us from using the createSelector from the x-internals store directly?
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.
To do so you would have to modify all the selectors because of a small difference. Si it's mostly to avoid useless diff in the initial PR
// our
createSelector([select1, select2], combiner)
// internal
createSelector(select1, select2, combiner)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.
Makes sense.
Do we want to migrate to the new API or keep our current one? Just so we align on new selectors
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 would be in favor of moving to the new selectors API.
That's why I directly imported the memoized one.
For the migration we would "just" have to replace all the imports and remove the [] around selectors
| store, | ||
| selectorSeriesWithIds, | ||
| ids, | ||
| // fastArrayCompare |
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.
Could we spread the IDs? useBarSeries('id1', 'id2')?
bernardobelchior
left a comment
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.
Thanks for this 🙏
| : acc, | ||
| 0, | ||
| ), | ||
| export const selectorChartLeftAxisSize = createSelectorMemoized( |
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.
Why are these memoized if they return a 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.
Probably nothing 🫣
JCQuintas
left a comment
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.
no issues, only questions 😆
Co-authored-by: Jose Quintas <juniorquintas@gmail.com>
First step for #20046 I do as few renaming as possible
Still need to work on the memoization aspect: Which selector should be memoized or not