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] Fix TS performances #12921

Closed
alexfauquette opened this issue Apr 26, 2024 · 3 comments · Fixed by #13137
Closed

[charts] Fix TS performances #12921

alexfauquette opened this issue Apr 26, 2024 · 3 comments · Fixed by #13137
Labels
component: charts This is the name of the generic UI component, not the React module! typescript

Comments

@alexfauquette
Copy link
Member

alexfauquette commented Apr 26, 2024

In #12920 the Typescript usage for pickers has been improved. Remain the AnimatedArea and BarElement to improve on charts

Search keywords: TypeScript

@alexfauquette alexfauquette added status: waiting for maintainer These issues haven't been looked at yet by a maintainer typescript component: charts This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Apr 26, 2024
@flaviendelangle
Copy link
Member

flaviendelangle commented Apr 26, 2024

Copy paste of my message on Slack:

For the Bar Chart issue, I think I was able to fix it by simplifying the type of the bar slot.
TypeScript struggled to match the props of BarElementPath with BarProps because of some heavy types inside @react-spring
Usually we don't define the props on those slots, we just set React.ElementType (on the pickers and on the core at least, the grid went the opposite way) and it seems to help here:

--- a/packages/x-charts/src/BarChart/BarElement.tsx
+++ b/packages/x-charts/src/BarChart/BarElement.tsx
@@ -73,7 +73,7 @@ export interface BarElementSlots {
    * The component that renders the bar.
    * @default BarElementPath
    */
-  bar?: React.JSXElementConstructor<BarProps>;
+  bar?: React.ElementType;

If we do want to keep this typing then we can assure TS that BarElementPath has the right value:

--- a/packages/x-charts/src/BarChart/BarElement.tsx
+++ b/packages/x-charts/src/BarChart/BarElement.tsx
@@ -60,7 +60,7 @@ export const BarElementPath = styled(animated.rect, {
     : ownerState.color,
   transition: 'opacity 0.2s ease-in, fill 0.2s ease-in',
   opacity: (ownerState.isFaded && 0.3) || 1,
-}));
+})) as React.JSXElementConstructor<BarProps>;

With both fixes, BarElement is still a hotspot but 3 to 4 type lighter on my machine (and without a trace so hard to say why, but it's inside the useSlotProps)

A few side notes:

Is there a reason why we use JSXElementConstructor instead of ElementType for the slots in the chart package?
React.ComponentPropsWithoutRef<'path'>, 'id' | 'color'> forces 2 Omit since React does an Omit to remove the ref, plus ComponentPropsWithoutRef forces to use the big table with all the HTML element listed. It seems more optimal (and more in line with the rest of our codebase) to use React.SVGProps<SVGPathElement> , which gives us : Omit<React.SVGProps<SVGPathElement>, 'id' | 'color' | 'ref'>
The other should be passed to externalForwardedProps instead of spread in additionalProps , the class should be passed to className instead of being inside additionalProps (several occurrences in the package)

@alexfauquette
Copy link
Member Author

Is there a reason why we use JSXElementConstructor instead of ElementType for the slots in the chart package?

Not that much, I picked it from the pickers' implementation

Copy link

⚠️ This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

@alexfauquette: How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

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

Successfully merging a pull request may close this issue.

2 participants