[charts] Split defaultizeAxis function into two

[bernardobelchior]

Split the defaultizeAxis function into defaultizeXAxis and defaultizeYAxis with the goal of simplifying types.

[bernardobelchior]

In addition to splitting the function, I'm considering making the types more accurate (and maybe predictable as well?).

Current State

It seems we have two "defaultizing" processes.

First, when we update the store in useChartCartesianAxis, then when we call computeAxisValue when using the selectorChartXAxis or selectorChartYAxis selectors.

At the moment, defaultizeAxis returns AxisConfig[], which is incorrect because some defaults are now provided, such as id, offset, position and width (for y-axes, or height for x-axes), but AxisConfig declares that these properties are optional.

Then, computeAxisValue receives an axis argument that is AxisConfig[], which is incorrect, and returns a AxisDefaultized<ScaleName, any, AxisProps>. Here, the types is still missing the optional properties that were given defaults in defaultizeAxis. Plus, computeAxisValue returns a { axis: DefaultizedAxisConfig<T extends ChartsAxisProps> , axisIds: string[] } which is a bit misleading as I'd expect the DefaultizedAxisConfig to be returned from a function named defaultizeAxis.

Proposal

1. Rename the current AxisConfig to RawAxisConfig or InputAxisConfig. The idea is to make it clear this data is untreated and that it probably needs some processing before usage;
2. Split defaultizeAxis into two function, one for x-axes and another for y-axes;
3. Update defaultizeXAxis to return an array of XAxisWithDefaults;
4. Update computeAxisValue to return ComputedXAxis.

@alexfauquette @JCQuintas what do you think?

[mui-bot]

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

Generated by 🚫 dangerJS against c0e5c11

[codspeed-hq]

CodSpeed Performance Report

Merging #16745 will not alter performance

_{Comparing bernardobelchior:split-defaultize-fn (c0e5c11) with master (b83be87)}

Summary

✅ 7 untouched benchmarks

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[JCQuintas]

1. Rename the current AxisConfig to RawAxisConfig or InputAxisConfig. The idea is to make it clear this data is untreated and that it probably needs some processing before usage;

We would have to provide type AxisConfig = NewName. Though I don't think the new name is necessary. We can just keep using AxisConfig.

• Split defaultizeAxis into two function, one for x-axes and another for y-axes;

Feels a bit unnecessary, the functions would be pretty much the same, dividing the types should be enough in this case.

• Update defaultizeXAxis to return an array of XAxisWithDefaults;

We could start renaming defaultizeXxx to initializeXxx and DefaultizedXxx to InitializedXxx.

[bernardobelchior]

We would have to provide type AxisConfig = NewName.

To avoid breaking changes, you mean?

Though I don't think the new name is necessary. We can just keep using AxisConfig.

Yeah, we can. I think a name that makes it clear that the config still needs to be processed would be clearer, but we don't need to change it.

• Split defaultizeAxis into two function, one for x-axes and another for y-axes;

Feels a bit unnecessary, the functions would be pretty much the same, dividing the types should be enough in this case.

My reasoning for suggesting the splitting of defaultizeAxis is that it can properly infer the types, allowing us to remove overloads and some extends clauses.

What do you mean by dividing types? Wouldn't that mean we'd still need to keep overloads/use extends clauses?

• Update defaultizeXAxis to return an array of XAxisWithDefaults;

We could start renaming defaultizeXxx to initializeXxx and DefaultizedXxx to InitializedXxx.

Yeah, sounds good 👍

[JCQuintas]

To avoid breaking changes, you mean?

yeah

My reasoning for suggesting the splitting of defaultizeAxis is that it can properly infer the types, allowing us to remove overloads and some extends clauses.

What do you mean by dividing types? Wouldn't that mean we'd still need to keep overloads/use extends clauses?

Depends on the type, but yes, in this case you are right that it would create an issue.