-
-
Notifications
You must be signed in to change notification settings - Fork 445
Bugfix/#602 - Add support for React 19 #665
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
Conversation
|
I am not sure how the CHANGELOG update is handled properly. I assume the PR gets stashed before getting merged so i don't know the merge commit ID to use in the CHANGELOG. So a few questions:
|
I tested the implementation also with React 18 in another repository. AFAIK it's not possible to add another app to this monorepo using React 18 next to the current implementation using React 19. |
Just read the provided documentation - its just there in provided comment that was made by the bot... Click here to learn what changesets are, and how to add one. |
@juliencrn Could this be expedited please? |
Thanks @ondrej-langr. For someone who doesn't know changesets at all like myself https://github.com/juliencrn/usehooks-ts/blob/master/.github/CONTRIBUTING.md doesn't provide much information how to handle it properly in this repository. #665 (comment) complained about a missing changeset but it didn't help me out but made me feel uncertain. So i asked :) According to https://www.npmjs.com/package/@changesets/cli the main purpose of changesets is to
and
and
. So i reverted the version updates i previously added manually and added a changeset via |
@iwan-uschka Indeed! Great job! Just a quick sidenote about the changeset message - remove that Changesets are unique in a way that you dont have to adhere to any maximum characters as far as i know so you can add bigger explanations of your updates. One pull request can have multiple changesets even! (but in that case its better to just make more PRs) |
Thx again @ondrej-langr.
I did exactly that but still got confused because you can find commits like b14db5b using that notation i used or 7ba7e3a with no changeset at all. Anyway, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the interest of speed and getting this reviewed as quickly as possible, it might be worth moving the changes related to www
and the eslint-config-custom
packages into another PR—maybe even two separate PRs. They can reference this branch as the base, which means that once this branch merges they will be able to fast follow, but they shouldn't hold up the merge IMO.
Edit: Thanks @iwan-uschka for the work on this. It looks great!
The last commit is from nearly a year ago. Perhaps this project is dead? |
Two options: i am too stupid or the UI is bullshit. I can see, that @lachieh took a look and reviewed this PR. Thanks for that (if that's the case 😸). But i cannot see any approval or change request. So i don't know what to do here. I have to apologize, i don't have the time right now to split things up into multiple PRs. I don't even know if the "complexity" of this PR is the crucial part here, like @musjj said. I am new to open source on Github, i don' know who is capable of merging this PR or even approving it. Is it the owner only (i hope not) or is it the bigger group of contributors, is it anybody? Is it ok to ping the owner of this repo? According to https://github.com/juliencrn he is active but not in this repository as it seems. |
Thanks for all the work you've put into this PR, @iwan-uschka. I'm not a maintainer, just another eager user who’s been subscribed to this issue. From what I can see, all the most recently merged PRs were handled by Julien, so I'd guess he's the only one with merge permissions. It might feel a bit bold, but I've had some luck in the past reaching out to the owners of stale open source projects via email. If @juliencrn doesn't respond to this ping, you could try using the email address listed on his personal site: https://www.juliencaron.com/contact. |
Maybe we have to create a separate issue to discuss the future (or at least the present 😄 ) of this project with juliencrn. |
Hi @iwan-uschka, thanks for all but I've just merged #651 to support React 19, this one did too many things at once. |
First: thanks @juliencrn for breaking the silence :) The problem with the latest release is, that it changes nothing in terms of usability. This commit doesn't fix any compatibility issue between this library and React 19. I might agree that my PR does a lot things at once but i think these changes are necessary. I expect that a small PR like this can be reviewed or at least discussed and not just closed out of the blue after 6 weeks of silence. |
Thanks, @iwan-uschka. I know it can be frustrating to get a PR closed when you've used your own time and put the work in to help fix problems for the wider community. If you're still up for contributing and you want to resolve #602, I would suggest that you submit a few new PRs that break this one up. Here's how I would break up the changes as PRs as changesets:
It for sure seems like a lot of work, but splitting it up like this means that each one of those PRs can be reviewed and merged entirely independent from each other. You mentioned that you're new to OSS, so I'll link this article around best practices—please don't take offense if you've heard this all before! Even writing excellent PRs means that you're creating more work for someone else, so the least you can do is make that work as easy to do as possible. If you'd like some assistance with any of this, let me know. I'm happy to help! |
Thank you @lachieh for the support, time and effort. I created new PRs to address the issues separately: I ran out of time so there is no extra PR for updating ps: i didn't want to sound too disappointed in my previous post, just felt a little baffled |
All good, @iwan-uschka! I'm not a maintainer here, I just have poke around in a few OSS projects so try to help when I can. @juliencrn, both of these PRs look good! |
Thank you @iwan-uschka for putting in the work. |
This PR adds support for React 19 by fixing TS issues regarding
useRef
(see https://react.dev/blog/2024/04/25/react-19-upgrade-guide#useref-requires-argument).To be able to use React 19 throughout the project i had to update some other dependencies as well.
To be able to use the TypeScript version of this project in VS Code i removed all
typescript
dependencies from allpackage.json
and added one singletypescript
dependency to the rootpackage.json
. I hope this is ok.Before this change, they were two versions of
typescript
:5.4.3
and5.3.3
. Now we have only one version oftypescript
:5.4.3
. To avoid running into warnings likewhen calling
pnpm lint
i had to update@typescript-eslint
to7.18.0
. This required some fixes to add to the codebase.