-
Notifications
You must be signed in to change notification settings - Fork 348
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
React 18 #1064
base: main
Are you sure you want to change the base?
React 18 #1064
Conversation
Size Change: -132 B (0%) Total Size: 833 kB
ℹ️ View Unchanged
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1064 +/- ##
==========================================
+ Coverage 68.64% 69.73% +1.09%
==========================================
Files 470 474 +4
Lines 100733 100795 +62
Branches 7146 10783 +3637
==========================================
+ Hits 69148 70291 +1143
+ Misses 31406 30504 -902
+ Partials 179 0 -179
... and 146 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
4fd1d18
to
53f2e32
Compare
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (dc8cc5e) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1064 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1064 |
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.
Glad you were able to get this working! That's too bad about portals not working - I think if you were able to get portals working then we could ship this change (even with webapp still on v16), I think that's the only actual API incompatibility that I'm aware of! But not worries if not, we can stagger this out, instead.
@@ -91,7 +90,7 @@ var htmlFromMarkdown = function (source) { | |||
* @param {string} html | |||
*/ | |||
var assertParsesToReact = function (source: string, html: string) { | |||
var actualHtml = htmlFromReactMarkdown(source); | |||
var actualHtml = htmlFromReactMarkdown(source); // ? |
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.
var actualHtml = htmlFromReactMarkdown(source); // ? | |
var actualHtml = htmlFromReactMarkdown(source); |
31b1f99
to
dc8cc5e
Compare
@jeremywiebe is this PR still needed? |
@benchristel Yes, but it's waiting on FE Infra work to move WB and Webapp to React 18 before I feel comfortable landing it. Ultimately, webapp (or any host app) defines what version of React Perseus uses so I think we could get into trouble if we move to React 18 and accidentally use some API/feature that is only available in React 18+. |
dc8cc5e
to
19f5d96
Compare
Summary:
NOT READY TO MERGE I've got the changes ready, but we need to wait on WB and webapp to be ready before I land this PR and do a release.
This PR moves this repo to use React 18. There were a few changes that needed to be made to be fully compatible with React 18.
ReactDOM.TestUtils
renderer to @testing-library.ErrorBoundary
tests as React'scomponentDidCatch
provides a different stack trace format (which is irrelevat to our code).ReactDOM.render()
toReactDOMClient.createRoot()
(we hope to move these to use Portals in the future, but I couldn't get them working).Issue: LEMS-1802
Test plan:
yarn test
✅yarn tsc
✅yarn storybook
✅ - I sampled many of the stories. I also checked the stories that were affected by the move fromReactDOM.render()
toReactDOMClient.createRoot()