Bad type on activeDot onClick #3922
Labels
bug
General bug label
P0
Critical priority issues
typescript
PRs or Issues surrounding Types or TS refactoring
Comments
|
Did this just happen in 2.9.1 or was it like this previously? |
|
@andrewangelle 😅. Sorry to keep tagging you but fyi. Looks like its the ActiveShape type change |
|
we were on the 2.3.2 before. The payload type was missing but at least it was usable. |
|
If you want, I can check if it works differently on different version? Do you want me to try it on some specific version? |
|
I'm afk else I'd check, but if you can find the highest version that you don't break with that would be awesome! Just as an explanation of why the types narrowed - recharts types are.. really bad. Which makes it tough for us to make changes in a reasonable way (all of the anys make us question what is what, etc.) These changes were supposed to be backwards compatible to what the types currently represent but looks like unfortunately weren't (if what I think happened did). |
|
2.8.0 looks like it works.. or at least there are no errors 😅 I hate the types but at least it works without @ts-ignore 😄
With 2.9.1 it looks like this |
|
btw we are using "typescript": "^4.6.2", if it makes any difference 🙂 |
Can you explain why you hate the types 😅? The goal is to make them better, more accurate - not worse. |
|
https://github.com/recharts/recharts/blame/master/src/cartesian/Line.tsx#L68 This is what broke Related PR #3772 |
ckifer
added
P0
@ckifer Please feel free to continue to do so. I have a fix. I'll have it up today. |
|
@andrewangelle 🙌 appreciate you! |
9 tasks
ckifer
pushed a commit
that referenced
this issue
Oct 31, 2023
<!--- Provide a general summary of your changes in the Title above --> fix(Line): onClick for activeDot is incorrectly typed ## Description <!--- Describe your changes in detail --> Have `activeDot` include the type for `DotProps` which fixes the incorrect types for the event handlers that are added in [line 2036 in generateCategoricalChart](https://github.com/recharts/recharts/blob/master/src/chart/generateCategoricalChart.tsx#L2036 ) which give a different payload to the callback mentioned in the issue. ## Related Issue <!--- This project only accepts pull requests related to open issues --> <!--- If suggesting a new feature or change, please discuss it in an issue first --> <!--- If fixing a bug, there should be an issue describing it with steps to reproduce --> <!--- Please link to the issue here: --> #3922 ## Motivation and Context <!--- Why is this change required? What problem does it solve? --> Fixes incorrect types that introduced a breaking change ## How Has This Been Tested? Manually. It's a purely typescript issue. Screenshot below. <!--- Please describe in detail how you tested your changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots (if appropriate): <img width="669" alt="Screenshot 2023-10-31 at 4 22 50 PM" src="https://github.com/recharts/recharts/assets/24420033/6eff15ec-e7cc-4056-92ed-f44ba3d007a2"> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] My code follows the code style of this project. - [ ] My change requires a change to the documentation. - [ ] I have updated the documentation accordingly. - [ ] I have added tests to cover my changes. - [ ] I have added a storybook story or extended an existing story to show my changes - [x] All new and existing tests passed.
ckifer
pushed a commit
that referenced
this issue
Nov 1, 2023
<!--- Provide a general summary of your changes in the Title above --> fix(Line): onClick for activeDot is incorrectly typed ## Description <!--- Describe your changes in detail --> Have `activeDot` include the type for `DotProps` which fixes the incorrect types for the event handlers that are added in [line 2036 in generateCategoricalChart](https://github.com/recharts/recharts/blob/master/src/chart/generateCategoricalChart.tsx#L2036 ) which give a different payload to the callback mentioned in the issue. ## Related Issue <!--- This project only accepts pull requests related to open issues --> <!--- If suggesting a new feature or change, please discuss it in an issue first --> <!--- If fixing a bug, there should be an issue describing it with steps to reproduce --> <!--- Please link to the issue here: --> #3922 ## Motivation and Context <!--- Why is this change required? What problem does it solve? --> Fixes incorrect types that introduced a breaking change ## How Has This Been Tested? Manually. It's a purely typescript issue. Screenshot below. <!--- Please describe in detail how you tested your changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots (if appropriate): <img width="669" alt="Screenshot 2023-10-31 at 4 22 50 PM" src="https://github.com/recharts/recharts/assets/24420033/6eff15ec-e7cc-4056-92ed-f44ba3d007a2"> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] My code follows the code style of this project. - [ ] My change requires a change to the documentation. - [ ] I have updated the documentation accordingly. - [ ] I have added tests to cover my changes. - [ ] I have added a storybook story or extended an existing story to show my changes - [x] All new and existing tests passed.
|
Fixed in 2.9.2, resolving this |
|
@ckifer look better but payload.payload does not exist, would it be possible to have it in such a way that which is not true |
|
I'm not an official maintainer, just a contributor. But throwing in my two cents: I'm sure it's possible, but I believe that would be an enhancement. While I agree it's incorrect, this is consistent with the behavior prior to the types change in 
|
|
Yeah +1, the goal for the fix was to get back to where we were before we broke you in the simplest way. Adding anything else to the type has potential to add more issues rather than resolve the one that needed fixed. Feel free to open a new issue or contribute though! |
renovate bot
added a commit
to SAP/ui5-webcomponents-react
that referenced
this issue
Nov 2, 2023
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [recharts](https://togithub.com/recharts/recharts) | [`2.9.1` -> `2.9.2`](https://renovatebot.com/diffs/npm/recharts/2.9.1/2.9.2) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>recharts/recharts (recharts)</summary> ### [`v2.9.2`](https://togithub.com/recharts/recharts/releases/tag/v2.9.2) [Compare Source](https://togithub.com/recharts/recharts/compare/v2.9.1...v2.9.2) Fix another TS issue from 2.9. ##### Fix - `Line/ActiveDot`: Fix breaking type change for the `onClick` function of `activeDot` on `Line` - this resolves [recharts/recharts#3922 - thank you [@​andrewangelle](https://togithub.com/andrewangelle) for the quick turnaround </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/SAP/ui5-webcomponents-react). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMS41IiwidXBkYXRlZEluVmVyIjoiMzcuMzEuNSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
](https://renovatebot.com){kind=link}
bodinsamuel
pushed a commit
to specfy/specfy
that referenced
this issue
Nov 13, 2023
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [recharts](https://togithub.com/recharts/recharts) | [`2.8.0` -> `2.9.3`](https://renovatebot.com/diffs/npm/recharts/2.8.0/2.9.3) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>recharts/recharts (recharts)</summary> ### [`v2.9.3`](https://togithub.com/recharts/recharts/releases/tag/v2.9.3) [Compare Source](https://togithub.com/recharts/recharts/compare/v2.9.2...v2.9.3) #### Fix `Brush`: Fix an issue where after 2.9 `Brush` does not correctly slice data when using `Line` components - fixes [recharts/recharts#3929 - thank you [@​HHongSeungWoo](https://togithub.com/HHongSeungWoo) ### [`v2.9.2`](https://togithub.com/recharts/recharts/releases/tag/v2.9.2) [Compare Source](https://togithub.com/recharts/recharts/compare/v2.9.1...v2.9.2) Fix another TS issue from 2.9. ##### Fix - `Line/ActiveDot`: Fix breaking type change for the `onClick` function of `activeDot` on `Line` - this resolves [recharts/recharts#3922 - thank you [@​andrewangelle](https://togithub.com/andrewangelle) for the quick turnaround ### [`v2.9.1`](https://togithub.com/recharts/recharts/releases/tag/v2.9.1) [Compare Source](https://togithub.com/recharts/recharts/compare/v2.9.0...v2.9.1) Bug fixes following 2.9.0 #### Fix - `TypeScript`: fix breaking change in `ActiveShape` types - fixes [recharts/recharts#3911 - thanks [@​andrewangelle](https://togithub.com/andrewangelle) - `CartesianGrid`: fix breaking change where you could no longer render `CartesianGrid` without a y-axis - fixes [recharts/recharts#3907 - thanks [@​akamfoad](https://togithub.com/akamfoad) - `Line`: fix infinite loop when `strokeDasharray` is `'0'` on `Line` - fixes [recharts/recharts#3899 (and maybe others) **Full Changelog**: recharts/recharts@v2.9.0...v2.9.1 ### [`v2.9.0`](https://togithub.com/recharts/recharts/releases/tag/v2.9.0) [Compare Source](https://togithub.com/recharts/recharts/compare/v2.8.0...v2.9.0) #### What's Changed Quite a lot this minor release! We sent out a cry for help and many answered - thank you so much for that 🙌🏼 This release aims at internal maintainability, long lingering bugs, and needed improvements. Highlights include equidistant tick improvements, an active bar feature, and an ~85k/~9kb (gzipped) bundle size reduction 🚀 ##### Feat - `Bar`: Implement activeBar for Bar component by [@​andrewangelle](https://togithub.com/andrewangelle) in [recharts/recharts#3756 - `CartesianGrid`: add `syncWithticks`, `horizonalValues`, and `verticalValues` props to allow more grid line customization by [@​morozovkirill](https://togithub.com/morozovkirill) in [recharts/recharts#3746 solves [recharts/recharts#2153 - `CartesianAxis`: Improve interval option 'equidistantPreserveStart' by [@​nikolasrieble](https://togithub.com/nikolasrieble) in [recharts/recharts#3768 - `CartesianAxis`: Throw an invariant when axisIds do not match between chart and axis components by [@​ckifer](https://togithub.com/ckifer) - `Brush`: add onDragEnd event to Brush component by [@​simkesd](https://togithub.com/simkesd) in [recharts/recharts#3774 ##### Fix - Active Shape improvements - `Funnel`: activeShape should work with Tooltip by [@​andrewangelle](https://togithub.com/andrewangelle) in [recharts/recharts#3772 - `Scatter`: activeShape should work with Tooltip by [@​andrewangelle](https://togithub.com/andrewangelle) in [recharts/recharts#3839 - `Pie`: activeShape should work with Tooltip by [@​andrewangelle](https://togithub.com/andrewangelle) in [recharts/recharts#3818 - `RadialBar`: activeShape should work with Tooltip by [@​andrewangelle](https://togithub.com/andrewangelle) in [recharts/recharts#3803 - `CartesianGrid`: Remove offset attribute from lines by [@​branberry](https://togithub.com/branberry) in [recharts/recharts#3854 solves [recharts/recharts#3810 - `ResponsiveContainer`: style prop is now passed down correctly by [@​d-gottlieb](https://togithub.com/d-gottlieb) in [recharts/recharts#3726 - `Legend`: "Functions are not valid as a React child" error in <Legend/> when a function is passed as the payload [#​3749](https://togithub.com/recharts/recharts/issues/3749) by [@​chris-mcdonald-dev](https://togithub.com/chris-mcdonald-dev) in [recharts/recharts#3750 - `Tooltip`: Fix tooltip position when container uses transform scale by [@​MateuszTrN](https://togithub.com/MateuszTrN) in [recharts/recharts#3748 - `Tooltip`: Tooltip does not include data from all charts when a separate dataset is passed to chart prop data and specified on Line/Area/etc prop data by [@​andrewangelle](https://togithub.com/andrewangelle) in [recharts/recharts#3733 fixes [recharts/recharts#3669 ##### Refactor Impossible to mention all of the great refactoring done this release thanks to [@​PavelVanecek](https://togithub.com/PavelVanecek) (33 PRs in one month!!) and others! Notable improvements include (non-breaking) type safety enhancements and source code file size reductions, and unit test improvements that will help reduce regressions. ##### Chore - Upgrade react-smooth to 2.0.5 - potentially fixes [recharts/recharts#1135 (edit: this was already fixed) - Add performance testing tool by [@​PavelVanecek](https://togithub.com/PavelVanecek) in [recharts/recharts#3829 - remove reduceCSSCalc by [@​HHongSeungWoo](https://togithub.com/HHongSeungWoo) in [recharts/recharts#3820 ##### Storybook - Add storybook-addon-performance by [@​PavelVanecek](https://togithub.com/PavelVanecek) in [recharts/recharts#3826 - many storybook improvements and fixes #### 🚀 New Contributors (!!) 🚀 - [@​d-gottlieb](https://togithub.com/d-gottlieb) made their first contribution in [recharts/recharts#3726 - [@​andrewangelle](https://togithub.com/andrewangelle) made their first contribution in [recharts/recharts#3733 - [@​wanisramdani](https://togithub.com/wanisramdani) made their first contribution in [recharts/recharts#3751 - [@​MateuszTrN](https://togithub.com/MateuszTrN) made their first contribution in [recharts/recharts#3748 - [@​chris-mcdonald-dev](https://togithub.com/chris-mcdonald-dev) made their first contribution in [recharts/recharts#3750 - [@​PavelVanecek](https://togithub.com/PavelVanecek) made their first contribution in [recharts/recharts#3759 - [@​simkesd](https://togithub.com/simkesd) made their first contribution in [recharts/recharts#3774 - [@​samtmorgan](https://togithub.com/samtmorgan) made their first contribution in [recharts/recharts#3778 - [@​Shashangbhagat](https://togithub.com/Shashangbhagat) made their first contribution in [recharts/recharts#3786 - [@​morozovkirill](https://togithub.com/morozovkirill) made their first contribution in [recharts/recharts#3746 - [@​branberry](https://togithub.com/branberry) made their first contribution in [recharts/recharts#3854 - [@​HHongSeungWoo](https://togithub.com/HHongSeungWoo) made their first contribution in [recharts/recharts#3820 **Full Changelog**: recharts/recharts@v2.8.0...v2.9.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 4pm every weekday" in timezone Europe/Paris, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/specfy/specfy). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40Ni4wIiwidXBkYXRlZEluVmVyIjoiMzcuNDYuMCIsInRhcmdldEJyYW5jaCI6ImNob3JlL3Jlbm92YXRlQmFzZUJyYW5jaCJ9--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
GMer78
pushed a commit
to GMer78/recharts-1
that referenced
this issue
Nov 24, 2023
<!--- Provide a general summary of your changes in the Title above --> fix(Line): onClick for activeDot is incorrectly typed ## Description <!--- Describe your changes in detail --> Have `activeDot` include the type for `DotProps` which fixes the incorrect types for the event handlers that are added in [line 2036 in generateCategoricalChart](https://github.com/recharts/recharts/blob/master/src/chart/generateCategoricalChart.tsx#L2036 ) which give a different payload to the callback mentioned in the issue. ## Related Issue <!--- This project only accepts pull requests related to open issues --> <!--- If suggesting a new feature or change, please discuss it in an issue first --> <!--- If fixing a bug, there should be an issue describing it with steps to reproduce --> <!--- Please link to the issue here: --> recharts#3922 ## Motivation and Context <!--- Why is this change required? What problem does it solve? --> Fixes incorrect types that introduced a breaking change ## How Has This Been Tested? Manually. It's a purely typescript issue. Screenshot below. <!--- Please describe in detail how you tested your changes. --> <!--- Include details of your testing environment, and the tests you ran to --> <!--- see how your change affects other areas of the code, etc. --> ## Screenshots (if appropriate): <img width="669" alt="Screenshot 2023-10-31 at 4 22 50 PM" src="https://github.com/recharts/recharts/assets/24420033/6eff15ec-e7cc-4056-92ed-f44ba3d007a2"> ## Types of changes <!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) ## Checklist: <!--- Go over all the following points, and put an `x` in all the boxes that apply. --> <!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> - [x] My code follows the code style of this project. - [ ] My change requires a change to the documentation. - [ ] I have updated the documentation accordingly. - [ ] I have added tests to cover my changes. - [ ] I have added a storybook story or extended an existing story to show my changes - [x] All new and existing tests passed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reproduction link
Steps to reproduce
Your onClick fuction on activeDot on Line is badly typed. If you use the type which is written in the onClick, then there's not a payload attribute on 2. position of the onClick.
https://pasteboard.co/DAYWcO1i0xa7.png
https://pasteboard.co/BYM0L0t2XByp.png
What is expected?
Have at least :any type on the second position for the payload.
What is actually happening?
We have a TS error
The text was updated successfully, but these errors were encountered: