Upgrade React packages to v18 #45235
Conversation
|
Size Change: +1.97 kB (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
jsnajdr
force-pushed
the
upgrade/react-18-pure
branch
from
d8782ab
to
272a87b
Compare
jsnajdr
force-pushed
the
upgrade/react-18-pure
branch
from
272a87b
to
01fa45e
Compare
jsnajdr
force-pushed
the
upgrade/react-18-pure
branch
3 times, most recently
from
e77309b
to
13d723b
Compare
| @@ -47,8 +47,8 @@ | |||
| "change-case": "^4.1.2" | |||
| }, | |||
| "peerDependencies": { | |||
| "react": "^17.0.0", | |||
| "react-dom": "^17.0.0" | |||
| "react": "^18.0.0", | |||
There was a problem hiding this comment.
In order for the rest of the ecosystem to update smoothly and at their own pace, should we support both 17 and 18 as peer deps in most packages? (As long as our code is still compatible with 17, that is)
There was a problem hiding this comment.
I'm not sure if our code will be compatible, that's the problem.
For example, I suspect that we'll need to update the useSelect hook to use useSyncExternalStore from React 18, otherwise it will not work 100% reliably with v18. Then the @wordpress/data library has a non-negotiable dependency on React 18, and that transitively affects all other libraries...
Let's keep the v17 compat requirement in mind, and see what we can do. Libraries like react-redux use a compat shim for useSyncExternalStore, and that allows them to support older versions back to React 16.
|
By far the biggest issue with the React 18 migration are unit tests firing warnings from These warnings were quite common in React 17, too, but now React 18 seems to be much more sensitive about them, probably because of the concurrent mode, and tests often fail. This In most affected libraries, someone filed a GitHub issue and most of the time the issue is closed after some time because most participants are confused and nobody is able to pinpoint the exact cause:
The Tests are failing on Most typical case is fetching some data with const fetchPromise = new Promise( ( resolve ) => {
apiFetch.mockImplementation( async ( { path } ) => {
resolve();
return response;
} );
} );
await act( () => fetchPromise );This works but it requires access to the library internals (or mocking the internals). And it's not reliable: it relies on the fact that the There are libraries where the async update is invisible: the function useFloating() {
const [ data, setData ] = useState( { x: null, y: null } );
useEffect( () => {
computeDomPosition().then( ( data ) => setData( data ) );
}, [] );
return data;
}I.e., it will calculate the DOM position of the rendered position and updates state in a But then a synchronous test that renders this component will miss the update: render( <FloatingComponent title="floating title" /> );
expect( screen.getByTitle( 'floating title' ) ).toBeInTheDocument();It will work (probably) because the assertion doesn't depend on positioning, but the state update will run a bit later, possibly during another test, and will trigger the "not wrapped in act()" warning. A common solution (recommended even by the library docs) is to wait for the extra tick: render( <FloatingComponent title="floating title" /> );
await act( async () => {} );
expect( screen.getByTitle( 'floating title' ) ).toBeInTheDocument();But this is next to impossible to do reliably: every time the tested UI does call Another library that performs the The Right Solution And our tests need to wait for that update to happen. For example, for await waitFor( () => hasFinalPosition( screen.getByRole( 'popover' ) ) );where |
jsnajdr
force-pushed
the
upgrade/react-18-pure
branch
2 times, most recently
from
3a1de78
to
fef2847
Compare
jsnajdr
force-pushed
the
upgrade/react-18-pure
branch
from
fef2847
to
3e83005
Compare
I just implemented this in #45587 and it really works 🎉 |
|
There's a failing test for the I've created #45736 to address it separately. |
| @@ -3,6 +3,7 @@ | |||
| */ | |||
| import { render, screen } from '@testing-library/react'; | |||
| import userEvent from '@testing-library/user-event'; | |||
| import { act } from 'react-test-renderer'; | |||
There was a problem hiding this comment.
@jsnajdr perhaps we could use the re-exported act from @testing-library/react?
There was a problem hiding this comment.
Oh yes, this was VSCode eagerly adding a random import statement as soon as I typed "act" 🙂
| @@ -245,7 +244,8 @@ describe( 'Basic rendering', () => { | |||
| name: 'Edit', | |||
| } ); | |||
|
|
|||
| await user.click( editButton ); | |||
| fireEvent.click( editButton ); | |||
There was a problem hiding this comment.
I'm curious about those instances where we actually go back and refactor from user-library to fireEvent. Could you elaborate on the rationale for that?
There was a problem hiding this comment.
I'm not sure, but I think it was something like this: user.click is an async loop like:
for ( const action of pointerActions ) {
await wait() // promisified setTimeout( 0 )
act( () => pointerAction( action ) );
}I.e. there is an async loop and the act() calls are synchronous. So, the update from computePosition().then() falls in between the pointerAction calls and is not wrapped in act().
But it's possible it's all bogus and user.click is fine. We'll need to revisit this. It's been a few weeks since I originally did these particular changes and today I know much more about how async act() works.
jsnajdr
force-pushed
the
upgrade/react-18-pure
branch
from
3e83005
to
6da6779
Compare
Only the unit test suites are in full React 18 concurrent mode. In that sense, the tests are one step ahead 🙂 There is still some flakiness there and some remaining "not wrapped in |
|
There's a subtle change that was missed here: In we remove this code gutenberg/lib/compat/wordpress-6.0/client-assets.php Lines 19 to 36 in a41ea81 wordpress-6.0 folder (previous version we did a react upgrade) and this folder is going to be removed very soon because the minimum required version of WP in the Gutenberg plugin is going to be updated to 6.1 now.
In other words, we need to move that code to That said, we shouldn't forget to update the version in core as well, when updating the packages, so I'm adding the "backport to php" label as a reminder. |
youknowriad
added
the
Backport to WP Beta/RC
* Upgrade React packages to v18 * Upgrade framer-motion to get React 18 support and updated types * React Native: upgrade to 0.69.4 * React Native: upgrade libraries * LinkControl: fix unit tests * Components: fix unit tests * Block Editor: fix unit tests * NUX: fix unit tests * useSelect: fix unit tests (partially) * LinkSettings test: fix finding blocks by label text * BlockDraggable test: fix finding blocks by label text * React Native test helpers: fix getting blocks * Block Library tests: fix finding blocks by label text * ToggleGroupControl: fix unit test by waiting for tooltip * File Block: update snapshots to account for empty children * Embed native tests: improve mocked API response * Embed native tests: wait for rich preview to appear * Format Library: fix finding blocks by label text * Edit Post: fix finding blocks by label text * Update native snapshots * useNestedSettingsUpdate: deoptimize/unbatch dispatching the updateBlockListSettings actions * Color Palette: wait for dropdown to appear * Add custom Jest matcher: toBePositionedPopover * Use toBePositionedPopover matcher in tests * Mobile - Update tests * Mobile - Update gitignore * Mobile - Update ruby config * [Mobile] - React Native 0.69.4 Upgrade - Android (WordPress#43486) * Mobile - React Native 0.69.4 upgrade, base Android changes * Mobile - Remove RNGestureHandlerEnabledRootView since its now deprecated * Mobile - Update bundle android script * [Mobile] - React Native 0.69.4 Upgrade - iOS * Mobile - Check for a local bundle running with metro and use a jsbundle as a fallback * Remove duplicated import of Gesture handler * Remove deprecated initialization for Reanimated * Update Podfile.lock after updating dependencies * Remove React peer deps from packages that don't need them * Add changelog entries about React 18 upgrade * Extract useIsScreenReaderEnabled hook, set state only if enabled=true * Mobile - Update Podfile.lock Co-authored-by: Marin Atanasov <tyxla@abv.bg> Co-authored-by: Gerardo <gerardo.pacheco@automattic.com> Co-authored-by: Derek Blank <derekpblank@gmail.com>
|
So is the current version of all these packages that require React 18 not ready for consumption? Using any of them requires using all of them from my testing due to peerDependencies, unless you want to use the ignore flag every time which we don't. Further pushing to actually try and use them resulted in typescript issues everywhere from underlying usage of @types/react with versions less than 18. |
This error indicates that there are two versions of
Unfortunately it was not feasible to keep compatibility with React 17. The |
|
@jsnajdr I did track it down via I know DefinitelyTyped/DefinitelyTyped@master...danieliser:DefinitelyTyped:master |
|
I believe it may be that outdated version of But this also mixes in with my other issue I created about the supposedly empty releases for all the packages that could in fact contain breaking changes that are not getting documented: #46730 |
|
Appears to be coming from
Will test overrides locally to see if simply changing its deps to use the newer |
|
Confirmed, simply changing that dependency in the There are a few straggling errors that didn't exist before but there are only 6 and I'll figure those out shortly. |
|
Ugg, just me or is that types package woefully untested. Many of the type files in it import packages not in package.json. Am I missing something, or should those all be listed? Had to add the following to get it to pass |
|
Also, what branch are the current releases in? Trunk shows those changes which are clearly live for the world as |
|
PR submitted that seemed to resolve it for me, no idea if/when it gets approved, though would love some extra testers: DefinitelyTyped/DefinitelyTyped#63716 |
Mamaduka
removed
the
Backport to WP Beta/RC
|
I've removed backport label. The changes will be part of the general package update. |
|
@desrosj discovered some issues with peer dependencies defined in some React libraries when testing WordPress core with Node 18.14.0 and npm: 9.3.1: WordPress/wordpress-develop#4028. I filed an issue #48009 in Gutenberg with steps to reproduce. |
Subset of #32765 that only upgrades the React packages, and does no other changes. I'd like to see which e2e tests etc are failing on the latest
trunk.