2.x: Support react 19 #4542
2.x: Support react 19 #4542
Conversation
|
We are not planning on allowing 2.x to support react 19 because of the many reasons you document here + many other hidden things that will break because of the hacks recharts currently uses to render children and read props from react elements |
|
That was not our impression when testing. What else is broken in your opinion? |
|
Recharts relies on element cloning and reading information props react components before and after they are rendered. We are removing all of that in 3.x so we can make changes without breaking things accidentally. And follow current react practices so recharts is easier to contribute to. I guess I'd be fine accepting this PR to "support" react 19, but this branch will no longer get updates once 3.0 is released. Right now just trying to keep it to backwards compatible bug fixes. Ideally we'd be done with 3.0 before react 19 was released and we would never accept react 19 in 2.x peerDeps. Since we're not done yet I guess this is fine. Just beware that even though test coverage is high there are still a lot of cases not covered explicitly. The Component.props references are extremely fragile. I don't really want to put out fires if we support react 19 in 2.x and issues start to flood in when things break. |
|
I mean. If you tested this internally and it works then I suppose it can't be that bad. We could release that as But otherwise yeah, as @ckifer says, 3.0 is getting closer so I too would prefer if this patch landed in 3.x branch. That branch also has a lot better test coverage. |
|
Yep, my thoughts exactly |
|
Libraries should work on compat before a new major is released. Otherwise early adoption by app devs is not possible if they use any library. |
|
imo this new major is more important due to what the refactoring is accomplishing (removing old react hacks which help compat and maintainability). But either way, this is fine as part of 2.x Will review and get this merged/released once marked ready/tests are passing |
|
@ckifer We are testing recharts with react 19 beta in production, everything works fine except for recharts. Current behavior : (notice that the chart disappears)
Expected behavior:
We'll be happy to participate in testing this as early adopters and provide feedback on the pr. |
Are you using a build from this PR? |
@ckifer Ready for review. Will fix tests as CI reveals them but you've configured your repo to block CI from first-time contributors. You can adjust that to only block CI from people new on GitHub which would allow me to iterate. |
|
I allowed the CI run but be aware that 2.x branch Okay correction, 2.x is not as bad as I remember. Still ~2200 tests. 3.x has ~3200 tests. So more but not like game-changing amount more. |
|
I file a separate PR targetting 3.x. I won't test the 3.x version on React 19 though. |
|
Yeah fair enough. 3.x is not ready yet. |
|
Forward port for 3.x: #4561 |
Not yet. Still waiting for this pr to be merged. |
|
Pulled down the PR and running chromatic visual different tests https://www.chromatic.com/build?appId=63da8268a0da9970db6992aa&number=972 Storybook from build here https://63da8268a0da9970db6992aa-nwmhgzcfdw.chromatic.com No visual differences. Feel free to click around to see if anything interactive breaks This is not on React 19. I will update my local to react 19 and re-run the same test |
src/cartesian/ErrorBar.tsx
Outdated
| offset = 0, | ||
| layout = 'horizontal', | ||
| width = 5, |
There was a problem hiding this comment.
Visual regression https://www.chromatic.com/test?appId=63da8268a0da9970db6992aa&id=664cfc9eadd585053f5b885c
This will need to be converted to a class component unfortunately :/
There was a problem hiding this comment.
Actually... it works in 2.x but not 3.x. Not sure why, we haven't touched error bar much. @PavelVanecek ?
There was a problem hiding this comment.
I'll just make it a class component to be safe.
`defaultProps` are no longer supported on function components. To simplify migration, the components that do rely on the original defaultProps behavior were migrated to class components. Libraries also need to manually apply `defaultProps` if they expect that these are resolved by the JSX runtime. In React 19, defaultProps are only resolved by the time the component instances may access props not when a parent renders or reads from the JSX e.g. via `children.props`.
That'd be nice. We'll land the patch in prod start of next week too. Would appreciate an alpha release so that we don't need to retain such a large patch. |
|
I'm building from this PR and am not seeing my BarCharts nor Donut Charts. Should this PR have addressed all chart types? |
|
Try installing the beta version of react-is @ZipBrandon |
|
@ckifer Thanks for the advisement but unfortunately this still isn't depicting the charts. I made a small project at https://github.com/ZipBrandon/barchart-troubleshooting to get a sanity check on this and the patch. Here's my |
|
@ZipBrandon here is the storybook of this branch with this PR merged (and upgraded to r19) https://63da8268a0da9970db6992aa-xevoyhcfue.chromatic.com/?path=/story/examples-barchart--stacked Bars and Pies look fine to me. Don't have time to check your repo yet but i will before I release |
|
@ckifer I'm seeing 18.2 on that storybook link |
|
Ok, I'll try to deploy again when I can and double check the version. And also check your repo out |
|
@ZipBrandon still seems fine to me |
|
@ckifer Thanks for checking that out. Maybe there is something in my build process and patching that is amiss. Can you put out an alpha on npm and I can try that? |
|
@ZipBrandon It might be remix tbh. I'm starting a new blank vite project (no remix) and making sure that works that for a quick sanity check. Then will go for the alpha release |
|
@ZipBrandon @eps1lon this is broken. @ZipBrandon if I add <BarChart
width={500}
height={300}
data={data}
margin={{
top: 5,
right: 30,
left: 20,
bottom: 5,
}}
>
<CartesianGrid strokeDasharray="3 3" />
<XAxis dataKey="name" xAxisId={0}/>
<YAxis yAxisId={0}/>
<Tooltip shared={false} trigger="click" />
<Legend />
<Bar dataKey="pv" fill="#8884d8" xAxisId={0} yAxisId={0} minPointSize={10}/>
<Bar dataKey="uv" fill="#82ca9d" xAxisId={0}yAxisId={0} minPointSize={10} />
</BarChart> |
|
I also don't have the time to fix it so if anyone wants to dive in and make another PR feel free |
|
Breaks about 80 unit tests when ran with React 19. Good thing is that R18 and below is fine... but we'll need all these to pass to release this |
Is there a branch I can checkout? |
|
It's not ideal because you need to upgrade the recharts repo to R19 before things break and you need to upgrade other things (storybook, testing library) before anything works. So it's a bit of a mess. Not sure why storybook doesn't fail but it must be using its own React version or something. I was using yalc to local publish and test in a fresh repo. On mobile rn but I'll push the branch up shortly |
|
Alpha release - https://www.npmjs.com/package/recharts/v/2.13.0-alpha.0 use this to determine what is breaking/what isn't. I just don't have bandwidth to fix it myself |
|
Green test suite: #4573 |
#4542 was incomplete with regards to `defaultProps`. This PR ensures the test suite is green in React 19 as well as well as fixing all new warnings introduced in React 19. --------- Co-authored-by: Coltin Kifer <ckifer@amazon.com>
|
@eps1lon Thank you for this fix regarding the defaultProps error message from React. I've tested the latest alpha release You can fix it by changing to |
Best reviewed without whitespace changes
Description
defaultPropsare no longer supported on function components. To simplify migration, the components that do rely on the original defaultProps behavior were migrated to class components.Libraries also need to manually apply
defaultPropsif they expect that these are resolved by the JSX runtime. In React 19, defaultProps are only resolved by the time the component instances may access props not when a parent renders or reads from the JSX e.g. viachildren.props.Related Issue
Motivation and Context
Adds support for React 19 to
recharts@2.xwithout having to patch published files.Complete React 19 support would require #4541 but that's a breaking change. Consumers of 2.x will have to use
resolutionsoroverrideswith their package managers to ensure the correctreact-isversion is usedHow Has This Been Tested?
We've been using
rechartsin various (internal) surfaces at Vercel. We've applied this patch to our codebase and charts have been working when they were previously broken.The following components we use explicitly:
ResponsiveContainerLineChartCartesianGridXAxisTooltipYAxisLegendLinePieChartCellPieSectorWe also use
SparkAreaChartfrom@tremor/reactwhich usesrechartsto implement that chart.Screenshots (if appropriate):
Types of changes
Checklist: