mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.42k stars 1.3k forks source link

[charts] Provide context-aware types based on props #14340

Open JCQuintas opened 2 months ago

JCQuintas commented 2 months ago

A current frustration with types right now is that we can't provide context aware types for the users due to how our types are declared.

This change could provide valuable IDE information to our final users in how the components can be used.

Example

import * as React from 'react';

type Prop1 = {
  height: number;
  width: number;
  data: number[];
  responsive?: never;
};

type Prop2 = {
  responsive: true;
  data: string[];
};

type Props = Prop1 | Prop2;

const Component = (props: Props) => {
  return null;
};

export default function Page() {
  return (
    <>
      <Component responsive data={['aaa']} />
      <Component height={1} width={2} data={[2]} />
      {/* Unexpected prop value */}
      <Component responsive={false} />
      {/* Data should be string when used with responsive */}
      <Component responsive data={[1]} />
    </>
  );
}

Possible implementations

Search keywords:

alexfauquette commented 2 months ago

Other options could be

interface Props<IsDataset> {
  dataset: IsDataset ? object[] : undefined
  series: BarSeries<IsDataset>
}

or


interface PropsWithDataset extends CommonProps { ... }
interface PropsWithoutDataset extends CommonProps { ... }

function Component (props: PropsWithDataset)
function Component (props: PropsWithoutDataset)
function Component (props: PropsWithoutDataset | PropsWithtDataset)
JCQuintas commented 2 months ago

Other options could be

the generic interface approach doesn't seem to work because the generic needs to be defined, and can't be automatically inferred by the passed prop

interface Gen<IsDataset> {
  dataset?: IsDataset extends any[] ? string[] : never;
  series?: IsDataset extends any[] ? never : string[];
}

const ComponentGen = <IsDataset extends any[]>(props: Gen<IsDataset>) => {
  return null;
};

export default function Page() {
  return (
    <>
      <ComponentGen dataset={['aaa']} />
      <ComponentGen series={['aaa']} />
      <ComponentGen dataset={['aaa']} series={['aaa']} />
    </>
  );
}

The second approach can work nicely. It also picks up info from the input props if necessary, eg:

interface Prop1<Dataset extends any[] = any[]> {
  height: number;
  width: number;
  data: Dataset;
  dataKey: Dataset extends (infer DataItem)[] ? keyof DataItem : never;
  responsive?: never;
};

interface Prop2  {
  responsive: true;
  data: string[];
};

const Component = <Dataset extends any[]>(props: Prop1<Dataset> | Prop2) => {
  return null;
};

export default function Page() {
  return (
    <>
      <Component responsive data={['aaa']} />
      <Component height={1} width={2} data={[{ go: true, nice: 'foo' }]} dataKey="go" />
      {/* Unexpected prop value */}
      <Component responsive={false} />
      {/* Data should be string when used with responsive */}
      <Component responsive data={[1]} />
    </>
  );
}
alexfauquette commented 2 months ago

WHile working on the typing, I noticed that because we extends configuration, the distinction between pro and community props is mixed.

For example If you have in your project a mix of pro and community component, the axis config will be typed as a pro one.

<BarChartPro xAxis={[{ zoom: true }]} /> // TS passes and it's expected
<BarChart xAxis={[{ zoom: true }]} /> // TS passes but should fail because MIT  chart don't have the zoom
JCQuintas commented 2 months ago

WHile working on the typing, I noticed that because we extends configuration, the distinction between pro and community props is mixed.

True, the ZoomProps are correct because the new XxxPro components have their own types, but the AxisConfigExtension applies to the axis of pro and community :(