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

- Update all packages #1

Merged

Conversation

pieterbergwerff
Copy link

  • Update to React 19

- import JSX from types react
- useCallback for functions in render
- Update to React 19
@pieterbergwerff
Copy link
Author

We have upgraded to React 19, fixed TypeScript errors, and utilized useCallback. The build is working again, but we haven’t performed thorough testing. If everything looks good, perhaps you could merge this PR?

Best regards,
Pieter and Marco

@pieterbergwerff
Copy link
Author

Hi @ChrisCrossCrash, any update on this?

@ChrisCrossCrash
Copy link
Owner

@pieterbergwerff thanks for your help! I'll take a look at it when I'm home from vacation sometime next week.

The previous "- Update all packages" commit added some extra indentation to `package.json` and `package-lock.json`. This makes it hard to use `git blame` to see when changes were made to `package.json`. To fix this, I will:

1. Revert the changes to `package.json` change with this commit, and just reformat `package-lock.json`. `package.lock` is out of sync with `package.json` for this commit.
2. Re-introduce the changes to `package.json` without the extra formatting change. `package.json` and `package-lock.json` should be in sync and formatted after this step.
3. Ignore the original "- Update all packages" and the revert commit in `.git-blame-ignore-revs` so that we can still get useful information from `git blame` in `package.json`

This partially reverts commit 9b697ce.
@ChrisCrossCrash
Copy link
Owner

@pieterbergwerff Thanks so much for your work on this. You've fixed the type errors where I could not! And without using any or as!

I noticed that your - Update all packages commit changes the formatting a bit for package.json and package-lock.json. That makes it difficult to use git blame on package.json to see when changes were made, since the latest change for all the lines would be the change in formatting. To fix this, I just reverted that commit, and re-introduced those new package versions in a new commit. Then, I added the commit hashes for the commits with formatting changes to .git-blame-ignore-revs so that those changes are ignored for the purposes of git blame. For example, git blame shows that the latest change to this line was 8 years ago:

image

When I made this fork, I was considering maintaining a fork of the original react-chartjs-2 repository so that others could continue to use it, but I'm not sure if I'm interested in that long term-commitment.

One good thing about this fork that we've been working on is that it shares all the same git history as react-chartjs-2, so if the maintainers of react-chartjs-2 are interested, they can merge it into their project. Otherwise, it's just a great starting point for anybody interested in maintaining a fork without a lot of outdated tooling.

In any case, I think it would be helpful to try to get the old tests working. Maybe also the StoryBook examples if it's not too difficult. Also, we have React 19 listed as a peer dependency, but not older versions like 18. Perhaps we could add those older versions as peer dependencies.

I still haven't tested out these changes in an actual project, and obviously this new version doesn't have any testing. However, it builds, so that's good enough for me to merge at the moment. Thanks!

@ChrisCrossCrash ChrisCrossCrash merged commit b5fc82f into ChrisCrossCrash:main Dec 23, 2024
@ChrisCrossCrash
Copy link
Owner

@pieterbergwerff In case you haven't seen it, a Pull request has been made on the react-chartjs-2 repository that adds support for React 19. It still hasn't been merged, and I doubt it will because it seems that react-chartjs-2 is abandoned. However, it might be useful to see where it goes because I'm sure there's a lot of interest in a React library for Chart.js. Perhaps you could take a look at the PR and see if any of the changes that you've made on this fork can improve on that PR.

@pieterbergwerff
Copy link
Author

Hi Chris,

Thanks for reviewing! Great that you took a look at the git blame issue. @Marco-Daniel and I were in a bit of a rush trying to fix things, so we didn’t really pay attention to that part. :) And yeah, avoiding the any / as took a bit of effort to figure out!

I did notice that you made a recommendation on the original PR for our fixes. Awesome, hopefully, they can make use of that.

A React library for Chart.js would definitely be a great addition. If you ever plan to set something like that up, we’d be happy to contribute in any way we can.

Have a great day!
Best regards,
Pieter

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

Successfully merging this pull request may close these issues.

2 participants