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
Conversation
@Automattic/cylon are you the folks to ping about this, or can you pass the ping along? |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~98825 bytes added 📈 [gzipped])
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])
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])
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. |
😅 e2e failures don't appear to be related… |
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
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. |
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. |
I'll ask around to check |
Actually, I do see that we use the package to run our unit tests: wp-calypso/apps/full-site-editing/package.json Lines 38 to 40 in d5bc4c9
|
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. |
I opened this patch puppeteer/puppeteer#5325 against Puppeteer to explore some better ideas on how this could be solved on the |
231c484
to
ab5afd3
Compare
Rebased |
All e2e are passing, this should be all set to land and save us all some time 😀 |
bin/install-if-no-packages.js
Outdated
const installResult = spawnSync( 'npm', [ 'ci' ], { | ||
shell: true, | ||
stdio: 'inherit', | ||
env: Object.assign( { PUPPETEER_SKIP_CHROMIUM_DOWNLOAD: 'true' }, process.env ), | ||
} ).status; |
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.
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.
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.
Done in ae81a1b
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.
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
.
Changes proposed in this Pull Request
@wordpress/scripts
has a dependency that installschromium
. That's a big dependency that takes a long time to download and in never used in our most common workflows. Skip thechromium
install on:npm start
flowsThe FSE tests that depend on
@wordpress/scripts
will need to runnpm ci
or similar before running e2e tests depending onchromium
.Testing instructions
touch package-lock.json; npm start
-chromium
should not be downloaded.