Skip to content
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 @wordpress/* duplicates #41141

Merged
merged 1 commit into from
Apr 16, 2020
Merged

Conversation

sgomes
Copy link
Contributor

@sgomes sgomes commented Apr 15, 2020

#39644 introduced a number of duplicates when updating packages from the @wordpress org. This is visible in the build logs:

WARNING in @wordpress/api-fetch
  Multiple versions of @wordpress/api-fetch found:
    3.11.0 /Users/sgomes/dev/wp-calypso/~/@wordpress/api-fetch
    3.12.0 ./~/@wordpress/api-fetch


WARNING in @wordpress/compose
  Multiple versions of @wordpress/compose found:
    3.11.0 /Users/sgomes/dev/wp-calypso/~/@wordpress/compose
    3.12.0 ./~/@wordpress/compose


WARNING in @wordpress/data-controls
  Multiple versions of @wordpress/data-controls found:
    1.8.0 /Users/sgomes/dev/wp-calypso/~/@wordpress/data-controls
    1.9.0 ./~/@wordpress/data-controls


WARNING in @wordpress/element
  Multiple versions of @wordpress/element found:
    2.11.0 /Users/sgomes/dev/wp-calypso/~/@wordpress/element
    2.12.0 ./~/@wordpress/element


WARNING in @wordpress/url
  Multiple versions of @wordpress/url found:
    2.11.0 /Users/sgomes/dev/wp-calypso/~/@wordpress/url
    2.12.0 ./~/@wordpress/url

This PR fixes that, by ensuring that there are no duplicate @wordpress/* packages in use.

Changes proposed in this Pull Request

  • Move all pinned @wordpress/* dependencies to the root package.json
  • Make all @wordpress/* dependencies on sub-projects use a compatibility range
  • Install newer versions of other @wordpress/* packages, to match the ones that were installed and thus avoid pulling in duplicate versions of their dependencies.
  • Add pinned dependencies for the new packages above.

Testing instructions

Ensure that the application builds correctly, and canaries/smoke testing doesn't reveal any issues.

@sgomes sgomes added [Type] Bug [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build labels Apr 15, 2020
@sgomes sgomes requested review from simison, sirreal, noahtallen and a team April 15, 2020 13:36
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Webpack Runtime (~50 bytes removed 📉 [gzipped])

name      parsed_size           gzip_size
manifest       -441 B  (-0.3%)      -50 B  (-0.2%)

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 (~3595 bytes removed 📉 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding     -13149 B  (-0.6%)    -3591 B  (-0.6%)
entry-main                -147 B  (-0.0%)       -4 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~6482 bytes removed 📉 [gzipped])

name                   parsed_size           gzip_size
plans                       -505 B  (-0.1%)     -465 B  (-0.4%)
email                       -505 B  (-0.2%)     -382 B  (-0.5%)
domains                     -505 B  (-0.1%)     -796 B  (-0.4%)
gutenberg-editor            -503 B  (-0.1%)     -699 B  (-0.3%)
security                    -486 B  (-0.1%)     -170 B  (-0.2%)
purchases                   -486 B  (-0.1%)     -165 B  (-0.1%)
posts-custom                -486 B  (-0.2%)     -180 B  (-0.2%)
posts                       -486 B  (-0.2%)     -180 B  (-0.2%)
pages                       -486 B  (-0.2%)     -160 B  (-0.3%)
media                       -486 B  (-0.1%)     -166 B  (-0.2%)
marketing                   -486 B  (-0.1%)     -174 B  (-0.2%)
hosting                     -486 B  (-0.3%)     -166 B  (-0.3%)
earn                        -486 B  (-0.2%)     -169 B  (-0.2%)
zoninator                   -407 B  (-0.2%)     -123 B  (-0.2%)
woocommerce                 -407 B  (-0.0%)     -166 B  (-0.0%)
stats                       -407 B  (-0.1%)     -128 B  (-0.1%)
settings-writing            -407 B  (-0.1%)     -136 B  (-0.1%)
settings                    -407 B  (-0.1%)     -120 B  (-0.1%)
scan                        -407 B  (-0.3%)     -116 B  (-0.3%)
post-editor                 -407 B  (-0.0%)     -166 B  (-0.0%)
plugins                     -407 B  (-0.1%)     -130 B  (-0.1%)
people                      -407 B  (-0.1%)     -130 B  (-0.1%)
notification-settings       -407 B  (-0.1%)     -134 B  (-0.2%)
jetpack-connect             -407 B  (-0.1%)     -120 B  (-0.1%)
help                        -407 B  (-0.1%)      -94 B  (-0.1%)
google-my-business          -407 B  (-0.2%)     -144 B  (-0.2%)
concierge                   -407 B  (-0.1%)      -94 B  (-0.1%)
comments                    -407 B  (-0.1%)     -148 B  (-0.1%)
activity                    -407 B  (-0.1%)     -117 B  (-0.1%)
account                     -407 B  (-0.1%)     -149 B  (-0.2%)
backups                     -406 B  (-0.1%)     -137 B  (-0.2%)
checkout                    -332 B  (-0.0%)     -258 B  (-0.1%)

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 (~1627 bytes removed 📉 [gzipped])

name                                                 parsed_size           gzip_size
async-load-my-sites-current-site-domain-warnings          -572 B  (-1.1%)     -398 B  (-2.9%)
async-load-design-playground                              -486 B  (-0.0%)     -179 B  (-0.0%)
async-load-design-blocks                                  -486 B  (-0.0%)     -181 B  (-0.0%)
async-load-design                                         -486 B  (-0.0%)     -179 B  (-0.0%)
async-load-signup-steps-plans                             -407 B  (-0.2%)     -141 B  (-0.3%)
async-load-signup-steps-domains                           -407 B  (-0.2%)     -124 B  (-0.2%)
async-load-signup-steps-clone-point                       -407 B  (-0.3%)      -77 B  (-0.2%)
async-load-reader-search-stream                           -407 B  (-0.5%)     -102 B  (-0.5%)
async-load-reader-following-manage                        -407 B  (-0.4%)     -129 B  (-0.5%)
async-load-blocks-inline-help-popover                     -407 B  (-0.2%)      -94 B  (-0.2%)
async-load-my-sites-sidebar                                -84 B  (-0.1%)      -12 B  (-0.1%)
async-load-landing-jetpack-cloud-components-sidebar        -84 B  (-0.5%)      -11 B  (-0.2%)

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.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@sgomes
Copy link
Contributor Author

sgomes commented Apr 15, 2020

Looks like this saves ~3.5KB (compressed) on gutenboarding, which is where most of the duplication was living.

@sgomes sgomes added [Status] Blocked / Hold and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 15, 2020
@sgomes
Copy link
Contributor Author

sgomes commented Apr 15, 2020

Changing status to "on hold" as @scinos looks into a possible yarn migration first.

@sgomes sgomes added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Blocked / Hold labels Apr 15, 2020
@sgomes
Copy link
Contributor Author

sgomes commented Apr 15, 2020

The yarn migration is on hold for now, so this is ready for a review!

@simison simison requested a review from p-jackson April 15, 2020 15:09
@noahtallen
Copy link
Contributor

Thanks for doing this! I am not very familiar with the various work that has been done to improve @wordpress/ packages with npm in calypso, so I probably missed something when I rebased it on top of some of the yarn-related changes

Copy link
Contributor

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the following are working after npm ci on this branch

  • ✅ calypso.localhost
  • ✅ calypso.localhost Gutenboarding
  • ✅ FSE plugin synced to dotcom
  • ✅ SPT selector in the page editor

Copy link
Member

@p-jackson p-jackson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After doing a npm run clean and npm ci I'm still getting warnings in the logs: 249b0-pb

I don't think I remember the warnings about the missing exports before this PR.

Everything's working when I manually test though.

"@wordpress/editor": "^9.13.0",
"@wordpress/element": "^2.12.0",
"@wordpress/i18n": "^3.10.0",
"classnames": "^2.2.6"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

classnames hasn't been moved to the root package.json as a pinned dependency. Given that o2 blocks is an "app" should this dependency still be pinned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're ok not pinning these versions anywhere for now, since we already have the ranges here guaranteeing we won't get incompatible versions. Team Calypso is working on a migration to yarn where we won't be pinning at all, just ranges, and they're hoping to get it ready in the next few days.

Comment on lines +27 to +30
"core-js": "^3.6.3",
"isomorphic-fetch": "^2.2.1",
"regenerator-runtime": "^0.13.3",
"svg4everybody": "^2.1.9"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this is a sharable package it makes sense that these dependencies are no longer pinned. Was this required for the PR or just some house keeping? Just checking that you didn't mean to add these to the root package.json too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed to make these ranges because of regenerator-runtime, which would have been duplicated. I went ahead and did the same for the other dependencies.

@sgomes
Copy link
Contributor Author

sgomes commented Apr 16, 2020

After doing a npm run clean and npm ci I'm still getting warnings in the logs: 249b0-pb

I don't think I remember the warnings about the missing exports before this PR.

They are there already in master, I'm afraid. They're part of the reason I'm working in this PR, and a subsequent one that will update Babel. You'll notice in your pb that there are no warnings for any @wordpress/* duplicates, however, which there currently are in master.

@sgomes sgomes merged commit 1d03056 into master Apr 16, 2020
@sgomes sgomes deleted the fix/wordpress-monorepo-duplicates branch April 16, 2020 09:33
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 16, 2020
@sgomes
Copy link
Contributor Author

sgomes commented Apr 16, 2020

Thank you for the reviews, @noahtallen and @p-jackson! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants