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.

Sign up for GitHub

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

Move code of the icfy-analyze CircleCI task to a NPM script #39502

Merged
merged 2 commits into from
Feb 18, 2020
Merged

Move code of the icfy-analyze CircleCI task to a NPM script #39502

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04a41a9
Move code of the icfy-analyze CircleCI task to a NPM script
jsnajdr Feb 18, 2020
02d3d34
Remove usage of cross-env from build scripts until its exit code bugs…
jsnajdr Feb 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,11 +475,7 @@ jobs:
environment:
NODE_ENV: 'production'
BROWSERSLIST_ENV: 'defaults'
command: |
set +o errexit
npm run preanalyze-bundles
node --max-old-space-size=4096 bin/icfy-analyze.js
mv client/stats.json client/chart.json "$CIRCLE_ARTIFACTS/icfy"
command: npm run analyze-icfy
- save_cache: *save-terser-cache
- save_cache: *save-babel-client-cache
- store-artifacts-and-test-results
Expand Down
5 changes: 3 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@
"npm": "^6.12.1"
},
"scripts": {
"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\"",
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 🙂

"autoprefixer": "postcss -u autoprefixer -r --no-map --config packages/calypso-build/postcss.config.js",
"postcss": "postcss -r --config packages/calypso-build/postcss.config.js",
"prebuild": "npm run install-if-deps-outdated",
Expand All @@ -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",
Copy link
Member

Choose a reason for hiding this comment

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

If we have issues with cross-env-shell, I think we can drop it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@sgomes sgomes Feb 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can drop cross-env completely, 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

"build-packages": "npx lerna run prepare --stream",
"build-packages:watch": "chokidar \"./packages/**\" -i \"./packages/*/dist/**\" -i \"**.css\" -i \"**.d.ts\" --debounce 1000 --throttle 10000 -c \"npm run build-packages\"",
"clean": "npm run clean:packages && npm run clean:build && npm run clean:public",
Expand Down