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

Jump to bottom

Create Block: Simplify the process of defining a config for templates #22235

Merged
merged 5 commits into from
May 13, 2020
Merged

Create Block: Simplify the process of defining a config for templates #22235

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b751a29
Create Block: Use glob to find all template files
gziolo May 8, 2020
7a882d8
Create block: Refactor template handling
gziolo May 11, 2020
a690676
Move output files templates to getBlockTemplate
gziolo May 11, 2020
0565d15
Add CHANGELOG entry
gziolo May 11, 2020
41c105f
Apply suggestions from code review
gziolo May 13, 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
1 change: 1 addition & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions packages/create-block/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Internal

- Refactored handling of predefined block templates [#22235](https://github.com/WordPress/gutenberg/pull/22235).

## 0.12.0 (2020-04-30)

### New Features
Expand Down
2 changes: 1 addition & 1 deletion packages/create-block/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ $ npm init @wordpress/block [options] [slug]
Options:
```
-V, --version output the version number
-t, --template <name> template type name, allowed values: "es5", "esnext" (default: "esnext")
-t, --template <name> block template type name, allowed values: "es5", "esnext" (default: "esnext")
--namespace <value> internal namespace for the block name
--title <value> display title for the block
--short-description <value> short description for the block
Expand Down
31 changes: 21 additions & 10 deletions packages/create-block/lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ const CLIError = require( './cli-error' );
const log = require( './log' );
const { engines, version } = require( '../package.json' );
const scaffold = require( './scaffold' );
const { getDefaultValues, getPrompts } = require( './templates' );
const {
getBlockTemplate,
getDefaultValues,
getPrompts,
} = require( './templates' );

const commandName = `wp-create-block`;
program
Expand All @@ -24,13 +28,13 @@ program
'it is used as the block slug used for its identification, the output ' +
'location for scaffolded files, and the name of the WordPress plugin.' +
'The rest of the configuration is set to all default values unless ' +
'overriden with some of the options listed below.'
'overridden with some of the options listed below.'
)
.version( version )
.arguments( '[slug]' )
.option(
'-t, --template <name>',
'template type name, allowed values: "es5", "esnext"',
'block template type name, allowed values: "es5", "esnext"',
'esnext'
)
.option( '--namespace <value>', 'internal namespace for the block name' )
Expand All @@ -41,14 +45,21 @@ program
.action(
async (
slug,
{ category, namespace, shortDescription, template, title }
{
category,
namespace,
shortDescription: description,
template: templateName,
title,
}
) => {
await checkSystemRequirements( engines );
try {
const defaultValues = getDefaultValues( template );
const blockTemplate = await getBlockTemplate( templateName );
const defaultValues = getDefaultValues( blockTemplate );
const optionsValues = pickBy( {
category,
description: shortDescription,
description,
namespace,
title,
} );
Expand All @@ -60,14 +71,14 @@ program
title: startCase( slug ),
...optionsValues,
};
await scaffold( template, answers );
await scaffold( blockTemplate, answers );
} else {
const propmpts = getPrompts( template ).filter(
const prompts = getPrompts( blockTemplate ).filter(
( { name } ) =>
! Object.keys( optionsValues ).includes( name )
);
const answers = await inquirer.prompt( propmpts );
await scaffold( template, {
const answers = await inquirer.prompt( prompts );
await scaffold( blockTemplate, {
...defaultValues,
...optionsValues,
...answers,
Expand Down
31 changes: 14 additions & 17 deletions packages/create-block/lib/scaffold.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
/**
* External dependencies
*/
const { dirname, join } = require( 'path' );
const { writeFile } = require( 'fs' ).promises;
const { snakeCase } = require( 'lodash' );
const makeDir = require( 'make-dir' );
const { readFile, writeFile } = require( 'fs' ).promises;
const { render } = require( 'mustache' );
const { snakeCase } = require( 'lodash' );
const { dirname } = require( 'path' );

/**
* Internal dependencies
*/
const initWPScripts = require( './init-wp-scripts' );
const { code, info, success } = require( './log' );
const { hasWPScriptsEnabled, getOutputFiles } = require( './templates' );
const { hasWPScriptsEnabled } = require( './templates' );

module.exports = async function(
templateName,
blockTemplate,
{
namespace,
slug,
Expand All @@ -35,6 +35,7 @@ module.exports = async function(
info( '' );
info( `Creating a new WordPress block in "${ slug }" folder.` );

const { outputTemplates } = blockTemplate;
const view = {
namespace,
namespaceSnakeCase: snakeCase( namespace ),
Expand All @@ -51,33 +52,29 @@ module.exports = async function(
textdomain: namespace,
};
await Promise.all(
getOutputFiles( templateName ).map( async ( file ) => {
const template = await readFile(
join(
__dirname,
`templates/${ templateName }/${ file }.mustache`
),
'utf8'
);
Object.keys( outputTemplates ).map( async ( outputFile ) => {
// Output files can have names that depend on the slug provided.
const outputFilePath = `${ slug }/${ file.replace(
const outputFilePath = `${ slug }/${ outputFile.replace(
/\$slug/g,
slug
) }`;
await makeDir( dirname( outputFilePath ) );
writeFile( outputFilePath, render( template, view ) );
writeFile(
outputFilePath,
render( outputTemplates[ outputFile ], view )
);
} )
);

if ( hasWPScriptsEnabled( templateName ) ) {
if ( hasWPScriptsEnabled( blockTemplate ) ) {
await initWPScripts( view );
}

info( '' );
success(
`Done: block "${ title }" bootstrapped in the "${ slug }" folder.`
);
if ( hasWPScriptsEnabled( templateName ) ) {
if ( hasWPScriptsEnabled( blockTemplate ) ) {
info( '' );
info( 'Inside that directory, you can run several commands:' );
info( '' );
Expand Down
116 changes: 57 additions & 59 deletions packages/create-block/lib/templates.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,91 +1,89 @@
/**
* External dependencies
*/
const glob = require( 'fast-glob' );
const { readFile } = require( 'fs' ).promises;
const { fromPairs } = require( 'lodash' );
const { join } = require( 'path' );

/**
* Internal dependencies
*/
const CLIError = require( './cli-error' );
const prompts = require( './prompts' );

const namespace = 'create-block';
const dashicon = 'smiley';
const category = 'widgets';
const author = 'The WordPress Contributors';
const license = 'GPL-2.0-or-later';
const licenseURI = 'https://www.gnu.org/licenses/gpl-2.0.html';
const version = '0.1.0';

const templates = {
const predefinedBlockTemplates = {
es5: {
defaultValues: {
namespace,
slug: 'es5-example',
title: 'ES5 Example',
description:
'Example block written with ES5 standard and no JSX – no build step required.',
dashicon,
category,
author,
license,
licenseURI,
version,
},
outputFiles: [
'.editorconfig',
'editor.css',
'index.js',
'$slug.php',
'style.css',
'readme.txt',
],
wpScripts: false,
},
esnext: {
defaultValues: {
namespace,
slug: 'esnext-example',
title: 'ESNext Example',
description:
'Example block written with ESNext standard and JSX support – build step required.',
dashicon,
category,
author,
license,
licenseURI,
version,
},
outputFiles: [
'.editorconfig',
'.gitignore',
'editor.css',
'src/edit.js',
'src/index.js',
'src/save.js',
'$slug.php',
'style.css',
'readme.txt',
],
wpScriptsEnabled: true,
},
};

const getTemplate = ( templateName ) => {
if ( ! templates[ templateName ] ) {
const getOutputTemplates = async ( name ) => {
const outputTemplatesPath = join( __dirname, 'templates', name );
const outputTemplatesFiles = await glob( '**/*.mustache', {
cwd: outputTemplatesPath,
dot: true,
} );
return fromPairs(
await Promise.all(
outputTemplatesFiles.map( async ( outputTemplateFile ) => {
const outputFile = outputTemplateFile.replace(
'.mustache',
gziolo marked this conversation as resolved.
Show resolved Hide resolved
''
);
const outputTemplate = await readFile(
Copy link
Member

Choose a reason for hiding this comment

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

Probably outside the scope of what you're trying to do here, since as far as I know it's not fundamentally changing anything that doesn't already exist, but: I wonder if we should be lazier here. Rather than eagerly loading every template for every template option, at least only loading the template files for what's relevant given the user input.

It could be a matter of the template string being a function to call when needed, or something more "clever" (not necessarily a good thing) like Object.defineProperty getters, or only getOutputTemplates for templates relevant by input.

The last might also be good to avoid fast-glob if we know we won't need the template files.

Copy link
Member Author

@gziolo gziolo May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It could be a matter of the template string being a function to call when needed

The last might also be good to avoid fast-glob if we know we won't need the template files.

To clarify, it only loads all templates for a block template selected when calling npm init @wordpress/block. By default, it will only grab files from lib/templates/esnext folder. At the moment, we use all loaded templates to scaffold the project for the block.

Rather than eagerly loading every template for every template option, at least only loading the template files for what's relevant given the user input.

Once we have some conditions that make some templates options, it will make sense to defer loading templates and using a technique like " template string being a function" 👍

Did I miss something?

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, it only loads all templates for a block template selected when calling npm init @wordpress/block. By default, it will only grab files from lib/templates/esnext folder. At the moment, we use all loaded templates to scaffold the project for the block.

That's good, and largely addresses the concern I had. It was a misinterpretation on my part.

I was thinking about it a little more later in the day yesterday, still misled by the prior concern, but technically still valid: Ideally we try to avoid holding file content in memory longer than necessary. It's something which Node streams excel at, though at a more simple level it could just be a matter of: Only ever having one file loaded in memory at a time, generate the output, move on to the next (rely on garbage collection).

It could have been more of a problem if my previous impression of the implementation was correct, which it's not. Now it may be overkill 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Only ever having one file loaded in memory at a time, generate the output, move on to the next (rely on garbage collection).

We had that before and I liked it more. Here I prematurely assumed that template downloaded from npm would be load into memory and removed instantaneously. Let's see how the final implementation looks like. If we could make it work with npm install my-template --no-save then we could list absolute paths to the individual file instead and that would solve both issues assuming that npm cleans up the node_modules folder when using npm init :)

join( outputTemplatesPath, outputTemplateFile ),
'utf8'
);
return [ outputFile, outputTemplate ];
} )
)
);
};

const getBlockTemplate = async ( templateName ) => {
if ( ! predefinedBlockTemplates[ templateName ] ) {
throw new CLIError(
`Invalid template type name. Allowed values: ${ Object.keys(
templates
`Invalid block template type name. Allowed values: ${ Object.keys(
predefinedBlockTemplates
).join( ', ' ) }.`
);
}
return templates[ templateName ];
};

const getDefaultValues = ( templateName ) => {
return getTemplate( templateName ).defaultValues;
return {
...predefinedBlockTemplates[ templateName ],
outputTemplates: await getOutputTemplates( templateName ),
};
};

const getOutputFiles = ( templateName ) => {
return getTemplate( templateName ).outputFiles;
const getDefaultValues = ( blockTemplate ) => {
return {
namespace: 'create-block',
dashicon: 'smiley',
category: 'widgets',
author: 'The WordPress Contributors',
license: 'GPL-2.0-or-later',
licenseURI: 'https://www.gnu.org/licenses/gpl-2.0.html',
version: '0.1.0',
...blockTemplate.defaultValues,
};
};

const getPrompts = ( templateName ) => {
const defaultValues = getDefaultValues( templateName );
const getPrompts = ( blockTemplate ) => {
const defaultValues = getDefaultValues( blockTemplate );
return Object.keys( prompts ).map( ( promptName ) => {
return {
...prompts[ promptName ],
Expand All @@ -94,13 +92,13 @@ const getPrompts = ( templateName ) => {
} );
};

const hasWPScriptsEnabled = ( templateName ) => {
return getTemplate( templateName ).wpScriptsEnabled || false;
const hasWPScriptsEnabled = ( blockTemplate ) => {
return ! ( blockTemplate.wpScripts === false );
gziolo marked this conversation as resolved.
Show resolved Hide resolved
};

module.exports = {
getBlockTemplate,
getDefaultValues,
getOutputFiles,
getPrompts,
hasWPScriptsEnabled,
};
1 change: 1 addition & 0 deletions packages/create-block/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"check-node-version": "^3.1.1",
"commander": "^4.1.0",
"execa": "^4.0.0",
"fast-glob": "^2.2.7",
"inquirer": "^7.1.0",
"lodash": "^4.17.15",
"make-dir": "^3.0.0",
Expand Down