Cache sorting order of dataset metas #6757
Conversation
| @@ -446,8 +447,11 @@ helpers.extend(Chart.prototype, /** @lends Chart */ { | |||
| meta.controller = new ControllerClass(me, i); | |||
| newControllers.push(meta.controller); | |||
| } | |||
| sorted.push(meta); | |||
There was a problem hiding this comment.
I wonder if we should check if the dataset is visible here so that we don't have to do it each time _getSortedDatasetMetas is called
There was a problem hiding this comment.
I don't think we necessarily need a new property visible cause that's basically just !hidden, right?
I was wondering if we could do the filtering of only visible datasets here to cache that as well, but on second look it seems like that's not possible because the legend shows the labels for visible and non-visible datasets
There was a problem hiding this comment.
Chart.js/src/core/core.controller.js
Lines 897 to 899 in eb0751c
There was a problem hiding this comment.
I'm not sure about the use case where datasets are shared between charts, @etimberg any insights?
There was a problem hiding this comment.
I'm not quite sure where we modify the data object, but stopping that sounds like another potentially good thing to change
There was a problem hiding this comment.
Perhaps 'modify' is the wrong word, but we attach extra info to the data object and so systems that treat that as immutable are unhappy
Chart.js/src/core/core.controller.js
Line 863 in e5b03a0
I used chart.js in a React project recently and tried writing my own wrapper. I had a look at the source code of react-chartjs-2 and they have to do a lot of work to handle the fact that the data object gets mutated. Dropping that would significantly simplify things
There was a problem hiding this comment.
I wonder if we need to treat options as immutable as well. Here's a PR for fixing dataset._meta: #6778
There was a problem hiding this comment.
So the meta.hidden and dataset.hidden are both needed. dataset.hidden is provided in dataset configuration (loaded from storage maybe) and meta.hidden is used to override that, when clicking legend (avoiding touching the provided configuration).
Now this could be changed to read the dataset.hidden to meta.hidden at initializion and only use meta.hidden runtime. Provide hide() and show() for developer access. But not in the scope of this PR.
There was a problem hiding this comment.
Should visible be made private (i.e. _visible) since we may change it in the future? (maybe even worth adding a TODO to make the change you suggested)
Timings from uPlot bench, but with stacked y-scale.
Master
PR
