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

Migrating code from react-chart-js to chartjs #6555

Open
ramyaragupathy opened this issue Sep 5, 2024 · 3 comments
Open

Migrating code from react-chart-js to chartjs #6555

ramyaragupathy opened this issue Sep 5, 2024 · 3 comments

Comments

@ramyaragupathy
Copy link
Member

While working on developing partner pages, @VinayakRugvedi flagged the following:

  • we are dependent on both chartjs and react-chartjs libraries for frontend components
  • chartjs library community is more active
  • react-chartjs library has not been in active development in the last two years

Going ahead we should stick to chartjs and move the existing react-chartjs components to chartjs as well.

cc @royallsilwallz @emi420 @manjitapandey

@htulipe
Copy link

htulipe commented Oct 10, 2024

Hi, I would like to contribute to the project and saw this issue that seemed like a good first task to tackle. I registered to attend the next team meetup to introduce myself and maybe talk a bit more about this issue.

In the meantime, I took the liberty to dig a little bit more into the matter here. I saw that react-chartjs-2 is a wrapper around chartjs so we can't replace the former by the latter unless we want to code our own chartjs react wrapper. That's doable but I don't see the benefit of this other than removing a dependency (which is always nice). If the wrapping library does a good job, even though it's old, I would stick to it.

Anyhow, I look forward to discuss the matter with you guys.

Cheers

@royallsilwallz
Copy link
Contributor

Hey @htulipe, we're glad that you digged into this issue.

The point you put out is valid. Redoing the charts in chartjs isn't much of a benefit if everything is working fine.
The only time it would create an issue is when we (in future) upgrade our main packages such as react or node version to higher ones. Then, the unmaintained react-chartjs-2 library might not support them and hence we would have to migrate the code to chartjs anyway.

So, you can start working on this and see how it goes. I'll be able to help you if you need any.

Thanks for your interest!

@htulipe
Copy link

htulipe commented Oct 24, 2024

Hi @royallsilwallz thanks for your reply. I see your point, I dug a little bit deeper as a consequence, here's what I found.

react-chartjs-2 has peer dependencies only on react and chartjs, no worries for node version then.

For chartjs, the peer dependency semver will allow any 4.x versions of the lib. Looking at the library and its repo, it does not look like there's a plan for a v5, v4 is the only version they have so far. Should they respect semver properly, chartjs wont be an issue here for the forseable future.

For react though, the semver won't allow v19 of React which had an RC release in April. There is no due date for the final version but we can expect it anytime soon. At this point we will have an issue if we update this project to React 19.

Where to go from here? First of all, I looked at the BC changes of React 19 and the codebase of react-chartjs-2: luckily none of the BC changes affect the codebase. So if React 19 final does not had other BC changes, updating the semver in react-chartsjs-2 should suffice. Maybe the author, even though inactive right now, will make a new release: I'm pretty sure we won't be the only one interested in having the library accepts React 19.

If that does not happen, we can integrate the codebase of react-chartjs-2 in this repo: we won't have to think about it anymore then. That's doable but I think it a bit early to do that, especially since the library is written in Typscript while our codebase here is not: we would have to strip away the typing which seems to me like a bad idea. Moreover I saw a PR that add TS support, so maybe we should wait for this PR to be merged before incorporating react-chartjs-2 code.

As a recap, I would say there is no rush in ditching react-chartjs-2 right now. Let me known.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants