SDK: Shuffle around configuration in SDK runner #26680

dmsnell · 2018-08-14T08:14:37Z

When building with the SDK we have a generic runner - sdk-cli.js
and target-specific options for that runner - gutenberg.js. We
invoke the runner script while specifying the target which it calls.

Some of our configuration and runtime code though has been split
across these two places and some of the configuration could maybe
be moved upwards into the main Calypso config.

In this patch I'm shuffling around the configuration and execution
code to try and separate those layers more clearly. This work serves
to prepare us as well for integrating other projects, such as the
notifications panel, into the Calpyso repository.

Testing

Everything needs to build without regression.

The results in iscalypsofastyet.com should show no change in
the output size of the app. The same is true of built plugins for
Gutenberg that were built with this script.

We need to verify that the plugins run inside of Gutenberg too.

npm run sdk:gutenberg -- --editor-script client/gutenberg/extensions/presets/o2/editor.js --view-script client/gutenberg/extensions/presets/o2/view.js --output-dir ~/wherever/

matticbot · 2018-08-14T08:14:38Z

Test live: https://calypso.live/?branch=sdk/shuffle-runner-config

simison · 2018-08-14T14:29:56Z

bin/sdk/gutenberg.js


-	const config = {
+	return {
 		...baseConfig,
 		...{
 			context: __rootDir,


Moving context: __rootDir, to sdk-cli.js would probably make sense since it needs to be there each time sdk-cli.js is run via bin scripts.

Thus we also don't need to pass __rootDir down to gutenberg.js

thanks. I wondered about that but didn't feel confident enough on my own to move it

We could even consider moving it to webpack.config.js if it's not too much noise there, although it's relevant only for the SDK CLI.

simison · 2018-08-14T15:10:43Z

Nice job!

The only regression I see is lack of --mode=production|development in CLI but suppose we'll build like so in future —

Production:

NODE_ENV=production calypso-sdk gutenberg --editor-script=client/gutenberg/extensions/hello-dolly/hello-block.js

Development:

calypso-sdk gutenberg --editor-script=client/gutenberg/extensions/hello-dolly/hello-block.js

That works for me, as long as development is the default. 👍

tyxla · 2018-08-15T08:23:09Z

webpack.config.js

+/**
+ * Converts @wordpress require into window reference
+ *
+ * Note this isn't the same as camelCase becuase of the


Typo here: becuase should be because. Also, perhaps we want to change camelCase to _.camelCase to be more specific.

thanks for the typo. interestedly though I wasn't referring to _.camelCase. maybe I should write camel case instead? I wanted to refer to the algorithm, not the lodash or underscore implementation…

camel case should be even better as it's more broad indeed 👍

space added!

tyxla · 2018-08-15T08:33:33Z

webpack.config.js

+const wordpressRequire = request => {
+	const [ , /* @wordpress */ name ] = request.split( '/' );
+
+	return `wp.${ name.replace( /-([a-z])/, ( match, letter ) => letter.toUpperCase() ) }`;


I see that you've removed the global flag /g, but this will not work as expected for imports that contain more than one dash in the second part of the name. In other words @wordpress/block-serialization-spec-parser will become wp.blockSerialization-spec-parser. We should bring back the global flag.

tyxla · 2018-08-15T08:50:26Z

webpack.config.js

+ * @return {string} global variable reference for import
+ */
+const wordpressRequire = request => {
+	const [ , /* @wordpress */ name ] = request.split( '/' );


Should /* @wordpress */ be listed before the comma?

yes but prettier moves it back - a long-standing bug sadly :(

adjusted to it's clearer

tyxla · 2018-08-15T08:52:44Z

bin/sdk-cli.js

+					description: 'Entry for editor-side JavaScript file',
+					type: 'string',
+					required: true,
+					coerce: value => path.resolve( __dirname, '../', value ),


Not related with the PR, but occurred to me now: the *-script args use the same coerce callback - should we define it separately and use it for both?

🤷‍♂️ is the point to not write it twice? I don't understand coerce so I don't have a real opinion on the matter.

It's not about coerce - it's just about not defining the same callback twice when we can define it just once and just reuse it.

meh. it's an inline callback used twice. depending on how I feel when you ask me I'd probably suggest leaving it for now. if we discover that it's an important part that will need to be shared across many different places and the behavior needs to be the same for all of them then yeah I could be for that. at this point I'm not sure we have enough warrant to make the abstraction

Agreed, sometimes writing one line twice makes the code more clear. :-)

coerse docs if you wanna check it out: https://github.com/yargs/yargs/blob/efc0970bc8f91359905882b6990ffc0786193068/docs/api.md#coercekey-fn @dmsnell

tyxla · 2018-08-15T08:58:33Z

webpack.config.js

@@ -163,10 +187,6 @@ function getWebpackConfig( { extensionName = '' } = {}, argv ) {
 					use: _.compact( [
 						MiniCssExtractPlugin.loader,
 						'css-loader',
-						extensionName && {


Are we intentionally removing the namespacing?

after removing it no, but temporarily in this diff we were

When building with the SDK we have a generic runner - `sdk-cli.js` and target-specific options for that runner - `gutenberg.js`. We invoke the runner script while specifying the target which it calls. Some of our configuration and runtime code though has been split across these two places and some of the configuration could maybe be moved upwards into the main Calypso config. In this patch I'm shuffling around the configuration and execution code to try and separate those layers more clearly. This work serves to prepare us as well for integrating other projects, such as the notifications panel, into the Calpyso repository.

tyxla

Nice work 👍 Looking good and testing as expected.

ockham · 2018-08-16T09:59:51Z

webpack.config.js

@@ -260,7 +292,7 @@ function getWebpackConfig() {
 					},
 				} ),
 		] ),
-		externals: [ 'electron' ],
+		externals: _.compact( [ externalizeWordPressPackages && wordpressExternals, 'electron' ] ),


TIL externals can be functions

tyxla · 2018-08-16T13:03:16Z

bin/sdk/gutenberg.js

-				...baseConfig.externals,
-				...getWordPressExternals(),
-				lodash: 'lodash',
-				wp: 'wp',


Seems like we forgot to account for externalizing wp - see #26725 for a fix.

dmsnell added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build [Project] SDK labels Aug 14, 2018

dmsnell requested a review from simison August 14, 2018 08:14

dmsnell requested review from ockham, ehg, coderkevin, tyxla and a team August 14, 2018 08:14

simison reviewed Aug 14, 2018

View reviewed changes

tyxla reviewed Aug 15, 2018

View reviewed changes

tyxla mentioned this pull request Aug 15, 2018

SDK: refactor block CSS name-spacing #26662

Merged

dmsnell force-pushed the sdk/shuffle-runner-config branch from 53217a5 to a4b01f3 Compare August 15, 2018 10:49

dmsnell added 3 commits August 15, 2018 12:36

small updates to shuffling script

8fef6e3

fix typo

7bd000e

move context to top

7e2a572

tyxla approved these changes Aug 15, 2018

View reviewed changes

dmsnell merged commit 5238751 into master Aug 15, 2018

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

dmsnell deleted the sdk/shuffle-runner-config branch August 15, 2018 13:51

ockham reviewed Aug 16, 2018

View reviewed changes

tyxla mentioned this pull request Aug 16, 2018

SDK: Fix externalization of the wp global variable #26725

Merged

tyxla reviewed Aug 16, 2018

View reviewed changes

simison mentioned this pull request Apr 5, 2019

Calypso build: No fixed context #32045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDK: Shuffle around configuration in SDK runner #26680

SDK: Shuffle around configuration in SDK runner #26680

dmsnell commented Aug 14, 2018

matticbot commented Aug 14, 2018

simison Aug 14, 2018

dmsnell Aug 14, 2018

simison Aug 15, 2018

dmsnell Aug 15, 2018

simison commented Aug 14, 2018

tyxla Aug 15, 2018

dmsnell Aug 15, 2018

tyxla Aug 15, 2018

dmsnell Aug 15, 2018

tyxla Aug 15, 2018

dmsnell Aug 15, 2018

dmsnell Aug 15, 2018

tyxla Aug 15, 2018

dmsnell Aug 15, 2018

dmsnell Aug 15, 2018

tyxla Aug 15, 2018

dmsnell Aug 15, 2018

tyxla Aug 15, 2018

dmsnell Aug 15, 2018

simison Aug 15, 2018

tyxla Aug 15, 2018

dmsnell Aug 15, 2018

tyxla left a comment

ockham Aug 16, 2018

tyxla Aug 16, 2018

SDK: Shuffle around configuration in SDK runner #26680

SDK: Shuffle around configuration in SDK runner #26680

Conversation

dmsnell commented Aug 14, 2018

matticbot commented Aug 14, 2018

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

simison commented Aug 14, 2018

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

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

Choose a reason for hiding this comment

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment