[charts] Make typescript more flexible about plugins and their params #16478
Conversation
alexfauquette
added
the
component: charts
|
Deploy preview: https://deploy-preview-16478--material-ui-x.netlify.app/ |
CodSpeed Performance ReportMerging #16478 will not alter performanceComparing Summary
|
| @@ -19,7 +22,7 @@ export const useChartContainerProps = < | |||
| >( | |||
| props: ChartContainerProps<TSeries, TSignatures>, | |||
| ref: React.Ref<SVGSVGElement>, | |||
| ): UseChartContainerPropsReturnValue<TSeries> => { | |||
| ): UseChartContainerPropsReturnValue<TSeries, TSignatures> => { | |||
There was a problem hiding this comment.
The hack I found is to force the type as AllPlugins but export the DataProvideProps as TSignatures
| export const useChartCartesianAxis: ChartPlugin< | ||
| UseChartCartesianAxisSignature<ChartSeriesType> | ||
| > = ({ params, store, seriesConfig, svgRef, instance }) => { | ||
| export const useChartCartesianAxis: ChartPlugin<UseChartCartesianAxisSignature<any>> = ({ |
There was a problem hiding this comment.
<any> here seems better than the previous ChartSeriesType, since ChartSeriesType means all of these, which causes conflicts.
I believe the main issue in in the ChartPlugin signature. As a function with extra props (getInitialState,models,etc) is a bit complex to type.
There was a problem hiding this comment.
Note for myself tomorrow: Test if CartesianChartsSeriesType make it works.
Otherwise I agree that the any is better than the override
There was a problem hiding this comment.
After verification it does not work
There was a problem hiding this comment.
Yeah it wont work with anything other than "any". To make it work we would have to make it work with useChartCartesianAxis<'bar'>, but currently only the type is generic.
This means that the we can make the type specific, but not the implementation. So a: UseChartCartesianAxis<'bar'> = useChartCartesianAxis gives an error, because useChartCartesianAxis = useChartCartesianAxis<'bar'> | useChartCartesianAxis<'pie'> | ...
Not specifically this example, but you get the gist I suppose. a: Type = b kinda of coerces the types if they are compatible 😆
github-actions
bot
added
the
PR: out-of-date
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
github-actions
bot
removed
the
PR: out-of-date
| "colors": { "type": { "name": "any" }, "default": "rainbowSurgePalette" }, | ||
| "dataset": { "type": { "name": "any" } }, | ||
| "height": { "type": { "name": "any" } }, | ||
| "id": { "type": { "name": "any" } }, | ||
| "margin": { "type": { "name": "any" } }, | ||
| "series": { "type": { "name": "any" } }, | ||
| "skipAnimation": { "type": { "name": "any" } }, |
There was a problem hiding this comment.
We really have to move away from our current doc generator 😆
Extracted from the radar and voronoid PRs.
The introduction of the polar and voronoid plugins reveal to missing usecases:
The typing of
useChartContainerPropsanduseChartContainerProPropsassumed all the plugins were set. And the pie chart was managed by avoiding this hook and manually manage props propagation.The radar chart needs a different set of plugins (replacing the cartesian by the polar one)
Same for the voronoi plugin which makes sens to add only for the Scatter chart and maybe the line chart latter.
This PR extends the propagation of the generics to set correctly the number plugins used by each chart.
It also introduces a file
XxxCharts.plugins.tswhich defines the array and plugins and its signature. This is done in order to clarify which chart uses which plugin