Prop type for Chart enforces optional props
mkarajohn opened this issue · 19 comments
Do you want to request a feature or report a bug?
Bug
What is the current behaviour?
Omitting optional props throws warnings in the IDE
What is the expected behaviour?
To not throw any warnings if I omit optional props
Steps to Reproduce the Problem
Create any Chart component and omit any of the props defined here
The change to the props was introduced in this commit and effectively makes these props required.
Specifications
- Version: 8.0.0
It would be my pleasure to open a PR in order to fix this
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.
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! 🙌
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!
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.
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.
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.
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
Added an entry to docs' todo.
Please feel free to reopen in case the bug doesn't come from WebStorm.
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
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.
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.
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?
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.ElementDo you maybe build through your IDE? I've just built the chart in a CRA with TS and no errors whatsoever.
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 :)
Just one last thing: would you mind pasting here your tsconfig.json file?
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
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.