Blocks: Make webpack.config.extensions.js extend calypso-build's

[ockham]

WIP.

Inspired by Automattic/jetpack#11802. The idea is to encapsulate complexity in @automattic/calypso-build and provide a nice enough interface to consumers.

[simison]

Does newspack need concept of "beta blocks" and outputting index.json

Could we simplify to:

/**
 * External dependencies
 */
const fs = require( 'fs' );
const getBaseWebpackConfig = require( '@automattic/calypso-build/webpack.config.js' );
const path = require( 'path' );

/**
 * Internal variables
 */
const editorSetup = path.join( __dirname, 'src', 'setup', 'editor' );
const viewSetup = path.join( __dirname, 'src', 'setup', 'view' );

function blockScripts( type, inputDir, blocks ) {
	return blocks
		.map( block => path.join( inputDir, 'blocks', block, `${ type }.js` ) )
		.filter( fs.existsSync );
}

const blocks = require( path.join( __dirname, 'src', 'setup', 'blocks.json' ) );

// Helps split up each block into its own folder view script
const viewBlocksScripts = blocks.reduce( ( viewBlocks, block ) => {
	const viewScriptPath = path.join( __dirname, 'src', 'blocks', block, 'view.js' );
	if ( fs.existsSync( viewScriptPath ) ) {
		viewBlocks[ block + '/view' ] = [ viewSetup, ...[ viewScriptPath ] ];
	}
	return viewBlocks;
}, {} );

// Combines all the different blocks into one editor.js script
const editorScript = [
	editorSetup,
	...blockScripts( 'editor', path.join( __dirname, 'src' ), blocks ),
];

const webpackConfig = getBaseWebpackConfig( null, {
	entry: {
		editor: editorScript,
		...viewBlocksScripts,
	},
	'output-path': path.join( __dirname, 'dist' ),
} );

module.exports = webpackConfig;

Alternatively ditch completely any json file and just read the src folder.

[jeffersonrabb]

Does newspack need concept of "beta blocks" and outputting index.json

I don't think Newspack needs beta blocks, at least not for the foreseeable future. If I understand correctly, Jetpack/WPCOM beta blocks were conceived primarily to allow for testing on WPCOM before a block is ready for release. If true, the concept wouldn't really be necessary since Newspack is purely a plugin. Does this thinking sound correct or am I forgetting some aspect of the rationale behind beta blocks?

[jeffersonrabb]

Alternatively ditch completely any json file and just read the src folder.

I like this approach. I tried removing src/setup/blocks.json and replacing the require() with:

const blocks = fs
  .readdirSync( path.join( __dirname, 'src', 'blocks' ) )
  .filter( block => fs.existsSync( path.join( __dirname, 'src', 'blocks', block, 'editor.js' ) ) );

How's this look?

[jeffersonrabb]

What do these recent changes mean for Babel? Can we remove babel.config.js and all of the @babel/ dependencies? I tested this, and it seemed to work, although for some reason I had to keep the following dependencies in to avoid errors:

"@babel/plugin-proposal-class-properties": "^7.4.0",
"@babel/plugin-proposal-export-default-from": "^7.2.0",
 "@babel/plugin-proposal-export-namespace-from": "^7.2.0",
"@babel/plugin-syntax-dynamic-import": "^7.2.0",
"@babel/preset-react": "^7.0.0",

[simison]

Does this thinking sound correct or am I forgetting some aspect of the rationale behind beta blocks?

Yeah, I think the need for beta blocks. came from dealing with several environments. Folks should be able to merge small PRs early on and continue working on blocks even before they're published to production. I'd imagine in a plugin like this you'd control release by simply versioning the release, while Calypso and wpcom are continuously delivered.

[jeffersonrabb]

What's the best strategy for controlling the isDevelopment variable? The output is un-minified which looks to be because isDevelopment is always going to be true outside of a Jetpack build environment. How would I override within this repo?

[jeffersonrabb]

@ockham I'm curious why you put the Internal dependencies block back in even though the only line is commented out? Was this just a copy/paste from Jetpack or was it meant to be a reminder that the workerCount line will be significant in this repo too?

[ockham]

@ockham I'm curious why you put the Internal dependencies block back in even though the only line is commented out? Was this just a copy/paste from Jetpack or was it meant to be a reminder that the workerCount line will be significant in this repo too?

Both 🙂 It's in JP too, and the reason in both cases is that I haven't entirely thought through what to do with it. 😬

[simison]

Without looking at code; does NODE_ENV=production npm run build work?

Adding --watch should work, too, as well other webpack cli stuff. (or maybe -- --watch to get pass NPM CLI.

[jeffersonrabb]

Without looking at code; does NODE_ENV=production npm run build work?

Confirmed. NODE_ENV=production npm run build -- --watch gets us minified output as well as file watching.

[ockham]

Does newspack need concept of "beta blocks" and outputting index.json

I don't think Newspack needs beta blocks, at least not for the foreseeable future. If I understand correctly, Jetpack/WPCOM beta blocks were conceived primarily to allow for testing on WPCOM before a block is ready for release. If true, the concept wouldn't really be necessary since Newspack is purely a plugin. Does this thinking sound correct or am I forgetting some aspect of the rationale behind beta blocks?

No, that pretty much sums it up. I agree that removing the beta blocks makes sense for Newspack 👍

[ockham]

Alternatively ditch completely any json file and just read the src folder.

I like this approach. I tried removing src/setup/blocks.json and replacing the require() with:

const blocks = fs
  .readdirSync( path.join( __dirname, 'src', 'blocks' ) )
  .filter( block => fs.existsSync( path.join( __dirname, 'src', 'blocks', block, 'editor.js' ) ) );

How's this look?

Looks good 👍

You might not even need that though. The original motivation for iterating over individual blocks was that we wanted to be able to build individual JS bundles (i.e. entrypoints) for each block's view.js -- but that might not be a concern for Newspack.

For a single-entrypoint bundle, with no concept of beta blocks, Newspack might not need a webpack.config.js of its own but can maybe directly use @calypso-build's. This is what o2-blocks in Calypso now do: https://github.com/Automattic/wp-calypso/blob/15955b5bced579198c3f6784a97984ff84f43245/packages/o2-blocks/package.json#L22

[ockham]

What do these recent changes mean for Babel? Can we remove babel.config.js and all of the @babel/ dependencies? I tested this, and it seemed to work, although for some reason I had to keep the following dependencies in to avoid errors:

"@babel/plugin-proposal-class-properties": "^7.4.0",
"@babel/plugin-proposal-export-default-from": "^7.2.0",
 "@babel/plugin-proposal-export-namespace-from": "^7.2.0",
"@babel/plugin-syntax-dynamic-import": "^7.2.0",
"@babel/preset-react": "^7.0.0",

We can at least heavily simplify babel.config.js by basing it on @automattic/calypso-build's (and removing those explicit dependencies), akin to Automattic/jetpack#11949

[jeffersonrabb]

You might not even need that though. The original motivation for iterating over individual blocks was that we wanted to be able to build individual JS bundles (i.e. entrypoints) for each block's view.js -- but that might not be a concern for Newspack.

For a single-entrypoint bundle, with no concept of beta blocks, Newspack might not need a webpack.config.js of its own but can maybe directly use @calypso-build's. This is what o2-blocks in Calypso now do: https://github.com/Automattic/wp-calypso/blob/15955b5bced579198c3f6784a97984ff84f43245/packages/o2-blocks/package.json#L22

I'm going to save this for a future PR.

[jeffersonrabb]

We can at least heavily simplify babel.config.js by basing it on @automattic/calypso-build's (and removing those explicit dependencies), akin to Automattic/jetpack#11949

Per discussion with @ockham, going to address Babel config reduction in a future PR.

[jeffersonrabb]

This is great. Thank you @ockham for making this possible!

[matticbot]

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀