Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
JCQuintas opened this issue Aug 26, 2024 · 4 comments
Open

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

JCQuintas opened this issue Aug 26, 2024 · 4 comments
Labels
breaking change component: charts This is the name of the generic UI component, not the React module! v8.x

Comments

@JCQuintas
Copy link
Member

JCQuintas commented Aug 26, 2024

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

  • All charts:
    • dataset vs no dataset
      • The dataset mode could require that the dataKey is set in the series
      • data mode would forbid dataKey and require data

Search keywords:

@JCQuintas JCQuintas added breaking change component: charts This is the name of the generic UI component, not the React module! v8.x labels Aug 26, 2024
@alexfauquette
Copy link
Member

alexfauquette commented Aug 26, 2024

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

JCQuintas commented Aug 27, 2024

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 :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: charts This is the name of the generic UI component, not the React module! v8.x
Projects
None yet
Development

No branches or pull requests

2 participants