Prop type for Chart enforces optional props about react-minimal-pie-chart HOT 19 CLOSED

In the meanwhile I found that the React team decided to deprecate defaultProps for function component. 😱

In the next future we might consider getting rid of them and switch to JS default arguments even though this would mess up the code quite a bit.

from react-minimal-pie-chart.

Added an entry to docs' todo.

Please feel free to reopen in case the bug doesn't come from WebStorm.

from react-minimal-pie-chart.

Hi @mkarajohn,
thanks for reporting!

This could be related to the way we join props and defaultProps types. Let's find out.

I'm quite concerned about the fact that tests haven't caught the issue.

from react-minimal-pie-chart.

Hi,

This could be related to the way we join props and defaultProps types. Let's find out.

Yes, it seems to me as well, this is the cause.

I think the intersection in this line makes the defaultProps required

You can see this TS sandbox example, for the same effect

from react-minimal-pie-chart.

I thought about it for a moment.

From the library's perspective PropsWithDefaults describes the props object (with default props) expected by PieChart function component.

Default props are therefore marked as required because they are expected to be provided or defaulted by React. In my usual TS+React routine I can't get any typing error.

And PropsWithDefaults is what you get when you retrieve components' props with:

React.ComponentProps<typeof PieChart>

Since pure Props types alone are not exported, there's currently no way to retrieve the type definition of the props the user is expected to provide. Which is probably a quite common issue with React's defaultProps in TS.

I'm totally open to find a better typing setup.

BTW, can I see a repro of your typing error? Maybe you're using the library in a way I haven't taken into account.

Cheers! 🙌

from react-minimal-pie-chart.

My setup is WebStorm 2019.3.4 on Mac and Windows 10. I get the warning regardless of OS.

I am using the component like this

<PieChart
              key={'proper'}
              animate
              animationDuration={500}
              animationEasing="ease-out"
              background={'#bfbfbf'}
              css={chartStyles}
              lineWidth={60}
              data={chartData}
            />

I cannot disclose any more code than that.

Default props are therefore marked as required because they are expected to be provided or defaulted by React.

I don't think default props should be marked as required. On that regard, my IDE is right for throwing those warnings at me.

Think about it. Default props are optional props, in the end. Whether or not your component handles them with some default value is an implementation detail. To the user they should not be any different from any other optional props, during development time. One can refer to the docs to tell what props are defaulted with what values.

If I alter the definition in the Chart.d.ts file to export declare function PieChart(props: Props): JSX.Element; the Typescript gods are appeased.

Cheers!

from react-minimal-pie-chart.

Hi @mkarajohn,
crazy idea: could the issue be related to WebStorm?! I took a look to it's bug tracker and found a few reports that seem to be similar to what you described.

from react-minimal-pie-chart.

Not a crazy idea at all :D. It may be one of these issues, However i am not sure which one could it be. The 1st one in that list (which is very similar to this case) is closed as a duplicate of this which is supposed to be fixed. I don't know, honestly, proceed with this issue however you feel like.

from react-minimal-pie-chart.

In the meanwhile I found that the React team decided to deprecate defaultProps for function component. 😱

Yep, that's true. For the better, if you ask me

from react-minimal-pie-chart.

Hi, it's this time of the year again 😄

Well, I upgraded to TS 4.1.3 from 3.9.7 and I got this exact same error again. I actually had completely forgotten about this issue, until now, when I searched for it and got here again 😅

This time I am on Linux.
A quick search at JetBrain's issue tracker brought nothing

Still the only fix for me is to do export declare function PieChart(props: Props): JSX.Element;

// global.d.ts

declare module 'react-minimal-pie-chart' {
  declare type Props = {
    animate?: boolean;
    animationDuration?: number;
    animationEasing?: string;
    background?: string;
    center?: [number, number];
    children?: ReactNode;
    className?: string;
    data: Data;
    lengthAngle?: number;
    lineWidth?: number;
    label?: LabelRenderFunction;
    labelPosition?: number;
    labelStyle?: CSSProperties | ((dataIndex: number) => CSSProperties | undefined);
    onBlur?: EventHandler<FocusEvent>;
    onClick?: EventHandler<MouseEvent>;
    onFocus?: EventHandler<FocusEvent>;
    onKeyDown?: EventHandler<KeyboardEvent>;
    onMouseOut?: EventHandler<MouseEvent>;
    onMouseOver?: EventHandler<MouseEvent>;
    paddingAngle?: number;
    radius?: number;
    reveal?: number;
    rounded?: boolean;
    segmentsShift?: number | ((dataIndex: number) => number | undefined);
    segmentsStyle?: CSSProperties | ((dataIndex: number) => CSSProperties | undefined);
    segmentsTabIndex?: number;
    startAngle?: number;
    style?: CSSProperties;
    totalValue?: number;
    viewBoxSize?: [number, number];
  };

  declare function PieChart(props: Props): JSX.Element;
}

I still think my response here has some base.

Cheers

from react-minimal-pie-chart.

Hi @mkarajohn,
I can't say for sure it's JetBrain. From a maintainer perspective I'd be inclined to take Typescript as a source of truth. If TS doesn't complain then it's fine.

User facing types are tested here and executed with bare tsc from here. Would it be possible to reproduce the error outside the IDE?

As a next step we could get rid of React's defaultProps (respecting size constraints of this library), but it's not on my top priority right now :) Happy to help out if you wanted to try a PR.

from react-minimal-pie-chart.

Well, it does in fact throw when simply running tsc

$ yarn tsc
yarn run v1.22.5
$ /mnt/4E141ED6141EC13F//Workspace//node_modules/.bin/tsc
src/pages/IssuesPage/subpages/Conflicts/components/Chart/Chart.tsx:155:12 - error TS2740: Type '{ key: string; background: string; css: SerializedStyles; lineWidth: number; data: never[]; }' is missing the following properties from type '{ animationDuration: number; animationEasing: string; center: [number, number]; data: Data; labelPosition: number; lengthAngle: number; lineWidth: number; paddingAngle: number; radius: number; startAngle: number; viewBoxSize: [...]; }': animationDuration, animationEasing, center, labelPosition, and 5 more.

155           <PieChart
               ~~~~~~~~

src/pages/IssuesPage/subpages/Conflicts/components/Chart/Chart.tsx:166:14 - error TS2740: Type '{ key: string; animate: true; animationDuration: number; animationEasing: string; background: string; css: SerializedStyles; lineWidth: number; data: ChartNormalizedData[]; }' is missing the following properties from type '{ animationDuration: number; animationEasing: string; center: [number, number]; data: Data; labelPosition: number; lengthAngle: number; lineWidth: number; paddingAngle: number; radius: number; startAngle: number; viewBoxSize: [...]; }': center, labelPosition, lengthAngle, paddingAngle, and 3 more.

166             <PieChart
                 ~~~~~~~~


Found 2 errors.

I tried replicating here but I couldn't. https://codesandbox.io/s/optimistic-voice-mxfvs?file=/src/App.tsx

Honestly, I am not sure what is happening. For me it makes sense that it is throwing (wth the union of the Props and DefaultProps, also see this, the sandbox specifically) but if I cannot reproduce it in another environment, what can I say 😅

So, what should I do? Would a PR that undoes the union of the Props and DefaultProps be an acceptable approach? You tell me.

from react-minimal-pie-chart.

2 3 brief questions:

• Can you build your app?
• Is there a way to replicate the issue out of your machine?
• What's your IDE and which version?

from react-minimal-pie-chart.

Can you build your app?

No, the compiling process throws, since this is a TS error

Is there a way to replicate the issue out of your machine?

I tried in the codesanbox, above, but I cannot reproduce.

But it also happens on my Mac machine, not only on Linux

What's your IDE and which version?

On linux:
WebStorm 2020.3
Build #WS-203.5981.135, built on November 27, 2020

On Mac:
WebStorm 2020.2.4
Build #WS-202.8194.6, built on November 24, 2020

BTW, this i how my IDE sees the type when I hover over the component

export function PieChart(     props: Props & {animationDuration: number, animationEasing: string, center: [number, number], data: Data, labelPosition: number, lengthAngle: number, lineWidth: number, paddingAngle: number, radius: number, startAngle: number, viewBoxSize: [number, number]}):    JSX.Element

from react-minimal-pie-chart.

Do you maybe build through your IDE? I've just built the chart in a CRA with TS and no errors whatsoever.

from react-minimal-pie-chart.

Do you maybe build through your IDE? I've just built the chart in a CRA with TS and no errors whatsoever.

I build from within the IDE, yes, but the tsc command whose output I posted above was executed from a standalone terminal, outside of the IDE.

Like I said, I am just as confused, but since I have found a workaround, I am not really requesting for a fix :)

from react-minimal-pie-chart.

Just one last thing: would you mind pasting here your tsconfig.json file?

from react-minimal-pie-chart.

Sure, I don't mind even if it's not the last thing 😄

{
  "compilerOptions": {
    "target": "es5",
    "lib": ["dom", "dom.iterable", "esnext"],
    "baseUrl": "./src",
    "allowJs": true,
    "skipLibCheck": true,
    "esModuleInterop": true,
    "allowSyntheticDefaultImports": true,
    "strict": true,
    "forceConsistentCasingInFileNames": true,
    "module": "esnext",
    "moduleResolution": "node",
    "resolveJsonModule": true,
    "isolatedModules": true,
    "noEmit": true,
    "jsx": "react",
    "downlevelIteration": true,
    "noFallthroughCasesInSwitch": true
  },
  "include": ["src"]
}

The codesandbox from above uses the basic configurations I use on my machine

from react-minimal-pie-chart.

I found this thread when looking for a solution to this exact problem.

So the workaround is to override the TypeScript types? Seems broken. At least this issue should be re-opened.

I'm on TypeScript 4.1.2. Would really appreciate a fix for this. My workaround for now is to @ts-ignore your type signature, which is hardly any good when the objective is to increase the strictness of the types everywhere.

from react-minimal-pie-chart.