-
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 #39383
Conversation
Doing |
Was also present on this PR's predecessor: #39243 (comment) |
BTW "Reviewers" automation worked only for team Luna? Edit: ohh blind me, Jon already requested a review from "Create" so I just double -pinged everyone. 🤦♂ |
This is broken but we know what the fix is. Please don't rush to review it 🙂 #39368 is intended to ensure these PRs don't fail in the future. |
3fcc82b
to
9292fc1
Compare
9292fc1
to
28c5cd3
Compare
Hm, considering that the test-full-site-editing check fails with this:
I'm guessing we'll need to fix something. I'll look into it :) |
28c5cd3
to
469eff3
Compare
Verified that the bug does not happen on master, but does happen on this branch with |
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 think I found the issue. On this branch, after npm ci
, @wordpress/components does not exist in the root node_modules. (Which explains why the test errors indicate that it cannot find @wordpress/components.)
I will note that build-full-site-editing does still complete successfully because it is using the dependency extraction plugin, so it never tries to import from node_modules.
~/source/wp-calypso(renovate/wordpress-monorepo) » ls node_modules/@wordpress/components
ls: node_modules/@wordpress/components: No such file or directory
# I don't understand why npm ls shows the same thing in both cases.
~/source/wp-calypso(renovate/wordpress-monorepo) » npm ls @wordpress/components noah.allen@Noahs-MacBook-Pro
wp-calypso-monorepo@0.17.0 /Users/noah.allen/source/wp-calypso
└─┬ wp-calypso@0.17.0 -> /Users/noah.allen/source/wp-calypso/client
├─┬ @wordpress/block-editor@3.7.0
│ └── @wordpress/components@9.2.0 deduped
├─┬ @wordpress/block-library@2.14.0
│ └── @wordpress/components@9.2.0 deduped
├── @wordpress/components@9.2.0
├─┬ @wordpress/edit-post@3.13.0
│ └── @wordpress/components@9.2.0 deduped
├─┬ @wordpress/editor@9.12.0
│ ├─┬ @wordpress/block-directory@1.5.0
│ │ └── @wordpress/components@9.2.0 deduped
│ └── @wordpress/components@9.2.0 deduped
├─┬ @wordpress/format-library@1.14.0
│ └── @wordpress/components@9.2.0 deduped
├─┬ @wordpress/nux@3.12.0
│ └── @wordpress/components@9.2.0 deduped
└─┬ @wordpress/server-side-render@1.8.0
└── @wordpress/components@9.2.0 deduped
On master, after npm ci
, it is installed:
~/source/wp-calypso(master) » ls node_modules/@wordpress/components
CHANGELOG.md CONTRIBUTING.md LICENSE.md README.md build build-module build-style node_modules package.json src
# Interestingly, this is the same structure as above.
~/source/wp-calypso(master) » npm ls @wordpress/components noah.allen@Noahs-MacBook-Pro
wp-calypso-monorepo@0.17.0 /Users/noah.allen/source/wp-calypso
└─┬ wp-calypso@0.17.0 -> /Users/noah.allen/source/wp-calypso/client
├─┬ @wordpress/block-editor@3.5.0
│ └── @wordpress/components@9.0.0 deduped
├─┬ @wordpress/block-library@2.12.0
│ └── @wordpress/components@9.0.0 deduped
├── @wordpress/components@9.0.0
├─┬ @wordpress/edit-post@3.11.0
│ └── @wordpress/components@9.0.0 deduped
├─┬ @wordpress/editor@9.10.0
│ ├─┬ @wordpress/block-directory@1.3.0
│ │ └── @wordpress/components@9.0.0 deduped
│ └── @wordpress/components@9.0.0 deduped
├─┬ @wordpress/format-library@1.12.0
│ └── @wordpress/components@9.0.0 deduped
├─┬ @wordpress/nux@3.10.0
│ └── @wordpress/components@9.0.0 deduped
└─┬ @wordpress/server-side-render@1.6.0
└── @wordpress/components@9.0.0 deduped
I'm not really familiar with the wp dependency changes you were working on @sirreal. Do you think it would be fixed by adding it as a peer dependency? I was not able to fix this error by adding @wordpress/components to the FSE app package.json. I am able to fix this issue by adding @wordpress/components as a normal dependency in the root package.json. (But that does not appear to be our approach for other wp dependencies :))
469eff3
to
cc57c72
Compare
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Webpack Runtime (~92 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 (~26453 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~529817 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 (~448892 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Moment.js Locales (~11478 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. |
This is interesting. I would expect the module to deduplicate to the root, but this does highlight the fragility. Let's not use a peer dependency for this, that's not a good fit for app dependencies. You could argue that this is a development dependency on Instead, I'd say that What do you think?
This is pretty concerning… did you |
@sirreal As you found out, this is not true and my earlier answer was wrong. A duplicate version of |
b56e66f
to
058758a
Compare
Rebased, added webpack alias to client, more installing, updating and deduping of dependencies. |
Maybe there is a solution: in root Then the pinned dependencies inside We have a convention to sort the Other than this, I don't think that NPM gives us any options how to ensure the tree is always installed the way we want it, without duplicates. |
@jsnajdr How do you feel about the webpack alias as a solution here? I'm tempted to add another for |
Holy cow this seems like a nightmare. Will yarn solve these problems? #39508 looks good to me if it helps to get it in for this branch. |
It's an ugly band-aid but yes, it solves the issue and we already use a webpack alias for deduping |
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 is working well in my testing in gutenboarding. I'd like to move ahead with it.
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.
Seems to be fine from the FSE side of things :)
This is a good idea, but seems like another fragile solution. I do believe npm will sort the dependencies on any subsequent install, and relying on this sort of undocumented implementation detail seems risky. |
Here's another solution: declare the pinned dependencies in the root When pinning a dependency, we want to pin it for the whole monorepo, i.e., for the whole If the root If an individual package declares a dependency, it's because it wants to use it. And the individual package almost never really needs a specific version. Declaring the expected API version (i.e., what That applies to Renovate's job is then a bit simpler: it will always change versions in root |
I don't have any objection to pinning on the root, and agree that it's probably the best option both from a practical and philosophical point of view. The root isn't a real project, but rather an aggregator, and as such its job is to make sure everything works together 👍 |
That seems like a solution worth trying. Do these sound like the criteria for moving to root dependency:
|
I think the criteria are even broader: every dependency that's pinned and managed by Renovate should be declared in root. Otherwise, pinning is not reliable. Even when the package is side-effect-free and innocent, like, say If
Then the Calypso bundle ships two versions of |
Closes #39508 (merged).
This PR contains the following updates:
3.9.0
->3.11.0
2.4.0
->2.5.0
3.3.0
->3.4.0
1.2.0
->1.4.0
3.5.0
->3.7.1
2.12.0
->2.14.1
6.10.0
->6.12.0
9.0.0
->9.2.1
3.10.0
->3.11.0
2.10.0
->2.12.0
4.12.0
->4.14.0
1.6.0
->1.8.0
2.1.0
->2.2.0
2.1.0
->2.2.0
2.7.0
->2.8.0
3.11.0
->3.13.1
9.10.0
->9.12.1
2.10.0
->2.11.0
1.12.0
->1.14.1
3.8.0
->3.9.0
1.7.0
->1.8.0
5.3.1
->5.4.0
2.8.0
->2.9.0
1.11.0
->2.0.0
2.1.0
->2.2.0
3.10.0
->3.12.1
2.10.0
->2.12.0
3.6.2
->3.7.0
3.10.0
->3.12.0
6.2.0
->7.1.0
1.6.0
->1.8.1
2.5.0
->2.6.0
2.9.0
->2.11.0
2.11.0
->2.13.0
Release Notes
WordPress/gutenberg
v3.11.0
Compare Source
v3.10.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: Whenever PR becomes conflicted, or if you tick the rebase/retry checkbox below.
👻 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.