-
Notifications
You must be signed in to change notification settings - Fork 91
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
feat: Upgrade to React 19 #3088
Conversation
"stylelint-config-css-modules": "^4.4.0", | ||
"stylelint-config-prettier": "^9.0.5", |
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.
ℹ️ all style-related rules have been deprecated as of stylelint 15 so this plugin is no longer necessary. Additionally, the older version has some conflicts with the other dependencies we need to update.
This is awesome! |
@brandonlenz are you the best person to review this PR? |
Can confirm this works with React 19. You can see our upgrade PR here: HHS/simpler-grants-gov#3515 . Would be great to get this pulled in 🚀 |
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.
LGTM, though it like the fonts changed a bit in the snapshot tests: https://happo.io/a/371/p/453/compare/931d2456d1b97c73a54aa598f8bfd7ba1fa50531/58f3089739ac5ee415c11cfbe179daad7a625971
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.
Excellent work @aligg. Looks like the diffs are gone now that you rebased. LGTM.
@crwallace - are you the current active maintainer (Just found your name in the docs). @sbolel and @acouch have thoroughly tested this PR and merging and releasing this upgrade will unblock other codebases to upgrade to react 19. Would you or someone you delegate have time to review this PR? Thank you! cc @brandonlenz @mdmower-csnw b/c I saw you reviewing PRs recently as well! (conscientious staffing could very well have shifted recently as well - if it helps, I know @sbolel would be willing to receive write access and serve as a reviewer, but not sure the protocol) |
Do we have confirmation that this is backwards compatible, just to ensure this isn't a breaking change (feat!) vs not? Otherwise, throwing my support in to get this through. |
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.
This is a great start. I think the other few updates are important to include here, and I'm interested in others' take on the form types.
src/components/forms/Radio/Radio.tsx
Outdated
| React.RefObject<HTMLInputElement> | ||
| null | ||
| undefined | ||
inputRef?: React.RefObject<HTMLInputElement> |
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.
🎯 suggestion: include all prop types for compatibility reasons
I agree with what's here as best practice, but I have concerns about this becoming a breaking change for someone.
What do you think of deprecating the other types before we remove them in a future release?
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.
The choice for this should be consistent across RangeInput
, Select
, TextInput
, and Textarea
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.
Updated via @jpandersen87
* LegacyInputRef and LegacyReactElement created * LegacyReactElement relies on user's React.Ref and React.LegacyRef in order to provide compatibility across types versions * unpin react and react-dom types resolution * updated all react-related dev dependencies * removed deprecated react-test-renderer and fixed affected unit tests
* use JSX.Element as functional component returns for React 16-19 compatibility * add react and react-dom types to peer dependencies so that library types properly reflect user's version
Optimize react 19 migration types to custom legacy types
* fix storybook (likely due to yarn v1) by updating to latest * update storybook-related plugin packages * update testing-library/react * remove unused storybook/addons * update happo dependencies * migrate pagination stories to be compatible
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.
This is great. There's a conflict or two on yarn.lock
that I can't resolve manually. Do you mind taking a look on your side?
We've updated eslint
& friends, so you'll see those conflicts in package.json, and we had to add "@typescript-eslint/utils": "8.26.1"
to the resolutions.
The other conflicts are from us removing inline eslint rules from a few files.
Let me know if you have any questions about this. I'm confident we can merge this once the conflicts are resolved.
src/types/legacyInputRef.ts
Outdated
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.
👏 praise: Thank you, this is exactly what we need!
@@ -121,17 +122,15 @@ | |||
"husky": "^9.0.10", | |||
"jsdom": "^24.0.0", | |||
"prettier": "^3.2.5", | |||
"react": "^18.2.0", | |||
"react-dom": "^18.2.0", | |||
"react-test-renderer": "^18.2.0", |
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.
👏 praise: Thank you for this cleanup!
@@ -2,20 +2,20 @@ | |||
|
|||
exports[`IdentifierGov component > renders consistently in Spanish 1`] = ` | |||
<section | |||
className="usa-identifier__section usa-identifier__section--usagov" |
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.
👏 praise: Good catch!
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.
For context: This is a natural side effect of updating the snapshot-based tests to use the actual DOM object from the render rather than the prior .toJSON()
-based way after removing react-test-renderer.
Just a heads up: there's an issue with the project's yarn v1 resolving dependencies improperly for the commit you reviewed (yarn v1 is known to be buggy and I have a PR to update it to the latest dependabot-supported version at #2878 ) which breaks storybook (and thus Happo). @aligg will be merging some updates soon that will update the storybook version which fixes the issue (updating storybook within this PR scope is the safer and more beneficial route since I have the PR for yarn specifically). |
Fix storybook via storybook dependency updates
Great, I'll double check |
Storybook should be fixed now. There is a future merge pending to fix the new small conflict that came up, if not handled via maintainer manually themselves before. |
Trusswork Main Branch Conflicts Fix
Fix happo config
This should be ready for another round of review. Want to shout-out @jpandersen87 for doing the heavy lifting to get this PR over the line while I was on PTO last week 👑 |
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.
LGTM!
Summary
Related PR
#3045
I know @sbolel has a PR up in progress - feel free to defer to that one. I had some time this morning to get this working though.
How To Test
Screenshots (optional)
N/A