Move code of the icfy-analyze CircleCI task to a NPM script #39502
Conversation
In hope to get better error handling a recovery, I'm moving the commands for the `icfy-analyze` CircleCI task (the `npm run preanalyze-bundles`, running the `bin/analyze-icfy.js` script, moving files to artifacts folder) to a NPM script called `analyze-icfy`. That script chains the steps using `&&` operator, relying on exit codes. That should be more reliable (or at least more comprehensible) than the `set +o errexit` bash option. This also creates a `build-client-stats` NPM script that's used by both `analyze-bundles` and `analyze-icfy` scripts.
jsnajdr
added
[Status] Needs Review
|
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
| "preanalyze-bundles": "cross-env-shell NODE_ENV=production CALYPSO_ENV=production EMIT_STATS=true PROGRESS=true npm run -s build-client-evergreen", | ||
| "analyze-bundles": "webpack-bundle-analyzer client/stats.json public/evergreen -h 127.0.0.1 -p 9898 -s gzip", | ||
| "analyze-bundles": "npm run build-client-stats && webpack-bundle-analyzer client/stats.json public/evergreen -h 127.0.0.1 -p 9898 -s gzip", | ||
| "analyze-icfy": "npm run build-client-stats && node --max-old-space-size=4096 bin/icfy-analyze.js && mv client/stats.json client/chart.json \"$CIRCLE_ARTIFACTS/icfy\"", |
There was a problem hiding this comment.
If there's an issue with cross-env-shell that results in a 0 exist status when we expect non-0, this type of chaining will fail to capture that, won't it?
There was a problem hiding this comment.
Yes, that's true. I discovered the cross-env issue only after migrating the icfy-analyze task to a NPM script and debugging it.
In other words, I don't expect this PR to solve any of the problems, but the refactoring still seems like a good idea to me 🙂
package.json
Outdated
| @@ -81,6 +81,7 @@ | |||
| "build-client-both": "CONCURRENT_BUILDS=2 concurrently -c cyan -n \"fallback ,evergreen\" \"npm run build-client-fallback\" \"npm run build-client-evergreen\"", | |||
| "build-client-if-prod": "node -e \"process.env.CALYPSO_ENV === 'production' && process.exit(1)\" || npm run build-client-both", | |||
| "build-client-if-desktop": "node -e \"(process.env.CALYPSO_ENV === 'desktop' || process.env.CALYPSO_ENV === 'desktop-development') && process.exit(1)\" || npm run build-client-fallback", | |||
| "build-client-stats": "cross-env-shell NODE_ENV=production CALYPSO_ENV=production EMIT_STATS=true PROGRESS=true npm run build-client-evergreen", | |||
There was a problem hiding this comment.
If we have issues with cross-env-shell, I think we can drop it here.
There was a problem hiding this comment.
We can drop cross-env completely, but it means dropping the remaining support for Windows that we have.
cross-env doesn't do anything useful on Unix systems: the shell already understands the ENV_VAR=value command --to-run syntax. It's useful for Windows: there, cross-env will parse the ENV_VAR=value prefixes, and sets up the environment and runs command --to-run in a way that Windows cmd.exe understands.
While removing cross-env, we could also get rid of rimraf and friends and replace them with rm -rf. That saves time where npm rimraf needs to download the package from the NPM registry.
There was a problem hiding this comment.
If we have issues with cross-env-shell, I think we can drop it here.
+1. I think it's reasonable to remove the ability for non-Unix developers to generate stats while we wait for the cross-env-shell fix.
There was a problem hiding this comment.
We can drop
cross-envcompletely, but it means dropping the remaining support for Windows that we have.
I'm not sure I'd remove it permanently, more as a temporary measure. I'm hoping that cross-env-shell will be fixed at some point, or perhaps we can find an earlier version of it without the bug.
There was a problem hiding this comment.
OK, I removed the cross-env usages in 02d3d34
The terser issue and the swallowed exit codes can bite at any time, including a production build.
matticbot
removed
the
[Status] Needs Review
In hope to get better error handling a recovery, I'm moving the commands for the
icfy-analyzeCircleCI task (thenpm run preanalyze-bundles, running thebin/analyze-icfy.jsscript, moving files to artifacts folder) to a NPM script calledanalyze-icfy. That script chains the steps using&&operator, relying on exit codes.That should be more reliable (or at least more comprehensible) than the
set +o errexitbash option.This also creates a
build-client-statsNPM script that's used by bothanalyze-bundlesandanalyze-icfyscripts.For more context, see also:
icfy-analyzeCI run wherepreanalyze-bundlesfailed with error and yet the task proceeded to runbin/icfy-analyze.jsinstead of terminating: https://circleci.com/gh/Automattic/wp-calypso/601462cross-envI found and reported when investigating the above CI failure. The CI failure doesn't seem to involve a SIGINT signal anywhere, but yet it's the only way I see how a failedwebpackbuild could lead to exit code 0.