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

Build: Remove chromium install from common flows #38900

Merged
merged 4 commits into from Jan 24, 2020

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Jan 17, 2020

Changes proposed in this Pull Request

@wordpress/scripts has a dependency that installs chromium. That's a big dependency that takes a long time to download and in never used in our most common workflows. Skip the chromium install on:

  • npm start flows
  • CircleCI

The FSE tests that depend on @wordpress/scripts will need to run npm ci or similar before running e2e tests depending on chromium.

Testing instructions

  • touch package-lock.json; npm start - chromium should not be downloaded.
  • No regressions in CircleCI or e2e tests.

@sirreal sirreal requested a review from a team as a code owner January 17, 2020 12:12
@matticbot
Copy link
Contributor

@sirreal sirreal self-assigned this Jan 17, 2020
@sirreal sirreal added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build [Type] Performance labels Jan 17, 2020
@sirreal
Copy link
Member Author

sirreal commented Jan 17, 2020

The FSE tests that depend on @wordpress/scripts will need to run npm ci or similar before running e2e tests depending on chromium.

@Automattic/cylon are you the folks to ping about this, or can you pass the ping along?

@matticbot
Copy link
Contributor

matticbot commented Jan 17, 2020

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

App Entrypoints (~98825 bytes added 📈 [gzipped])

name                   parsed_size            gzip_size
entry-main               +141453 B   (+9.2%)   +37170 B  (+10.0%)
entry-login              +141453 B  (+13.9%)   +37170 B  (+14.0%)
entry-domains-landing    +141453 B  (+35.0%)   +37170 B  (+35.1%)
entry-jetpack-cloud       -92803 B  (-38.5%)   -25362 B  (-34.8%)
entry-gutenboarding       +28584 B   (+1.5%)   +12677 B   (+2.5%)

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

Sections (~279228 bytes removed 📉 [gzipped])

name                   parsed_size             gzip_size
posts-custom             -866414 B   (-75.9%)  -209199 B   (-74.8%)
posts                    -219054 B   (-44.2%)   -46141 B   (-39.3%)
me                       -155230 B   (-41.5%)   -25949 B   (-30.8%)
site-blocks              -103812 B   (-29.9%)   -22879 B   (-26.2%)
post-editor              -102756 B    (-5.1%)   -20425 B    (-3.8%)
activity                 -102756 B   (-18.3%)   -20425 B   (-15.0%)
devdocs                   +95765 B  (+174.7%)   +26432 B  (+187.2%)
import                    +79917 B   (+65.0%)   +20630 B   (+64.6%)
wp-super-cache            +75707 B   (+51.9%)   +15682 B   (+39.3%)
stats                     -72903 B    (-8.5%)   -16596 B    (-7.8%)
notification-settings     +55918 B   (+22.3%)   +11279 B   (+17.0%)
happychat                 -55754 B   (-18.5%)   -10817 B   (-13.9%)
comments                  -47589 B    (-8.6%)    -7057 B    (-5.7%)
woocommerce               +30025 B    (+1.5%)    +6901 B    (+1.3%)
purchases                 +30025 B    (+3.3%)    +6901 B    (+3.2%)
checkout                  +30025 B    (+2.7%)    +6901 B    (+2.5%)
media                     +16779 B    (+4.9%)    +3825 B    (+4.2%)
privacy                   +15185 B    (+7.3%)    +4779 B    (+8.7%)
marketing                  -7109 B    (-1.7%)    -3070 B    (-2.9%)

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

name                                                         parsed_size            gzip_size
async-load-design-playground                                   -102756 B   (-5.7%)   -20425 B   (-5.0%)
async-load-design                                               -78027 B   (-4.2%)   -17832 B   (-4.2%)
async-load-design-blocks                                        -72731 B   (-2.6%)   -13524 B   (-2.1%)
async-load-lib-preferences-helper                               -51122 B  (-78.0%)   -13779 B  (-74.8%)
async-load-my-sites-checklist-wpcom-checklist-component-jsx     -19038 B  (-12.6%)    -5304 B  (-14.2%)
async-load-lib-happychat-connection                             -13219 B  (-17.7%)     +547 B   (+3.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.

@sirreal
Copy link
Member Author

sirreal commented Jan 17, 2020

  • No regressions in CircleCI or e2e tests.

😅 e2e failures don't appear to be related…

@sirreal sirreal removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 17, 2020
@sirreal sirreal added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 17, 2020
@noahtallen
Copy link
Member

@Automattic/cylon are you the folks to ping about this, or can you pass the ping along?

I can take a look at this. I think Ajax wrote the tests initially for SPT, but it shouldn't be hard to add. By the way ci/circleci: test-full-site-editing still seems to pass on this PR so is there anything I need to change?

before running e2e tests depending on chromium

Oh duh, I guess we don't run any e2e tests that depend on that yet. It honestly looks like we only use the Jest unit test config from that package.

I'll be looking at e2e test suites soon, so I'll keep this in mind. I don't think it should affect any of our existing test code for the time being.

@sirreal
Copy link
Member Author

sirreal commented Jan 17, 2020

It honestly looks like we only use the Jest unit test config from that package.

It might be worth removing the dependency if that's all it's used for. I believe the Jest preset is published as its own package as well.

@noahtallen
Copy link
Member

It might be worth removing the dependency if that's all it's used for.

I'll ask around to check

@noahtallen
Copy link
Member

Actually, I do see that we use the package to run our unit tests:

"test:unit": "npx wp-scripts test-unit-js --config='jest.config.js'",
"test:unit:help": "npx wp-scripts test-unit-js --config='jest.config.js' --help",
"test:unit:watch": "npx wp-scripts test-unit-js --config='jest.config.js' --watch",

@gziolo
Copy link
Member

gziolo commented Jan 20, 2020

Any thoughts how it could be resolved on the wp-scripts side? It’s super annoying for all projects that don’t use e2e tests 😅 Gutenberg uses the same env flag for CI. My idea always was to install Chromium only when you execute tests that need it.

@gziolo
Copy link
Member

gziolo commented Jan 20, 2020

I opened this patch puppeteer/puppeteer#5325 against Puppeteer to explore some better ideas on how this could be solved on the @wordpress/scripts side.

@sirreal
Copy link
Member Author

sirreal commented Jan 23, 2020

Rebased

@sirreal sirreal requested review from a team January 23, 2020 11:33
@sirreal
Copy link
Member Author

sirreal commented Jan 23, 2020

All e2e are passing, this should be all set to land and save us all some time 😀

const installResult = spawnSync( 'npm', [ 'ci' ], {
shell: true,
stdio: 'inherit',
env: Object.assign( { PUPPETEER_SKIP_CHROMIUM_DOWNLOAD: 'true' }, process.env ),
} ).status;
Copy link
Member

Choose a reason for hiding this comment

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

Here I see the same error as I fixed in #39009 for install-if-deps-outdated -- wrong type of installResult and suboptimal error message. I'll push a quick fix to this branch.

Copy link
Member

Choose a reason for hiding this comment

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

Done in ae81a1b

Copy link
Member

@jsnajdr jsnajdr left a comment

Choose a reason for hiding this comment

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

Looks good 👍 It would be nice to optimize npm run update-deps, too. One doesn't need to install Chromium to generate a correct lockfile. But that script is implemented as one-liner directly in package.json.

@sirreal sirreal removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 23, 2020
@sirreal sirreal added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 23, 2020
@sirreal sirreal added [Status] Ready to Merge [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 23, 2020
@sirreal sirreal merged commit d42d997 into master Jan 24, 2020
@sirreal sirreal deleted the remove/pupeteer-install branch January 24, 2020 09:38
@matticbot matticbot removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Ready to Merge labels Jan 24, 2020
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.

None yet

6 participants