-
Notifications
You must be signed in to change notification settings - Fork 352
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
React 18 #1064
Conversation
Size Change: +285 B (+0.03%) Total Size: 854 kB
ℹ️ View Unchanged
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1064 +/- ##
==========================================
+ Coverage 69.97% 70.30% +0.33%
==========================================
Files 505 508 +3
Lines 105660 105703 +43
Branches 7686 11450 +3764
==========================================
+ Hits 73933 74317 +384
+ Misses 31545 31386 -159
+ Partials 182 0 -182
... and 148 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 (a973c6e) 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.
packages/simple-markdown/src/__tests__/simple-markdown.test.tsx
Outdated
Show resolved
Hide resolved
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
@jeremywiebe React 18 is deployed in webapp, so I think this PR is now unblocked! |
4ea5a98
to
43fedbe
Compare
@@ -152,8 +148,5 @@ | |||
"cypress:ci": "cross-env BABEL_COVERAGE=1 cypress run --component --env CYPRESS_COVERAGE=1", | |||
"format": "prettier --write .", | |||
"typecheck": "tsc" | |||
}, | |||
"dependencies": { | |||
"tiny-invariant": "^1.3.1" |
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.
Now that we only have dependencies
, this has moved up into the main body of dependencies above.
fb03485
to
22446e1
Compare
22446e1
to
6806313
Compare
## Summary: This PR is the part of a chain of PRs upgrading Perseus to React 18. 1. This PR --> #1400 2. #1401 3. #1402 4. #1403 I thought I had the React 18 (#1064) upgrade all ready (and it was even approved), but when I returned to that PR, I noticed linter and test failures. This lead me to discover that I need to upgrade testing-library. This then breaks a bunch of tests (which I'll fix in future PRs). Issue: LEMS-1802 ## Test plan: This PR will introduce many test failures. That's ok, they'll be fixed in follow-up PRs so that hopefully each step is relatively easy to review. Author: jeremywiebe Reviewers: jeresig, #perseus, jandrade, somewhatabstract Required Reviewers: Approved By: jeresig Checks: ⏭️ Upload Coverage, ✅ gerald, ⏭️ Publish npm snapshot, ❌ Jest Coverage (ubuntu-latest, 20.x), ❌ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ gerald, ⏭️ Upload Coverage, ⏭️ Publish npm snapshot, ❌ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ❌ Jest Coverage (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ⏭️ Upload Coverage, ⏭️ Publish npm snapshot, ❌ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ❌ Jest Coverage (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1400
## Summary: This PR is the part of a chain of PRs upgrading Perseus to React 18. 1. #1400 2. This PR --> #1401 3. #1402 4. #1403 It appears that React 18 and RTL are more sensitive to not having mutating calls wrapped in `act()`. The changes in this PR are all simple changes that wrap function calls that cause DOM changes in `act()` calls. There are a few special cases that are failing yet, but this PR cleans up the straightforward ones. Issue: LEMS-1802 ## Test plan: `yarn test` should have many fewer test failures than PR #1400 Upcoming PRs will address the remaining test failures. ### Before ```sh $ yarn test ... Test Suites: 20 failed, 1 skipped, 168 passed, 188 of 189 total Tests: 104 failed, 14 skipped, 2469 passed, 2587 total Snapshots: 22 failed, 1 file obsolete, 137 passed, 159 total Time: 25.267 s, estimated 26 s Ran all test suites. error Command failed with exit code 1. ``` ### After ```sh $ yarn test ... Test Suites: 4 failed, 1 skipped, 184 passed, 188 of 189 total Tests: 9 failed, 14 skipped, 2564 passed, 2587 total Snapshots: 4 failed, 1 file obsolete, 155 passed, 159 total Time: 24.056 s, estimated 25 s Ran all test suites. error Command failed with exit code 1. ``` Author: jeremywiebe Reviewers: jeremywiebe, jeresig, #perseus, somewhatabstract Required Reviewers: Approved By: jeresig Checks: ⏭️ Upload Coverage, ✅ gerald, ⏭️ Publish npm snapshot, ❌ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ❌ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald, ⏭️ Upload Coverage, ⏭️ Publish npm snapshot, ❌ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ❌ Jest Coverage (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1401
## Summary: This PR is the part of a chain of PRs upgrading Perseus to React 18. 1. #1400 2. #1401 3. This PR --> #1402 4. #1403 This PR fixes a few instances where there exists an RTL API for tests instead of making direct DOM object calls. RTL recommends this approach and this fixes a few more tests. Issue: LEMS-1802 ## Test plan: `yarn test` more tests pass ### Before ```sh $ yarn test ... Test Suites: 4 failed, 1 skipped, 184 passed, 188 of 189 total Tests: 9 failed, 14 skipped, 2564 passed, 2587 total Snapshots: 4 failed, 1 file obsolete, 155 passed, 159 total Time: 24.056 s, estimated 25 s Ran all test suites. error Command failed with exit code 1. ``` ### After ```sh $ yarn test ... Test Suites: 3 failed, 1 skipped, 185 passed, 188 of 189 total Tests: 6 failed, 14 skipped, 2567 passed, 2587 total Snapshots: 4 failed, 1 file obsolete, 155 passed, 159 total Time: 20.517 s, estimated 24 s Ran all test suites. error Command failed with exit code 1. ``` Close! :) Author: jeremywiebe Reviewers: jeremywiebe, jeresig, #perseus, somewhatabstract Required Reviewers: Approved By: jeresig Checks: ⏭️ Upload Coverage, ✅ gerald, ⏭️ Publish npm snapshot, ❌ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ❌ Jest Coverage (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald, ⏭️ Upload Coverage, ⏭️ Publish npm snapshot, ❌ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ❌ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1402
## Summary: This PR is the part of a chain of PRs upgrading Perseus to React 18. 1. #1400 2. #1401 3. #1402 4. This PR --> #1403 A few tests needed larger changes than simply wrapping the actions in `act()` (see #1402). This PR covers the remaining tests that were failing for various reasons. In some cases the tests were largely re-written (sometimes for better test clarity while re-working them to pass). In the case of the Zoomable tests, we were waiting for the text to be updated in the element, but the styling changes had not yet settled as the updates caused multiple `setState()` calls to be made which all needed to be called and then resolved by a subsequent render (thus the new `waitForStyle()` helper). In the Graded Group test update, I'm not sure how it worked as written. In the test that was fixed, we were testing mobile and in that case the "Check" button reads "Try again" on the second and subsequent attempts. In the Group tests, I needed to wrap some actions in `act()` as usual, but I also took the opportunity to change the element finding logic away from the `getAll...()` functions to depend more on the aria labels. This makes these tests read more clearly (instead of accessing item at index `[0]` we get the textbox with the string `"value rounded to the nearest ten"`). Lastly, there was one outdated snapshot due to a test file being renamed. Issue: LEMS-1802 ## Test plan: `yarn test` all tests now pass! ### Before ```sh $ yarn test ... Test Suites: 3 failed, 1 skipped, 185 passed, 188 of 189 total Tests: 6 failed, 14 skipped, 2567 passed, 2587 total Snapshots: 4 failed, 1 file obsolete, 155 passed, 159 total Time: 22.175 s, estimated 27 s Ran all test suites. ``` ### After ```sh $ yarn test ... Test Suites: 1 skipped, 188 passed, 188 of 189 total Tests: 14 skipped, 2573 passed, 2587 total Snapshots: 159 passed, 159 total Time: 26.765 s Ran all test suites. ``` Author: jeremywiebe Reviewers: jeremywiebe, jeresig, #perseus, somewhatabstract Required Reviewers: Approved By: jeresig Checks: ✅ codecov/patch, ✅ codecov/project, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald, ✅ gerald, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), 🚫 Upload Coverage, ✅ gerald, ⏭️ Publish npm snapshot, 🚫 Cypress (ubuntu-latest, 20.x), 🚫 Jest Coverage (ubuntu-latest, 20.x), 🚫 Check builds for changes in size (ubuntu-latest, 20.x), 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1403
…ect the test anyways
## Summary: This PR represents what should be the final set of fixes for React 18. It includes the following changes that quiet any remaining console errors as well as test failures: * caching React "roots" created by `ReactDOMClient.createRoot()` in `tex.ts`. React warns when you call `ReactDOMClient.createRoot()` multiple times on the same DOM element. So this PR caches elements that have been processed in a `WeakMap`. The `WeakMap` will avoid a "memory leak" by "over retention" of DOM nodes. * Fixes a "uniqueId" warning coming out of Wonder Blocks Popover. It turns out that even though the WB `Popover` constrains it's `content` property to only be `PopoverContents` or `PopoverContentsCore`, Typescript happily allows any JSX to be assigned to it. So I've fixes `packages/math-input/src/components/keypad/__tests__/keypad-v2-mathquill.test.tsx` to wrap it's `contents` in a `PopoverContentsCore`. See [this](https://khanacademy.slack.com/archives/C4PE1QM5Y/p1721077735704319) Slack thread. * Introduction of `waitForDeferredRenders` which uses testing library's `waitFor()` to wait for the next React render. We have a pattern in our old `reactRender()` that causes testing library to complain that a DOM update was not wrapped in `act()`. This function silences this warning by essentially wrapping the effects of `reactRender()` inside of an `act()` by way of `waitFor()`. * Other actions that caused render/updates wrapped in `waitFor()` also. Issue: LEMS-1802 ## Test plan: `yarn test` - they should all pass and there should be no console errors logged Author: jeremywiebe Reviewers: jeremywiebe, benchristel, jeresig, somewhatabstract, handeyeco Required Reviewers: Approved By: benchristel, jeresig Checks: ❌ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1418
## Summary: At the end of the React 18 work I noticed two failing tests in the Interactive Graph suite. I found that its the two tests that had to be customized to pass related to `startCoords`. If I remove this customization, all tests pass. My hunch is that with React 16 and the older `@testing-library` package, there was some async DOM updates/events that didn't flush all the way leaving the graph in a non-final state after keypresses, thus failing the tests. Issue: LEMS-2186 ## Test plan: `yarn test` ✅✨🎉😍 Author: jeremywiebe Reviewers: jeremywiebe, benchristel, nishasy Required Reviewers: Approved By: benchristel, nishasy Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1423
## Summary: @handeyeco [noticed](#1064 (comment)) that the bundle size for `math-input` grew considerably in the React 18 base branch (#1064). <img width="633" alt="image" src="https://github.com/user-attachments/assets/c7c8a6c2-d3d2-4a88-8c38-86e728a627c1"> It turns out that `react` and `react-dom` were no longer listed as `peerDependencies` in its `package.json`. I found the same regression in `@khanacademy/perseus`'s `package.json` file. We use a Rollup plugin ([`rollup-plugin-auto-external`](https://github.com/stevenbenisek/rollup-plugin-auto-external)) to automatically mark peer dependencies as external to avoid bundling them into our packages. When `react` and `react-dom` were excluded from `peerDependencies` they got bundled in and that was why the bundle size exploded. Issue: LEMS-1802 ## Test plan: I ran `yarn build` locally and checked the generated `index.js` file in each `dist` folder. They no longer contain the React code. I will also re-check the bundle size comment in the PR once it updates. Author: jeremywiebe Reviewers: handeyeco Required Reviewers: Approved By: handeyeco Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1425
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/math-input@20.0.0 ### Major Changes - [#1064](#1064) [`c6a5cbe13`](c6a5cbe) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - React 18 ### Patch Changes - [#1421](#1421) [`9a3bce37f`](9a3bce3) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Enhance types in tests using @testing-library/user-event - [#1064](#1064) [`c6a5cbe13`](c6a5cbe) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Fix: prevent `react` and `react-dom` from being bundled ## @khanacademy/perseus@26.0.0 ### Major Changes - [#1064](#1064) [`c6a5cbe13`](c6a5cbe) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - React 18 ### Minor Changes - [#1426](#1426) [`4b6fc712e`](4b6fc71) Thanks [@benchristel](https://github.com/benchristel)! - Tweak the animation of the protractor rotation handle. It's now slightly slower and the magnification is less extreme. ### Patch Changes - [#1421](#1421) [`9a3bce37f`](9a3bce3) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Enhance types in tests using @testing-library/user-event - [#1064](#1064) [`c6a5cbe13`](c6a5cbe) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Fix: prevent `react` and `react-dom` from being bundled - Updated dependencies \[[`9a3bce37f`](9a3bce3), [`c6a5cbe13`](c6a5cbe), [`c6a5cbe13`](c6a5cbe), [`c6a5cbe13`](c6a5cbe)]: - @khanacademy/math-input@20.0.0 - @khanacademy/simple-markdown@0.13.0 - @khanacademy/pure-markdown@0.3.7 ## @khanacademy/perseus-editor@9.0.0 ### Major Changes - [#1064](#1064) [`c6a5cbe13`](c6a5cbe) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - React 18 ### Patch Changes - [#1421](#1421) [`9a3bce37f`](9a3bce3) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Enhance types in tests using @testing-library/user-event - Updated dependencies \[[`4b6fc712e`](4b6fc71), [`9a3bce37f`](9a3bce3), [`c6a5cbe13`](c6a5cbe), [`c6a5cbe13`](c6a5cbe)]: - @khanacademy/perseus@26.0.0 - @khanacademy/math-input@20.0.0 ## @khanacademy/simple-markdown@0.13.0 ### Minor Changes - [#1064](#1064) [`c6a5cbe13`](c6a5cbe) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - React 18 - [#1064](#1064) [`c6a5cbe13`](c6a5cbe) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - React 18 ## @khanacademy/pure-markdown@0.3.7 ### Patch Changes - Updated dependencies \[[`c6a5cbe13`](c6a5cbe), [`c6a5cbe13`](c6a5cbe)]: - @khanacademy/simple-markdown@0.13.0 ## @khanacademy/perseus-dev-ui@2.0.0 ### Major Changes - [#1064](#1064) [`c6a5cbe13`](c6a5cbe) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - React 18 ### Patch Changes - Updated dependencies \[[`9a3bce37f`](9a3bce3), [`c6a5cbe13`](c6a5cbe), [`c6a5cbe13`](c6a5cbe), [`c6a5cbe13`](c6a5cbe)]: - @khanacademy/math-input@20.0.0 - @khanacademy/simple-markdown@0.13.0 - @khanacademy/pure-markdown@0.3.7 Author: khan-actions-bot Reviewers: jeremywiebe Required Reviewers: Approved By: jeremywiebe Checks: ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1433
Summary:
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.We're now ready!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).devDependencies
in the rootpackage.json
to bedependencies
(import/no-extraneous-dependencies
was complaining about@types/react
being indevDependencies
). Since the root package.json is just a host for the workspace and doesn't build any production code, the distinction betweendependencies
anddevDependencies
is irrelevant.react-docgen
instead of the defaultreact-docgen-typescript
(react-docgen
is the default in Storybook 8 so we can revert this once we upgrade). See this comment that pointed to the fix.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()