Simplify or remove several package.json scripts #38931

jsnajdr · 2020-01-19T17:43:46Z

Bunch of simplifications of the npm scripts in package.json. Followup to #38190 that fixes plenty of things.

Eliminate the need for setting NODE_PATH when building or running Calypso
It was needed only to require two specific files (webpack config for webpack dev server and search index for devdocs). Updated the webpack externals definition to remove that need.

Fix the NODE_ARGS argument in the start-build scripts
node NODE_ARGS=... is an error that fails the script invocation:

Error: Cannot find module '/Users/jsnajdr/src/wp-desktop/calypso/NODE_ARGS=--max_old_space_size=8192'

We want to invoke node with $NODE_ARGS arguments and supply a default.

Cc @nsakaimbo who introduced the regression in #38576.

Validate fallback build in a separate post* script
Makes lines in package.json shorter 🙂 Cc @sgomes

Update path to stats.json file in whybundled script
The (rarely used) npm run whybundled didn't have a correct path to stats.json (inside client/ since the monorepo-ization)

Update clean scripts
Updates the paths of generated files to remove (server/ to client/server/), uses run-s to run all clean:* scripts at once.

Remove the analyze-css NPM script
Finishes the removal started in #27979.

matticbot · 2020-01-19T17:43:49Z

Test live: https://calypso.live/?branch=update/package-json-bits

matticbot · 2020-01-19T17:57:44Z

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

Webpack Runtime (~2055 bytes added 📈 [gzipped])

name      parsed_size            gzip_size
manifest     -38661 B  (-22.8%)    +2055 B  (+6.6%)

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

name                 parsed_size            gzip_size
entry-main             -108960 B   (-7.1%)   -31451 B   (-8.5%)
entry-login            -108960 B  (-10.7%)   -31451 B  (-11.8%)
entry-gutenboarding    -108960 B   (-5.7%)   -31451 B   (-6.2%)

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

Sections (~685619 bytes added 📈 [gzipped])

name                   parsed_size             gzip_size
checkout                 +800867 B   (+70.9%)  +187786 B   (+67.2%)
woocommerce              -748896 B   (-36.5%)  -171177 B   (-31.9%)
settings                 +741275 B  (+146.4%)  +182042 B  (+140.4%)
post-editor              -492277 B   (-24.7%)  -127002 B   (-23.5%)
settings-performance     +260268 B  (+127.7%)   +57388 B  (+105.5%)
settings-writing         +255011 B   (+58.9%)   +63147 B   (+58.3%)
settings-security        +208452 B   (+80.2%)   +48203 B   (+70.7%)
settings-discussion      +201938 B  (+121.3%)   +46747 B  (+110.1%)
home                     +195574 B   (+73.8%)   +48069 B   (+68.9%)
site-blocks              +149695 B   (+43.2%)   +34509 B   (+39.4%)
privacy                  +149695 B   (+71.8%)   +34509 B   (+62.8%)
notification-settings    +149695 B   (+59.6%)   +34509 B   (+52.1%)
me                       +149695 B   (+40.0%)   +34509 B   (+41.0%)
happychat                +149695 B   (+49.5%)   +34509 B   (+44.5%)
checklist                +149695 B   (+52.1%)   +34509 B   (+45.6%)
account-close            +149695 B   (+53.3%)   +34509 B   (+46.2%)
account                  +149695 B   (+49.4%)   +34509 B   (+42.7%)
security                 +140017 B   (+34.1%)   +35786 B   (+33.5%)
marketing                +135028 B   (+31.6%)   +22927 B   (+21.6%)
help                     +131185 B   (+28.4%)   +29308 B   (+25.5%)
jetpack-connect          -106158 B   (-19.5%)   -26446 B   (-18.4%)
reader                    +98230 B   (+25.0%)   +22135 B   (+21.5%)
signup                    +53315 B   (+33.6%)   +15397 B   (+38.6%)
plugins                   -52857 B   (-11.2%)   -12899 B   (-10.7%)
plans                     -42340 B   (-10.0%)    -9760 B    (-8.9%)
google-my-business        +34020 B   (+13.2%)    +9245 B   (+13.1%)
theme                     -30863 B   (-12.3%)    -7392 B   (-11.0%)
domains                   -27182 B    (-3.2%)    -3298 B    (-1.7%)
stats                     -19923 B    (-2.3%)     +974 B    (+0.5%)
zoninator                 +12201 B    (+4.7%)    +4421 B    (+6.3%)
purchases                 -11302 B    (-1.2%)    -3145 B    (-1.4%)
export                     +8950 B    (+5.3%)    +1037 B    (+2.3%)
themes                     -6036 B    (-1.9%)    -3715 B    (-4.4%)
hosting                    +3517 B    (+1.5%)     -231 B    (-0.4%)

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

name                                         parsed_size            gzip_size
async-load-design-playground                   -685023 B  (-37.8%)  -163574 B  (-39.8%)
async-load-design                              -685023 B  (-36.7%)  -163574 B  (-38.1%)
async-load-design-blocks                       -671808 B  (-24.2%)  -160718 B  (-25.0%)
async-load-components-web-preview-component     +20227 B   (+5.8%)    +3696 B   (+4.1%)
async-load-reader-search-stream                 -19968 B  (-23.4%)    -3263 B  (-14.9%)
async-load-reader-following-manage              -14834 B  (-13.3%)    -1056 B   (-3.6%)

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.

nsakaimbo · 2020-01-19T20:07:26Z

Cc @nsakaimbo who introduced the regression in #38576.

Technically, #38576 did not cause a regression. This initial code change was necessary to address changes to memory constraints for wp-desktop that were introduced by Node 12. This change wasn’t an issue until the conversion of the Calypso client into a lerna package.

Regardless - I think it’s worth calling out the language used here. We shouldn’t encourage a culture of finger-pointing. Not saying this was anyone’s intention, but such phrasing could easily be construed this way. We all work together on the same team. Moving forward, we should all (myself included) be mindful of the language we use when solving these issues as they come up.

jsnajdr · 2020-01-19T22:24:22Z

Hi @nsakaimbo 👋 I apologize for not being careful about the phrasing I used. I wanted to call your attention to the part of the diff where your review would be valuable, not to assign blame to you or anyone else. I agree that would be unproductive and pointless.

I still believe that #38576 introduced a change into the start-build-fallback script that probably wasn't fully intended. While the old code:

node $NODE_ARGS --max_old_space_size=8192 build/bundle.js

runs the build/bundle.js script and "only" fails to use max_old_space_size passed in NODE_ARGS, the patched code:

node NODE_ARGS=${NODE_ARGS:---max_old_space_size=8192} build/bundle.js

fails to run the script at all, as it passes the NODE_ARGS=... string as a first argument and Node interprets it as a script filename. The result (when running make dev-server in wp-desktop that subsequently invokes the start-build-fallback script in Calypso) is the error I mentioned above:

Error: Cannot find module '/Users/jsnajdr/src/wp-desktop/calypso/NODE_ARGS=--max_old_space_size=8192'

sgomes · 2020-01-20T11:28:34Z

Validate fallback build in a separate post* script
Makes lines in package.json shorter 🙂 Cc @sgomes

Splitting that out into its own script sounds good to me! It should help contain changes too, as I believe newer eslint versions will require more parameters to avoid errors.

Update path to stats.json file in whybundled script
The (rarely used) npm run whybundled didn't have a correct path to stats.json (inside client/ since the monorepo-ization)

That's actually one of my favourite and most often used tools in bringing down bundle sizes, so thank you for fixing that! 🙂

package.json

sgomes · 2020-01-21T13:34:00Z

package.json

-		"build-client-evergreen": "cross-env-shell BROWSERSLIST_ENV=evergreen NODE_PATH=$NODE_PATH:server:client:. node $NODE_ARGS ./node_modules/webpack/bin/webpack.js --config client/webpack.config.js --display errors-only",
+		"build-client-fallback": "cross-env-shell BROWSERSLIST_ENV=defaults node $NODE_ARGS ./node_modules/webpack/bin/webpack.js --config client/webpack.config.js --display errors-only",
+		"postbuild-client-fallback": "npm run validate-fallback-es5",
+		"build-client-evergreen": "cross-env-shell BROWSERSLIST_ENV=evergreen node $NODE_ARGS ./node_modules/webpack/bin/webpack.js --config client/webpack.config.js --display errors-only",
 		"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)\" || cross-env-shell NODE_ARGS=${NODE_ARGS:---max_old_space_size=8192} npm run build-client-both",


I know this code was already here, but why do some scripts (like this one) extend NODE_ARGS, while others (like preanalyze-bundles and prewhybundled) rewrite it?

why do some scripts extend NODE_ARGS, while others rewrite it?

That's a good point and I'm cleaning up NODE_ARGS in 42fb4f6 and further consolidating the build scripts in 1d99184

After these changes, the scripts set and use the NODE_ARGS variable only at the place where node is actually spawned to run webpack or the server bundle -- build-client-do and start-bundle-do, respectively.

No other tasks set max_old_space_size now. The default value is OK for everyone, and there's also a way (used by wp-desktop) to override it by setting the NODE_ARGS environment variable.

The build-client-do consolidation works as follows: instead of two NPM tasks:

"build-client-fallback": "cross-env-shell BROWSERSLIST_ENV=fallback node bin/webpack [lot of webpack and node arguments]" "build-client-evergreen": "cross-env-shell BROWSERSLIST_ENV=evergreen node bin/webpack [lot of webpack and node arguments]"

there are three and duplication is removed:

"build-client-do": "node bin/webpack [lot of webpack and node arguments]" "build-client-fallback": "cross-env-shell BROWSERSLIST_ENV=fallback npm run build-client-do" "build-client-evergreen": "cross-env-shell BROWSERSLIST_ENV=evergreen npm run build-client-do"

That's a great solution, thank you! 👍

I'd like to try out the NODE_OPTIONS environment variable, too. It's supported directly by Node.js, and was added only in 8.x, which is not that ancient. We probably started setting max_old_space_size at a time when NODE_OPTIONS wasn't available yet.

sgomes · 2020-01-21T13:35:48Z

package.json

-		"clean": "npm run -s clean:build && npm run -s clean:devdocs && npm run -s clean:public && npm run -s clean:packages",
-		"clean:build": "npx rimraf build server/bundler/*.json .babel-cache",
-		"clean:devdocs": "npx rimraf server/devdocs/search-index.js",
+		"clean": "run-s -s clean:*",


Very nice 👍

run-p might be even faster for those of us with SSDs

I considered run-p, too, but was afraid of race conditions like npx rimraf doing the download and install two times in parallel.

nsakaimbo · 2020-01-21T21:52:39Z

@jsnajdr Thanks for handling my feedback in a constructive and positive way. I figured it would be better to err on the side of being transparent about any potential misunderstandings, and glad to hear we're on the same page. 😄

I appreciate the breakdown of the underlying issue - I was definitely unaware this change inadvertently borked the build/bundle.js script 🤔(CI passed when this change was PR'd and I assumed all was good). Your changes here look good, and CI for wp-desktop seems happy with the fix. Does this mean we can also omit NODE_PATH from the desktop Makefile?

https://github.com/Automattic/wp-desktop/blob/124c942230f2f4c57e0497716718fdf14d8b6ae0/Makefile#L106

Finishes the removal started in #27979.

… invoked Updates the paths of generated files to remove (`server/` to `client/server/`), use `run-s` to run all `clean:*` scripts at once.

Makes lines in `package.json` shorter 🙂

`node NODE_ARGS=...` is an error that fails the script invocation: ``` Error: Cannot find module '/Users/jsnajdr/src/wp-desktop/calypso/NODE_ARGS=--max_old_space_size=8192' ``` We want to invoke `node` with `$NODE_ARGS` arguments and supply a default.

…ypso It was needed only to `require` two specific files (webpack config for webpack dev server and search index for devdocs). Updated the webpack `externals` definition to remove that need.

…at runs webpack

…ross-env and a *-do subtask

jsnajdr · 2020-01-22T14:28:40Z

I was definitely unaware this change inadvertently borked the build/bundle.js script 🤔(CI passed when this change was PR'd and I assumed all was good)

@nsakaimbo As far as I know, the start-build-fallback script is ever run by start-build-if-desktop when running in the desktop-development mode. You'd need to run make dev-server in wp-desktop/ to trigger this. The production builds and all the CI tasks run in different code paths.

Does this mean we can also omit NODE_PATH from the desktop Makefile?

Yes, that should be possible with one caveat: you'll need to update externals definitions in the webpack config:

https://github.com/Automattic/wp-desktop/blob/develop/webpack.shared.js#L76-L78

so that they are mapped to paths relative to the current working directory when running the server bundle. See the changes that this PR does to the client/webpack.config.node.js file.

The CALYPSO_SERVER=true variable can also be removed from the desktop Makefile and be replaced with browserslist configuration. At this moment, I suspect that the server bundle for desktop (ran by the Node.js server inside Electron) is transpiled with the target being the default browserslist configuration, and that likely adds too many polyfills.

sgomes

LGTM! Thank you for fixing these issues!

package.json

blowery · 2020-01-27T15:42:23Z

package.json

@@ -86,8 +87,9 @@
 		"start": "npm run -s build",
 		"poststart": "run-p -s start-build-if-web start-build-if-desktop build-packages:watch",
 		"reformat-files": "./node_modules/.bin/prettier --ignore-path .eslintignore --write \"**/*.{js,jsx,json,ts,tsx}\"",
-		"start-build-fallback": "cross-env-shell BROWSERSLIST_ENV=defaults NODE_PATH=$NODE_PATH:server:client:. node NODE_ARGS=${NODE_ARGS:---max_old_space_size=8192} build/bundle.js",
-		"start-build-evergreen": "cross-env-shell BROWSERSLIST_ENV=evergreen NODE_PATH=$NODE_PATH:server:client:. node $NODE_ARGS --max_old_space_size=8192 build/bundle.js",
+		"start-build-do": "cross-env-shell node ${NODE_ARGS:---max_old_space_size=8192} build/bundle.js",


i had no idea this syntax was possible ${NODE_ARGS:---max_old_space_size=8192}

is that a cross-env-shell thing or a npm scripts thing?

I think it may be missing NODE_ARGS= beforehand, judging by the other lines.

It's a bash syntax for fancy parameter expansion and setting default values. See the "Default Values" section in https://devhints.io/bash#parameter-expansions

cross-env-shell does some transforms to the bash-like command line that make it compatible with Windows cmd.exe. But it won't understand the ${FOO:-val} syntax and this will be one of many things broken in Windows build.

I removed cross-env-shell from this particular script, as it doesn't set any environment variables.

I think it may be missing NODE_ARGS= beforehand, judging by the other lines.

NODE_ARGS= is needed only if the caller wants to add some Node.js command line options, or override the default value of max_old_space_size. Currently, the only place that does this is the wp-desktop build.

blowery · 2020-01-27T15:43:06Z

package.json

@@ -111,8 +113,8 @@
 		"typecheck": "tsc --noEmit",
 		"update-deps": "npx rimraf package-lock.json && npm run -s distclean && npm i && replace --silent 'http://' 'https://' . --recursive --include='package-lock.json' && touch -m node_modules",
 		"validate-fallback-es5": "npx eslint --parser-options=ecmaVersion:5 --no-eslintrc --no-ignore ./public/fallback/*.js",
-		"prewhybundled": "cross-env-shell CALYPSO_ENV=production EMIT_STATS=withreasons NODE_ARGS=--max_old_space_size=8192 npm run -s build-client",
-		"whybundled": "whybundled stats.json",
+		"prewhybundled": "cross-env-shell CALYPSO_ENV=production EMIT_STATS=withreasons npm run -s build-client-evergreen",


is there a way to skip this if we already have a client/stats.json?

Not easily, this is not Makefile 🤣

blowery

couple little nits, but this is good progress! LGTM

… vars

sirreal · 2020-01-27T17:24:40Z

package.json

@@ -111,8 +113,8 @@
 		"typecheck": "tsc --noEmit",
 		"update-deps": "npx rimraf package-lock.json && npm run -s distclean && npm i && replace --silent 'http://' 'https://' . --recursive --include='package-lock.json' && touch -m node_modules",
 		"validate-fallback-es5": "npx eslint --parser-options=ecmaVersion:5 --no-eslintrc --no-ignore ./public/fallback/*.js",
-		"prewhybundled": "cross-env-shell CALYPSO_ENV=production EMIT_STATS=withreasons NODE_ARGS=--max_old_space_size=8192 npm run -s build-client",
-		"whybundled": "whybundled stats.json",
+		"prewhybundled": "cross-env-shell CALYPSO_ENV=production EMIT_STATS=withreasons npm run -s build-client-evergreen",


I don't think we need to worry about x-platform support here, we can do something like the following:

[[ -f client/stats.json ]] && echo '`client/stats.json exists, skipping generation. To rebuild, remove the stats.json file.' || CALYPSO_ENV=production EMIT_STATS=withreasons npm run -s build-client-evergreen

Do bear in mind that the stats file may already exist but not be the one we want, e.g. if npm run analyze-bundles was previously run (that one doesn't contain reasons). If we're going to do this, we should rename the file to stats-with-reasons.json to make it very clear this is the one we want.

Also, not sure how we should handle staleness. Presumably the developer will be able to tell that it took no time at all and thus it was an existing, possibly stale file?

jsnajdr added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build [Type] Janitorial labels Jan 19, 2020

jsnajdr requested review from ockham, sgomes, sirreal, nsakaimbo and a team January 19, 2020 17:43

jsnajdr self-assigned this Jan 19, 2020

sgomes reviewed Jan 21, 2020

View reviewed changes

jsnajdr added 9 commits January 22, 2020 13:47

Remove the analyze-css NPM script

1197c7c

Finishes the removal started in #27979.

Update clean scripts: paths of generated files, simplify how they are…

805ac37

… invoked Updates the paths of generated files to remove (`server/` to `client/server/`), use `run-s` to run all `clean:*` scripts at once.

Update path to stats.json file in whybundled script

42a3da0

Validate fallback build in a separate script

68aa95b

Makes lines in `package.json` shorter 🙂

Eliminate the need for setting NODE_PATH when building or running Cal…

d9e29d3

…ypso It was needed only to `require` two specific files (webpack config for webpack dev server and search index for devdocs). Updated the webpack `externals` definition to remove that need.

Update path to stats.json file in analyze-bundles script

c4d16e7

Consolidate setting max_old_space_size to the build-client scripts th…

42fb4f6

…at runs webpack

Remove duplication from build-client and start-build tasks by using c…

1d99184

…ross-env and a *-do subtask

jsnajdr force-pushed the update/package-json-bits branch from 5bd0639 to 1d99184 Compare January 22, 2020 13:55

sgomes self-requested a review January 27, 2020 15:37

sgomes approved these changes Jan 27, 2020

View reviewed changes

blowery reviewed Jan 27, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

blowery reviewed Jan 27, 2020

View reviewed changes

Settle on PROGRESS=true as the truth marker

d37f01e

blowery reviewed Jan 27, 2020

View reviewed changes

blowery approved these changes Jan 27, 2020

View reviewed changes

Remove cross-env-shell from start-build-do, as it doesn't set any env…

c976568

… vars

jsnajdr merged commit 561a700 into master Jan 27, 2020

jsnajdr deleted the update/package-json-bits branch January 27, 2020 16:39

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 27, 2020

sirreal reviewed Jan 27, 2020

View reviewed changes

jsnajdr mentioned this pull request Jan 29, 2020

Improve reliability of the npm clean script by avoiding run-s #39131

Merged

sirreal mentioned this pull request Jan 30, 2020

Docker: Remove NODE_PATH #39159

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify or remove several package.json scripts #38931

Simplify or remove several package.json scripts #38931

jsnajdr commented Jan 19, 2020

matticbot commented Jan 19, 2020

matticbot commented Jan 19, 2020 •

edited

Loading

nsakaimbo commented Jan 19, 2020

jsnajdr commented Jan 19, 2020 •

edited

Loading

sgomes commented Jan 20, 2020

sgomes Jan 21, 2020

jsnajdr Jan 22, 2020

sgomes Jan 22, 2020

jsnajdr Jan 22, 2020

sgomes Jan 21, 2020

blowery Jan 21, 2020

jsnajdr Jan 22, 2020

nsakaimbo commented Jan 21, 2020

jsnajdr commented Jan 22, 2020

sgomes left a comment

blowery Jan 27, 2020

sgomes Jan 27, 2020

jsnajdr Jan 27, 2020

jsnajdr Jan 27, 2020

jsnajdr Jan 27, 2020

blowery Jan 27, 2020

jsnajdr Jan 27, 2020

blowery left a comment

sirreal Jan 27, 2020

sgomes Jan 27, 2020 •

edited

Loading

Simplify or remove several package.json scripts #38931

Simplify or remove several package.json scripts #38931

Conversation

jsnajdr commented Jan 19, 2020

matticbot commented Jan 19, 2020

matticbot commented Jan 19, 2020 • edited Loading

nsakaimbo commented Jan 19, 2020

jsnajdr commented Jan 19, 2020 • edited Loading

sgomes commented Jan 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsakaimbo commented Jan 21, 2020

jsnajdr commented Jan 22, 2020

sgomes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blowery left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgomes Jan 27, 2020 • edited Loading

Choose a reason for hiding this comment

matticbot commented Jan 19, 2020 •

edited

Loading

jsnajdr commented Jan 19, 2020 •

edited

Loading

sgomes Jan 27, 2020 •

edited

Loading