Calypso Build Package: Add `calypso-build` bin shorthand #32295

ockham · 2019-04-16T06:55:04Z

Changes proposed in this Pull Request

Alias bin/build-packages as transpile bin. This will allow us to add more bin scripts.
Add calypso-build alias for webpack --config=webpack.config.js

Relevant docs: https://docs.npmjs.com/files/package.json#bin

Testing instructions

npm run distclean && npm ci

Verify that npm run build-packages still transpiles packages code and writes it to packages/*/dist/cjs and packages/*/dist/esm, respectively.

npx lerna run build

Verify that apps/ are built successfully.

matticbot · 2019-04-16T06:55:05Z

Test live: https://calypso.live/?branch=update/calypso-build-bins

matticbot · 2019-04-16T07:10:29Z

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

Webpack Runtime
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.

name      parsed_size    gzip_size
manifest         +0 B         +2 B  (+0.0%)

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

name   parsed_size           gzip_size
build        +16 B  (+0.0%)      +32 B  (+0.0%)

Sections
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

name         parsed_size           gzip_size
post-editor      +1110 B  (+0.1%)     +354 B  (+0.1%)
zoninator         +786 B  (+0.7%)     +261 B  (+0.9%)
reader            +324 B  (+0.1%)      +91 B  (+0.1%)
purchases         +324 B  (+0.0%)     +128 B  (+0.1%)

Async-loaded Components
React components that are loaded lazily, when a certain part of UI is displayed for the first time.

name                                         parsed_size           gzip_size
async-load-design-blocks                         +1110 B  (+0.0%)     +354 B  (+0.1%)
async-load-components-web-preview-component       +324 B  (+0.1%)      +90 B  (+0.1%)

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.

simison · 2019-04-16T07:26:24Z

docs/monorepo.md

@@ -1,5 +1,4 @@
-Publishing Packages with the Monorepo
-=====================================
+# Publishing Packages with the Monorepo


We could change this to just "Publishing with the Monorepo" now that we also have /apps

Did in https://github.com/Automattic/wp-calypso/pull/32137/files#diff-f7cb0098c1437c5ff523953cc67ca583R1

ockham · 2019-04-16T11:41:01Z

Add cb alias for webpack --config=webpack.config.js

I'm starting to wonder if we can easily do this 🤔

package.json's bin field (unlike scripts) is meant for files shipped with the package to be copied to the importing package's node_modules/.bin/, from where npm will find them when called.

However (unlike the script that we use to transpile JS), our webpack call is just that: a call to webpack, with an argument provided, and accepting other arguments. And due to our monorepo setup, we can't simply point a bin to ./node_modules/.bin/webpack, since it's not installed there, even though webpack is a dependency of calypso-build.

This leaves us with the following options:

Add our webpack invocation to scripts rather than bin. But then again, how will other packages access that?
Write a JS file to wrap around the Webpack call. This seems kind of absurd:
If we do it JS, we need to pass through all args we get. Not sure if this is easily possible with execSync? Also, simply having to use execSync is kinda gross here.
Maybe we can add a (shell-agnostic) shell script that invokes webpack? Is passing args any easier there?

simison · 2019-04-17T10:42:57Z

cc @gziolo & @mkaz since this seems related to comment in https://make.wordpress.org/core/2019/03/25/building-javascript/#comment-35581

gziolo · 2019-04-17T10:50:06Z

Write a JS file to wrap around the Webpack call. This seems kind of absurd:

If we do it JS, we need to pass through all args we get. Not sure if this is easily possible with execSync? Also, simply having to use execSync is kinda gross here.

Maybe we can add a (shell-agnostic) shell script that invokes webpack? Is passing args any easier there?

Well, this is what we do in @wordpress/scripts:

https://github.com/WordPress/gutenberg/blob/master/packages/scripts/bin/wp-scripts.js
https://github.com/WordPress/gutenberg/blob/master/packages/scripts/scripts/build.js

It's the same technique that create-react-app or ksc-scripts are using.

sirreal · 2019-04-17T16:40:38Z

I think we can get cb 😁

I needed to distclean and install to get the monorepo package bins to be picked up.

At that point, I have node_modules/.bin/cb available, meaning it's also accessible to package scripts 💥

Through some… ehem… fanciness… we essentially get a pass-through to webpack's CLI with a default --config pointing to the @automattic/calypso-build/webpack.config.js if not provided 🎉

sirreal · 2019-04-17T17:01:37Z

packages/tree-select/package.json

@@ -29,6 +29,6 @@
 	"scripts": {
 		"clean": "npx rimraf dist",
 		"prepublish": "npm run clean",
-		"prepare": "npx @automattic/calypso-build"
+		"prepare": "../calypso-build/bin/transpile.js"


Ugly but effective 😅
The npx --package @automattic/calypso-build transpile version was broken 😬

How was that broken? 🤔

(Fine to keep the ugly-but-effective version to be able to ship this soon.)

I may have been wrong and "fixed" this before doing the distclean dance to get the monorepo package bins properly installed.

Please double check me, we may be able to use npx --package @automattic/calypso-build transpile, although I suspect the change will be painful for some folks as they'll likely experience the same problem.

Wondering if #32383 might've been the actual reason for this. Seems like a stretch, but maybe some silent error when trying to build a local package along the dependency chain?

I reverted the commit since it seems to work in my testing 🤔 I'll leave it to reviewers/testers to correct me there...

packages/calypso-build/bin/cb.js

ockham · 2019-04-18T08:00:09Z

Thanks Jon, loving it! I've pushed a commit to have our apps/ use cb (which works like a charm). Going to rebase and then set to 'Needs Review'.

docs/monorepo.md

apps/o2-blocks/package.json

ockham · 2019-04-18T12:03:14Z

Finall figured out the build error (--env.WP, without ='true', can confuse webpack about the entry point, if put in the wrong place).

ockham · 2019-04-18T12:24:56Z

Somewhat related: #32389 to guard against broken o2-blocks builds.

packages/calypso-build/package.json

simison · 2019-04-19T08:19:26Z

This can probably be switched to use cb?

wp-calypso/package.json

Line 233 in 150878d

    
           "build-notifications": "webpack --config='client/notifications/webpack.config.js'",

(the whole thing should probably be in /apps but that's whole another story.)

simison

Looks fantastic and works well in my testing.

Tested:

npm ci
npm start
npm run build-notifications:

wp-calypso/package.json

Line 233 in 150878d

"build-notifications": "webpack --config='client/notifications/webpack.config.js'",
npm run build-notifications: by switching webpack to cb
npx lerna run build
npx lerna run prepare

Only standing note I left was about build-notifications

This reverts commit 5da3969d80f8713012474fb5aa6688c965a46757.

ockham · 2019-04-23T06:09:18Z

I'll rebase

Done

package.json

packages/calypso-build/bin/cb.js

sirreal · 2019-04-23T06:49:33Z

apps/full-site-editing/package.json

@@ -16,10 +16,10 @@
 	},
 	"homepage": "https://github.com/Automattic/wp-calypso",
 	"scripts": {
-		"plugin": "webpack --source='plugin'",
+		"plugin": "cb --config='./webpack.config.js' --source='plugin' --env.WP",


Do you know where these --source arguments come from? I don't see them documented in webpack CLI.

Yeah, they're a custom addition to argv in full-site-editing's Webpack config:

wp-calypso/apps/full-site-editing/webpack.config.js

Line 28 in acfc8a2

* @param {object} argv.source "plugin" or "theme"

Was also wondering if we should discourage 🤔

Was also wondering if we should discourage 🤔

Oh, that's interesting! I'd say cb itself should be very strict about adhering to the webpack CLI, but let's not prescribe how consumers should use it (yet)…

Some feedback about naming:

cb feels like something very generic, like a shell command. Specialized commands should have longer names, e.g., calypso-build.

webpack --source: again, very generic, looks like a built-in webpack option. --calypso-something-source would communicate better that it's something custom.

Thanks Jarda! I tend to agree with both points.

For a while, I was thinking that cb was a nice shorthand, and that we could use npx --package=@automattic/calypso-build cb to make sure we're getting the right command, but using that caused some rather annoying that I wasn't able to resolve easily.

Inclined to file a commit to s/cb/calypso-build/g, unless anyone objects? @sirreal?

cb feels like something very generic, like a shell command. Specialized commands should have longer names, e.g., calypso-build.

@blowery suggested this in the interest of brevity. In practice, this will likely live mostly inside package.json scripts 🤔 I'm fine either way.

webpack --source: again, very generic, looks like a built-in webpack option. --calypso-something-source would communicate better that it's something custom.

This isn't a "core" calypso-build option, but some configuration that a particular webpack config function is exposing which is why I'm not too concerned. It would be harmful if folks start copying the pattern and expect it to be "core" functionality… Perhaps it is something to discourage?

webpack --source: again, very generic, looks like a built-in webpack option. --calypso-something-source would communicate better that it's something custom.

Agree. We're using env (e.g. env.WP) to communicate Calypso-specific stuff, whereas full-size-editing deviates from that pattern. However, the --source arg has been merged prior to this PR and is thus arguably orthogonal to this one, so I'd like to change it with a separate PR.

apps/full-site-editing/package.json

apps/o2-blocks/package.json

Co-Authored-By: ockham <ockham@raz.or.at>

sirreal

This is looking great!

ockham added [Status] In Progress Build [Project] SDK labels Apr 16, 2019

ockham self-assigned this Apr 16, 2019

simison reviewed Apr 16, 2019

View reviewed changes

ockham force-pushed the update/calypso-build-bins branch from a955fb4 to a88bf81 Compare April 16, 2019 11:35

ockham requested a review from a team as a code owner April 17, 2019 16:30

sirreal reviewed Apr 17, 2019

View reviewed changes

ockham commented Apr 18, 2019

View reviewed changes

packages/calypso-build/bin/cb.js Outdated Show resolved Hide resolved

ockham changed the title ~~Calypso Build Package: Alias bin/build-packages as transpile bin~~ Calypso Build Package: Add cb bin shorthand Apr 18, 2019

simison reviewed Apr 18, 2019

View reviewed changes

docs/monorepo.md Outdated Show resolved Hide resolved

ockham force-pushed the update/calypso-build-bins branch 3 times, most recently from 2a753bd to 40cc278 Compare April 18, 2019 09:50

ockham commented Apr 18, 2019

View reviewed changes

apps/o2-blocks/package.json Outdated Show resolved Hide resolved

ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Apr 18, 2019

ockham force-pushed the update/calypso-build-bins branch from 2047809 to 619ced7 Compare April 19, 2019 07:38

simison reviewed Apr 19, 2019

View reviewed changes

packages/calypso-build/package.json Outdated Show resolved Hide resolved

simison approved these changes Apr 19, 2019

View reviewed changes

ockham added 9 commits April 23, 2019 12:04

Tighten config check in cb.js

65781e3

Fix build

408dc7f

Revert "Revert "Revert "Static paths to transpile 😬"""

100a49b

This reverts commit 5da3969d80f8713012474fb5aa6688c965a46757.

Remove ^ range

6c9f988

Shrinkwrap

0379658

Use cb to build notifications

5920f90

Just transpile 😎

c790b3f

Use cb to build full-side-editing

c15e370

Make webpack.config.js path relative

f220fac

ockham force-pushed the update/calypso-build-bins branch from 60b9d96 to f220fac Compare April 23, 2019 06:09

sirreal reviewed Apr 23, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

packages/calypso-build/bin/cb.js Outdated Show resolved Hide resolved

sirreal reviewed Apr 23, 2019

View reviewed changes

ockham added 2 commits April 23, 2019 12:56

Use current working dir's webpack.config.js

8755b59

Ugh wrong dir

e4fee9b

sirreal reviewed Apr 23, 2019

View reviewed changes

apps/full-site-editing/package.json Outdated Show resolved Hide resolved

apps/full-site-editing/package.json Outdated Show resolved Hide resolved

apps/o2-blocks/package.json Outdated Show resolved Hide resolved

sirreal and others added 5 commits April 23, 2019 13:13

Remove redundant --env.WP arg

d009155

Co-Authored-By: ockham <ockham@raz.or.at>

Remove redundant --env.WP arg

b23ba67

Co-Authored-By: ockham <ockham@raz.or.at>

Calypso Build: Change default outputPath

bd0cb18

Remove redundant --output-path='./dist'' arg

56f9398

s/cb/calypso-bin/g

334f224

ockham force-pushed the update/calypso-build-bins branch from 4d32168 to 334f224 Compare April 23, 2019 07:20

ockham changed the title ~~Calypso Build Package: Add cb bin shorthand~~ Calypso Build Package: Add calypso-build bin shorthand Apr 23, 2019

sirreal approved these changes Apr 23, 2019

View reviewed changes

ockham merged commit 925d1c7 into master Apr 23, 2019

ockham deleted the update/calypso-build-bins branch April 23, 2019 07:55

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

Copons unassigned ockham Apr 23, 2019

Copons mentioned this pull request Apr 23, 2019

Full Site Editing: Update the Content Slot block #32529

Merged

mattwiebe mentioned this pull request Apr 24, 2019

Full Site Editing: add the wp_template post_type #32568

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calypso Build Package: Add `calypso-build` bin shorthand #32295

Calypso Build Package: Add `calypso-build` bin shorthand #32295

ockham commented Apr 16, 2019 •

edited

Loading

matticbot commented Apr 16, 2019

matticbot commented Apr 16, 2019 •

edited

Loading

simison Apr 16, 2019

simison Apr 16, 2019

ockham commented Apr 16, 2019 •

edited by sirreal

Loading

simison commented Apr 17, 2019

gziolo commented Apr 17, 2019

sirreal commented Apr 17, 2019

sirreal Apr 17, 2019 •

edited

Loading

ockham Apr 18, 2019

ockham Apr 18, 2019

sirreal Apr 18, 2019

ockham Apr 18, 2019

ockham Apr 18, 2019

ockham commented Apr 18, 2019

ockham commented Apr 18, 2019

ockham commented Apr 18, 2019

simison commented Apr 19, 2019 •

edited

Loading

simison left a comment

ockham commented Apr 23, 2019

sirreal Apr 23, 2019

ockham Apr 23, 2019

sirreal Apr 23, 2019

jsnajdr Apr 23, 2019

ockham Apr 23, 2019 •

edited by sirreal

Loading

sirreal Apr 23, 2019

ockham Apr 23, 2019

sirreal left a comment

Calypso Build Package: Add calypso-build bin shorthand #32295

Calypso Build Package: Add calypso-build bin shorthand #32295

Conversation

ockham commented Apr 16, 2019 • edited Loading

Changes proposed in this Pull Request

Testing instructions

matticbot commented Apr 16, 2019

matticbot commented Apr 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham commented Apr 16, 2019 • edited by sirreal Loading

simison commented Apr 17, 2019

gziolo commented Apr 17, 2019

sirreal commented Apr 17, 2019

sirreal Apr 17, 2019 • edited Loading

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

ockham commented Apr 18, 2019

ockham commented Apr 18, 2019

ockham commented Apr 18, 2019

simison commented Apr 19, 2019 • edited Loading

simison left a comment

Choose a reason for hiding this comment

ockham commented Apr 23, 2019

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

ockham Apr 23, 2019 • edited by sirreal Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirreal left a comment

Choose a reason for hiding this comment

Calypso Build Package: Add `calypso-build` bin shorthand #32295

Calypso Build Package: Add `calypso-build` bin shorthand #32295

ockham commented Apr 16, 2019 •

edited

Loading

matticbot commented Apr 16, 2019 •

edited

Loading

ockham commented Apr 16, 2019 •

edited by sirreal

Loading

sirreal Apr 17, 2019 •

edited

Loading

simison commented Apr 19, 2019 •

edited

Loading

ockham Apr 23, 2019 •

edited by sirreal

Loading