Try #2: Upgrade @wordpress packages to pre-React 18 #73890
Conversation
tyxla
added
[Status] In Progress
DO NOT MERGE
dependencies
|
This PR modifies the release build for editing-toolkit To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-mMA-p2 |
tyxla
force-pushed
the
try/upgrade-wordpress-packages-pre-react18-2
branch
2 times, most recently
from
88adc7e to
152c838
Compare
tyxla
force-pushed
the
try/upgrade-wordpress-packages-pre-react18-2
branch
from
24ec7c2 to
2a2ed8d
Compare
tyxla
force-pushed
the
try/upgrade-wordpress-packages-pre-react18-2
branch
4 times, most recently
from
3713c7d to
5f6357c
Compare
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~22292 bytes added 📈 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~4880 bytes removed 📉 [gzipped]) Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~14342 bytes added 📈 [gzipped]) React components that are loaded lazily, when a certain part of UI is displayed for the first time. Moment.js Locales (~3055 bytes added 📈 [gzipped]) Locale data for moment.js. Unless you are upgrading the moment.js library, changes in these chunks are suspicious. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
tyxla
force-pushed
the
try/upgrade-wordpress-packages-pre-react18-2
branch
2 times, most recently
from
400d12e to
4c40d8e
Compare
|
The e2e issue happens in this context:
This does not happen in production, and it also doesn't happen when I'm logged into my main account in calypso.live. |
|
The fix was pretty straightforward: In this code, The main concern I have from this is whether there are other instances of |
There was a problem hiding this comment.
Amazing work here @tyxla! This is a huge amount of changes for the upgrade 😅 Hopefully the next round is easier!
With the e2e bug fix, I realized that this PR adds empty dependency arrays ([]) to a lot of useSelect calls without necessarily adding dependencies. I went through the PR and commented on each missing dependency I could find in each useSelect call where we're adding the dependency array. (I think this should be as extensive as possible with human eyes 😅)
Some of these might not be functionally needed, but I think it's worth fixing just in case they would introduce bugs. I decided to comment first, and then I'll work on updating all of them.
Edit: updated all of these in 5955722
apps/editing-toolkit/editing-toolkit-plugin/editor-site-launch/src/launch-modal/index.tsx
Show resolved
Hide resolved
...ting-toolkit/editing-toolkit-plugin/editor-site-launch/src/launch-steps/final-step/index.tsx
Outdated
Show resolved
Hide resolved
...t/editing-toolkit-plugin/wpcom-block-editor-nav-sidebar/src/components/nav-sidebar/index.tsx
Show resolved
Hide resolved
noahtallen
force-pushed
the
try/upgrade-wordpress-packages-pre-react18-2
branch
from
87108f1 to
5955722
Compare
There was a problem hiding this comment.
Thank you for your help addressing the last bits, @noahtallen! 🙌
I've spent quite some time verifying that these are all correct and I've made some observations about a few that we could further polish. This took a while: 😅
I'll take care of them immediately, and do some smoke testing before marking this as ready to review.
...t/editing-toolkit-plugin/wpcom-block-editor-nav-sidebar/src/components/nav-sidebar/index.tsx
Show resolved
Hide resolved
| "@wordpress/dom-ready": "^3.22.0", | ||
| "@wordpress/edit-post": "^6.19.0", | ||
| "@wordpress/editor": "^12.21.0", | ||
| "@wordpress/element": "^4.20.0", | ||
| "@wordpress/env": "^4.7.0", |
There was a problem hiding this comment.
Noting that we intentionally didn't change this dependency, because:
- The
@wordpress/envpackage isn't a dependency of any other package and doesn't have any other packages as dependencies. Therefore, it can be updated separately and independently. - There's a breaking change in the way the PHPUnit tests are set up. Ideally, we should take care of that in a separate PR.
There was a problem hiding this comment.
Good observation, that matches my experience with wp-env updates. (I also have a WIP PR somewhere for this phpunit change, which I should resurrect!)
| @@ -164,8 +163,7 @@ const mapDispatchToProps = ( dispatch, ownProps ) => { | |||
| }; | |||
| }; | |||
|
|
|||
| export default compose( | |||
There was a problem hiding this comment.
Necessary because compose types aren't very friendly, but dealing with them is work for another time and another PR.
| @@ -167,23 +167,21 @@ | |||
| "@types/validator": "^13.7.1", | |||
| "@types/webpack-env": "^1.16.3", | |||
| "@types/wordpress__api-fetch": "^3.2.4", | |||
| "@types/wordpress__block-editor": "^6.0.5", | |||
There was a problem hiding this comment.
We are removing some of those type packages because they augment or redeclare the @wordpress/data types, which is in direct conflict with the new way we're consuming @wordpress/data types - directly from the package.
| "@wordpress/interface@^4.8.0": "patch:@wordpress/interface@npm%3A4.8.0#./.yarn/patches/@wordpress-interface-npm-4.8.0-8a39ad37cf.patch" | ||
| "@wordpress/base-styles": "4.13.0", | ||
| "@wordpress/interface@^4.8.0": "patch:@wordpress/interface@npm%3A4.8.0#./.yarn/patches/@wordpress-interface-npm-4.8.0-8a39ad37cf.patch", | ||
| "@wordpress/data-controls": "2.22.0", |
There was a problem hiding this comment.
Pinning to avoid type conflicts and a waterfall of unwanted dependencies.
| "@wordpress/base-styles": "4.13.0", | ||
| "@wordpress/interface@^4.8.0": "patch:@wordpress/interface@npm%3A4.8.0#./.yarn/patches/@wordpress-interface-npm-4.8.0-8a39ad37cf.patch", | ||
| "@wordpress/data-controls": "2.22.0", | ||
| "@wordpress/element": "4.20.0" |
There was a problem hiding this comment.
This package has been a bit merciless with breaking changes sneaking into minor versions, so we need to pin it down to avoid them inadvertently sneaking into the code.
| export type Status = 'error' | 'info' | 'success' | 'warning'; | ||
| } | ||
|
|
||
| declare module '@wordpress/rich-text' { |
There was a problem hiding this comment.
These two are a classic example of types we needed to specifically declare because those packages also attempted to declare the @wordpress/data module, which directly entered in conflict with the new package-provided @wordpres/data types. Ideally, each of those packages should provide types directly.
There was a problem hiding this comment.
Do you think it'd be worth trying to fix the DT defs? (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/wordpress__rich-text/index.d.ts)
Technically, I guess those are broken for everyone with new wordpress data versions.
| @@ -70,6 +70,8 @@ const Guide: React.FC< Props > = ( { children, className, onFinish } ) => { | |||
| <div className="guide__buttons"> | |||
| { currentPage > 0 && ( | |||
| <Button className="guide__back-button" isTertiary onClick={ goBack }> | |||
| { /* eslint-disable-next-line @typescript-eslint/ban-ts-comment */ } | |||
| { /* @ts-ignore This is declared as a global variable and provided by webpack. */ } | |||
There was a problem hiding this comment.
Necessary because multiple packages are attempting to declare this global.
tyxla
force-pushed
the
try/upgrade-wordpress-packages-pre-react18-2
branch
2 times, most recently
from
1641572 to
d8da603
Compare
|
The failing e2e tests are flaky and seem to be failing in other branches. I think this one is ready for review 👀 |
tyxla
force-pushed
the
try/upgrade-wordpress-packages-pre-react18-2
branch
from
012518d to
b8a0f10
Compare
|
Given that most of the changes are TS changes and I have a green light, in light of how quickly this can get stale, I'm inclined to ship it now. I already did extensive testing and I'm going to do so on staging, too. Thanks everyone 🙌 |
cc @jsnajdr - could this be something we could detect and prevent within |
Yes, with ever-changing The bad thing is that the selector returns a different value on each call, although the inputs are, at least semantically, the same. So, the main question is why The A second place where query object equality is checked is the reducer that stores the query results. Here the query object is serialized to a string with the
If it turns out that Other than that, this is more like a typical React issue with immutability and effect dependencies, not something |
|
Nice clarification! |
|
Translation for this Pull Request has now been finished. |
This PR aims to upgrade the
@wordpresspackages to versions before upgrading to React 18.The second take of #73155.
Does a lot of dependency and TS changes. There are some dependency compromises and specific pinning, but this PR aims to make a step forward towards updating the
@wordpresspackages to the latest version and restarting their regular periodic upgrades.I think it requires thorough testing of all major flows.