[charts] Divide the logic for useXxxSeries into useXxxSeriesContext

[JCQuintas]

Fixes #16537

• Divides useXxxSeries into useXxxSeries(seriesId?: SeriesId | SeriesId[]) and useXxxSeriesContext
• useXxxSeries() now returns all the series of type Xxx
  • Previously it returned the now useXxxSeriesContext, an object with { series, seriesOrder }
• Fix some typings
• Update demos

[mui-bot]

Deploy preview: https://deploy-preview-16546--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against aea0a5b

[codspeed-hq]

CodSpeed Performance Report

Merging #16546 will not alter performance

_{Comparing JCQuintas:series-context-hooks (aea0a5b) with master (9b0bc3f)}

Summary

✅ 6 untouched benchmarks

[bernardobelchior]

Not sure if problematic, but there's a slight change in behavior.

Before, passing invalid IDs would result in an undefined value at the same index, but now there's no undefined values in the returned array.

If there are two series s1 and s3, before createSeriesSelectorsOfType(['s1', 's2', 's3']) would return [series1, undefined, series3], but now it will return [series1, series3].

Isn't that a problem? Would make sense to return a object of ID to series instead since order doesn't matter?

[alexfauquette]

I'm not a big fan of the undefined because it impacts the typing. But a warning message for id that don't exist could be a plus

[bernardobelchior]

If we don't care about order (and I think we don't because it is provided by the user), wouldn't it make more sense to use an object?

Otherwise the user needs to do returnedSeries.find(series => series.id === 's3') to find the series identified by 's3', instead of returnedSeries['s3'].

[JCQuintas]

🤔 I like returning the object. I think I can make the typings work correctly as well

[JCQuintas]

On second thought, the ergonomics of returning an object would sub-optimal. Currently you can pass a single series, so if you want direct access to multiple series you should use

const s1 = useXxxSeries('1')
const s2 = useXxxSeries('2')

If you don't know how many series you want beforehand, you probably don't know their Ids as well, so you can pass an array and will receive an array, and if you need an object somehow you can easily iterate over it yourself.

[alexfauquette]

If we go back to potential usecases:

1. You know the series id you are looking for:
   1. you want a single series so you can use useLineSeries('the_id') to get the series
   2. you want multiple series fo which you know the ids, you can use useLineSeries([id1, id2])
2. You want lines but don't knwo the ids. You can use useLineSeries() to get all of them.

open questions

For cases 1.1 and 1.2 what happen when a user pass a wrong id? My personal opinion is to warn devs, but don't try to be too smart. In such case, the error will be on their side and we can do something else than informing them as clearly as possible.

Finally does the useXxxSeriesContext is still usefull since it does not cover a usecase? I think yes, because of some specific features like the staking groups.

About the question "If we don't care about order" I thik we do because it impacts how series are plotted on top of each other. This could be more reliable. Object.values does not give any guaranty on the returned order.

- return Object.values(processedSeries[seriesType]?.series ?? {});
+ return processedSeries[seriesType]?.seriesOrder?.map(seriesId => processedSeries[seriesType]?.series[seriesId]) ?? [];

[JCQuintas]

If the object was created "in-order", modern js keeps the order when using Object.values/keys/entries, predictable object key order is not recent anymore. 😆

So, Object.values does guarantee the return order, what we don't guarantee is the creation order. 😅

I'll create a PR with your suggestion and warn users

[alexfauquette]

what we don't guarantee is the creation order.

And the update 😇

{
  ...prevSeries,
  [idToUpdate]: {
    ...prevSeries[idToUpdate],
    newField, newValue
  }
}

[JCQuintas]

@alexfauquette that is still preserved. If it exists in ...prevSeries, it means it is added to the object first, in the correct order, then [idToUpdate] updates the previously created value, which doesn't change the order

[bernardobelchior]

I thought order didn't matter in the return array, but it if it is tied to the internal order of the series, then I agree that an object isn't the best representation for that.

What I find weird about returning an array is that we might provide an array with 3 IDs and get back an array with only two elements. If users aren't aware of this, they might do returnedArray[2] and it is undefined.

However, if the array contains the series in the order they are rendered in, then I think it makes sense to keep the array. If the order is the one passed in by the IDs, then I guess there's less benefits in returning an array.

Nevertheless, I don't think this is a blocker