[charts-pro] Simplify zoom testing #17525
Conversation
JCQuintas
commented
Apr 23, 2025
JCQuintas
commented
- Fixes a bug where the selector wouldn't be memoized
- Make zoom tests easier to reason about
JCQuintas
added
type: bug
|
Deploy preview: https://deploy-preview-17525--material-ui-x.netlify.app/ |
CodSpeed Performance ReportMerging #17525 will not alter performanceComparing Summary
|
Could you describe the bug you're fixing? |
| { | ||
| memoizeOptions: { | ||
| // This selector returns Record<AxisId, DefaultizedZoomOptions> which is a map. | ||
| // Whenever the component re-renders, the axis is often a new object, which makes this a new instance. |
There was a problem hiding this comment.
I don't understand.
If the axis object provided by the user is a new one. Then nearly everything will update.
Especially memorizing that zoom should be between 0.3 and 0.6 does not seem relevant if you're not sure the axis scale stays the same .
If we want to add a memo, it's more on the selectorChartRawXAxis
There was a problem hiding this comment.
Sorry, I didn't convey the full issue in the message.
The issue is this line above (xLookup, yLookup) => ({ ...xLookup, ...yLookup }),
If anything in the chain updates and requires a re-evaluation of the hook, not a re-render yet. This hook will force a re-render, because {...} is always a new object.
There was a problem hiding this comment.
And if the result of this selector is used on a useEffect for example, it will always re-trigger it.
There was a problem hiding this comment.
I might miss understand something about selectors.
For me if a selector got the same input it does not run computation and return the memoized result.
The xLookup (same for the yLookp) is derived from the state with the following chain of selectors:
state -> selectorChartCartesianAxisState -> selectorChartRawXAxis -> selectorChartXZoomOptionsLookup
So if user do not modify the xAxis the selectorChartRawXAxis should return the same object.
Since xLookup and yLookup are the same the selector should not run the function and just return the prefiouse ref.
There was a problem hiding this comment.
It is a bit confusing for me too, but as far as I can understand, there are two ways to PREVENT an update.
- The function args didn't change.
- The result itself didn't change.
If one of these is true, a re-render wont trigger. So we can prevent changes in two different ways.
What this does is that it prevents zoom re-render, even if anything up the tree changed, as long as the result of this specific selector doesn't change.
This means that anything that doesn't change the zoomOptions will be ignored for the sake of re-rendering.
When the zoom is controlled, the axis data will change a lot, and it can cause slowdowns. Event though the zoom options are mostly static. It would also create issues in the zoom hooks, as they would have to re-run mid-gesture.
The zoom options are the only "static" configuration that was updating a lot, an unexpectedly. (Because it would always create a new object).
This change makes sure it only updates when it needs to.
There was a problem hiding this comment.
When the zoom is controlled, the axis data will change a lot, and it can cause slowdowns. Event though the zoom options are mostly static. It would also create issues in the zoom hooks, as they would have to re-run mid-gesture.
I struggle to reproduce your issue. I'm playing with the https://mui.com/x/react-charts/zoom-and-pan/#zoom-synchronization and this hook only runs once on mount. I can zoom, pan, and reset zoom the console log inside the function is never called as expected.
Could you share an example of the interaction you're fixing?
There was a problem hiding this comment.
Yes, the user would have to pass onZoomChange={(v) => setZoomData(v)} for the issue to happen.
There was a problem hiding this comment.
On my pc the issues happen when I interact fast with the chart, while logging/inspect is open.
There was a problem hiding this comment.
I've remove it from this PR for now
| // This selector returns Record<AxisId, DefaultizedZoomOptions> which is a map. | ||
| // Whenever the component re-renders, the axis is often a new object, which makes this a new instance. | ||
| // We need to use a custom equality check to avoid re-rendering the chart. | ||
| resultEqualityCheck: isDeepEqual, |
There was a problem hiding this comment.
I think we might be able to achieve the same solution with a different approach.
We're usually getting the zoomOptions only to later access a specific axis' options by doing zoomOptions[axis.id]. Would it make sense to create a selector for that directly (e.g., selectorChartAxisZoomOptions)?
That way in most cases we would skip creating this object and the functions would receive the zoom options for the given axis. Plus, we would also avoid using deep equal here.
There was a problem hiding this comment.
I would rather not change it now as I use it in a different way on another PR, which this PR was extracted from.
But I like the idea and will try to test it there
There was a problem hiding this comment.
In this comment I argued for using isDeepEqual because I thought it would be better and under the condition that we don't use it often, but some days later we're already introducing another usage. That's the reason I'm a bit hesitant about this.
If we can find a way to replace this deep equal (either my suggestion or another approach), and if we agree that we'll refactor the deep equal after this PR and its follow-up are merged, then I'm good and we can merge it. Does that sound good?
My fear is that we might introduce performance problems if we overuse deep equals, and I think we should try to avoid them.
There was a problem hiding this comment.
I've remove it from this PR for now
JCQuintas
changed the title
