-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix(deps): update wordpress monorepo #46223
Conversation
3edbb08
to
18ab7e6
Compare
081e21d
to
8697953
Compare
Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com D50919-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2 |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Webpack Runtime (~518 bytes removed 📉 [gzipped])
Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded. App Entrypoints (~417067 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~391383 bytes added 📈 [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 (~162801 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. 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. |
@@ -67,7 +67,8 @@ class ExtensiveLodashReplacementPlugin { | |||
throw createError( 'Could not determine root `lodash-es` version.' ); | |||
} | |||
|
|||
if ( baseLodashVersion !== this.baseLodashESVersion ) { | |||
// TODO: change these to the exact versions when lodash and lodash-es releases match again |
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 means we will be replacing [email protected]
with [email protected]
? I don't think that is valid, it will re-introduce bugs that lodash already fixed, including a security issue they fixed in v4.17.19.
As an alternative approach we could drop this lodash -> lodash-es
replacement, or change the plugin to replace in the other way (lodash-es -> lodash
)
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.
I'm not saying it's a great approach 😉 Just trying to get to a temporary solution until lodash-es
is released again. Happy to see alternatives be explored in the meantime, but I don't see this as something that's blocking us. How affected are we really by those 2 minor releases that were released in the meantime?
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.
I mean we don't even use zipObjectDeep
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.
What if tomorrow lodash
releases 4.18.0
, with a ton of new features and somebody does the update? Everything will break because we force a replacement with 4.17.15
, and those errors may appear only in prod. What if somebody starts using zipObjectDeep
(or any of the other fixed methods)?
I think than the fastest and easiest alternative is just drop the lodash-es replacement, and deal with the hypothetical performance regression later.
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.
Well, the performance regression isn't that hypothetical, but I'll leave this call to @sgomes.
No strong feelings either way, but in general I believe this lodash problem should be solved separately and we shouldn't be blocking this PR.
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.
There's a lengthy discussion on the pros/cons of the change in Slack. But to summarise the summary:
- @tyxla's approach would preserve the current status quo while upgrading libraries
- Switching to
lodash
instead oflodash-es
would make the critical path on small endpoints likelogin
larger (bad) while making the total size of the application smaller (good, but not that impactful).
Dropping the plugin altogether is yet another option.
@razvanpapadopol Thanks for finding that. It's a bug in core. This was fixed in WordPress/gutenberg#25976, once it is released we can remove the patch. I've applied a patch to this PR to prevent the crash in the mean time. |
cd4b4c7
to
ba7faf9
Compare
Smoke-tested the editor Launch flow (Complete setup), everything seems to be working fine |
123852d
to
ea415ef
Compare
@Automattic/team-calypso 👋 Any updates on this one? Jetpack is also in sore need of a Would be great if this landed so folks could use it as a cheatsheet for other repos 😊 |
ea415ef
to
8fe29a8
Compare
8fe29a8
to
21e3960
Compare
I have rebased this branch with
|
I added a commit that fixed data controls type names in tests: a change of internal implementation detail in WordPress/gutenberg#25362 that is nevertheless used in tests. There are two more test failures, when testing data resolvers and the uninitialized -> pending -> resolved state progression while the resolver is running. These tests depend on a subtle implementation detail that's been changed in WordPress/gutenberg#21289. Before that PR, resolver starts running synchronously when its selector is called, dispatching an action that updates state from After that PR, the resolver is triggered asynchronously with |
The next commit fixes the failing selector resolution tests by adding the right number of |
Looking at the remaining unit test failures, there are errors from React caused by |
I deduplicated all packages for which this PR added a new version: got the list with After that deduplication, only 4 tests are failing. |
Now it looks like there are some issues with composite-checkout types cc: @Automattic/shilling |
Thanks for the heads-up! There was some old code from before the package was converted to TS which didn't have proper types and also had some outdated properties from a previous refactor; I've pushed a few commits to hopefully resolve those issues. |
Now the checkout tests are returning this error:
I assume this has to do with multiple versions of React being present in the app, since the code hasn't changed in this PR, but I'm not sure how to track that down. |
@jsnajdr whenever you're back online do you think you could take a look at the bundle differences? It looks like react and a load of other shared dependencies got pushed into the gutenboarding bundle but I'm not sure why. |
There's a lot of duplicated
I made this list with a custom script (gist here) that traverses the There are also two versions of React installed:
To remedy this, we should formally upgrade Calypso to 16.14.0 or newer (maybe even 17?), coordinated with Gutenberg. |
The React duplication is present in
It's a regression introduced in #47779 and I'm proposing a fix in #48644. Merging that PR and rebasing this one should fix some of the duplication. |
cefadea
to
03dac2d
Compare
I'm very curious, why does it seem like every time we have one of these PRs, the packages versions have been duplicated a lot? Is there a problem with how we add dependencies that we could fix with better linting or documentation? It'd be lovely to avoid having the same problem every few months :P |
@scinos Is there a way to stop renovate from stomping on our changes every so often? Can we expel it from this PR entirely at this point? I don't think it's helpful for us right now in this context as we have to keep redoing the fixes every time. |
I moved my changes out into #48679 to make sure they don't get clobbered in this PR. |
@sirbrillig That's a good call, I'll do the same with the color changes if it's possible. |
Yeah, good plan. I don't have any changes locally from before the previous clobber (52239fbd26, when I pushed my changes), but if it's helpful @sirreal, I think this might need to be replaced or made into its own PR:
|
03dac2d
to
d1723f6
Compare
This looks like a Renovate bug. The Renovate docs say that Renovate will never update a branch once you edit it by pushing a new commit. And we clearly pushed a lot of these, and yet Renovate has reset our branch several times already, starting in early December. @rarkins is this a known issue? |
Renovate looks at the head commit author to determine if the branch has been modified. If anyone force pushed and left Renovate's commit or author last then it will think the branch is not edited. Any other behaviour would be a bug. |
@rarkins Thanks for chiming in! The most recent undesired force-push is:
And the cefadea HEAD commit was done by @sirbrillig, both as committer and author. That means that Renovate shouldn't have touched the branch. |
d1723f6
to
e46193b
Compare
I haven't been able to confirm for sure, but I think the problem here is a combination of (a) many PRs, and (b) this PR is quite old. For performance reasons we don't fetch every PR in a repo and limit it to 1000. According to the logs, Renovate isn't finding this PR, so think that is explained by the above. This log shows that the last 1000 PRs only included 16 Renovate PRs:
Unfortunately GitHub's GraphQL interface for PRs doesn't let us filter by PR creator. Something you could do to work around this is to rename and close old PRs so that they are recreated and go back to the top of the list. |
Thanks @rarkins for looking into the issue. I'll proceed by creating a completely new branch and PR for these changes, because there are many manual interventions that need to be done. If Renovate cannot find a particular PR using the Github API, that looks like an error or at least an unexpected condition. Could Renovate refrain from modifying the PR in that case? Force pushing into a branch with custom changes is a very disruptive action, and Renovate should perform it only when it's 100% sure it's doing the right thing. |
It can't tell the difference between a branch with no PR versus one with a PR that it can't find. A branch with no PR is a valid state. But we might be able to do a modified check at the git layer instead. |
e46193b
to
a5dd89c
Compare
Closed by #48729 |
This PR contains the following updates:
^2.11.0
->^2.13.0
^3.18.0
->^3.20.0
^3.7.0
->^3.9.0
^2.0.1
->^3.1.0
^2.0.1
->^3.1.0
^1.13.6
->^1.16.0
^4.3.6
->^5.0.0
^2.22.6
->^2.25.0
^6.20.3
->^6.23.0
^10.0.5
->^11.0.0
^3.19.3
->^3.21.0
^2.20.3
->^2.23.0
^2.16.0
->^2.23.0
^4.22.3
->^4.24.0
^4.22.3
->^4.24.0
^1.16.3
->^1.18.0
^2.13.1
->^2.15.0
^2.10.0
->^2.11.0
^4.11.2
->^4.14.0
^3.21.6
->^3.24.0
^9.20.6
->^9.23.0
^2.16.0
->^2.18.0
^2.16.0
->^2.18.0
1.6.0
->2.1.0
^1.9.0
->^1.10.0
^7.1.0
->^7.3.0
^1.22.6
->^1.24.0
^2.9.0
->^2.10.0
^3.14.0
->^3.16.0
^3.14.0
->^3.16.0
^2.4.0
->^2.7.0
^0.7.5
->^0.9.0
^2.1.0
->^2.3.0
^6.2.0
->^6.4.0
^1.9.3
->^1.11.0
^2.14.0
->^2.16.0
^1.15.0
->^1.17.0
^2.8.3
->^2.10.0
^3.20.5
->^3.22.0
^2.20.3
->^2.22.0
^1.7.0
->^1.9.0
^1.7.0
->^1.9.0
^3.10.0
->^3.12.0
^3.20.4
->^3.22.0
^12.1.1
->^12.3.0
^1.16.5
->^1.18.0
^2.9.0
->^2.11.0
^2.17.0
->^2.19.0
^2.21.3
->^2.23.0
Release Notes
WordPress/gutenberg
v2.13.0
Compare Source
v2.12.0
Compare Source
Renovate configuration
📅 Schedule: At any time (no schedule defined).
🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.
♻️ Rebasing: Renovate will not automatically rebase this PR, because other commits have been found.
👻 Immortal: This PR will be recreated if closed unmerged. Get config help if that's undesired.
This PR has been generated by WhiteSource Renovate. View repository job log here.