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.

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

Create block: Add support for external templates hosted on npm #22175

Conversation

fabiankaegy
Copy link
Member

@fabiankaegy fabiankaegy commented May 7, 2020

Motivation

The biggest motivation to support templates comes from the fact that https://github.com/WordPress/gutenberg-examples contains multiple examples of blocks that are used in the block editor tutorials (https://developer.wordpress.org/block-editor/tutorials/block-tutorial/). The idea is that we make it easier for folks and let them running one command to install such block in the plugins directory in WordPress when going through the tutorial with one command:

npm init @wordpress/block -t 05-recipe-card-esnext

As of today, they need to either copy the folder from the repository or recreate it manually when going through the tutorial.

Description

With this PR I have introduced the ability to pull templates for the create-block package from npm. This would allow for anyone to contribute / use the starter template they would like with the ease of use the create-block cli provides.

External templates

The external templates need to contain the .mustache files and one template.json file with the metadata that is currently being stored in the templates.js file.

How has this been tested?

This is being tested by running node index.js with different -t parameters. To test things directly from NPM I have created this example package: https://www.npmjs.com/package/gutenberg-esnext-template

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@gziolo gziolo self-requested a review May 7, 2020 16:28
@gziolo gziolo added [Tool] Create Block /packages/create-block [Type] Enhancement A suggestion for improvement. [Status] In Progress Tracking issues with work in progress labels May 7, 2020
@gziolo gziolo requested a review from mkaz May 7, 2020 16:42
Copy link
Member

@aaronjorbin aaronjorbin left a comment

Choose a reason for hiding this comment

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

Love this. One thing that I think would be super helpful is an example added to the readme so that devs can see what a sample template npm package looks like.

packages/create-block/lib/templates.js Outdated Show resolved Hide resolved
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Impressive work so far @fabiankaegy. I'm really excited about this feature 💯

I left quite detailed feedback. Let me know how you feel about it.

packages/create-block/lib/index.js Outdated Show resolved Hide resolved
packages/create-block/lib/templates.js Outdated Show resolved Hide resolved
packages/create-block/lib/templates.js Outdated Show resolved Hide resolved
packages/create-block/lib/templates.js Outdated Show resolved Hide resolved
packages/create-block/lib/templates.js Outdated Show resolved Hide resolved
packages/create-block/lib/templates.js Outdated Show resolved Hide resolved
packages/create-block/lib/templates.js Show resolved Hide resolved
packages/create-block/lib/templates.js Outdated Show resolved Hide resolved
packages/create-block/lib/templates.js Outdated Show resolved Hide resolved
@gziolo gziolo changed the title Add external templates to create block DRAFT Create block: Add support for external templates hosted on npm May 8, 2020
@gziolo gziolo requested a review from oandregal May 8, 2020 08:20

Inside that bootstrapped directory _(it doesn't apply to `es5` template)_, you can run several commands:
When bootstraped with the `esnext` templateyou can run several commands inside the directory:
Copy link
Member

Choose a reason for hiding this comment

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

One more thing, wpScriptsEnabled enables those commands. Once we sort out the final shape of template definition we can update this section to reflect it.

Copy link
Member Author

@fabiankaegy fabiankaegy May 8, 2020

Choose a reason for hiding this comment

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

@gziolo Yes the commands get enabled by wpScriptsEnabled but at the end it matters what the creator of the template has in their scripts in the package.json.

Thats why I changed it to only refer to the esnext template.

"style.css",
"readme.txt"
],
"wpScriptsEnabled": true
Copy link
Member

@gziolo gziolo May 8, 2020

Choose a reason for hiding this comment

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

For the future PR:

Thinking about it now when documented, it should be opt-out rather than opt-in, we want people to use wp-scripts :)

"licenseURI": "https://www.gnu.org/licenses/gpl-2.0.html",
"version": "0.1.0"
},
"outputFiles": [
Copy link
Member

@gziolo gziolo May 8, 2020

Choose a reason for hiding this comment

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

For the future PR:

I'm very interested to find a way to automate the discovery of templates with glob call or something like this. It would be great to be able to default to templates folder that the template author can override:

{
    "templatesFolder": "./my-folder"
}

@fabiankaegy
Copy link
Member Author

@gziolo I have fixed / implemented the suggestions you provided. I'm still struggling with the cleanup of the temp folder. (But it is now located in the os temp folder)

@fabiankaegy fabiankaegy requested a review from gziolo May 8, 2020 09:25
@fabiankaegy
Copy link
Member Author

For the future PR:

I think it could be interesting to refactor wpScriptsEnabled to a more general buildSystem where you have the option to provide the name of npm scripts to run directly after the installation.

Usecase:
If a template uses something different from webpack or even if it uses Wordpress script but wants to rename the name of the individual scripts they could provide the name of the scripts to run after installation to build it.

Example:

template.json

{
    ...,
    "buildProject": true,
    "buildCommand": "custom name"
}

Wit this the scaffold tool would then run npm install and npm run custom name afterwards.

@gziolo
Copy link
Member

gziolo commented May 8, 2020

I think it could be interesting to refactor wpScriptsEnabled to a more general buildSystem where you have the option to provide the name of npm scripts to run directly after the installation.

I was thinking about that as well. The simplest approach would be to use scripts property:

  • when not set, it uses the default
  • when set to an empty value like null or {} it works the same way as wpScriptsEnabled: false today
  • when set to { test: "my-custom-command" } the author of the template needs to provide all scripts

In #22235, I tried to flip the flag and make wp-scripts enabled by default. Now, I tempted to implement what I outlined above in another PR. Any concerns regarding this proposal?

Comment on lines +115 to +116
- `namespaceSnakeCase`
- `slugSnakeCase`
Copy link
Member

Choose a reason for hiding this comment

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

namespaceSnakeCase and slugSnakeCase ar sort of special here. We should omit them and find a way to apply those modifications inside the template.

@@ -45,7 +45,7 @@ program
) => {
await checkSystemRequirements( engines );
try {
const defaultValues = getDefaultValues( template );
const defaultValues = await getDefaultValues( template );
Copy link
Member

Choose a reason for hiding this comment

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

The fact that all methods need to be converted to async here is a bit concerning. I'm coming to the conclusion that we should call async getTemplate( templateName ); first and then pass the object to all other methods. This way we avoid converting all methods to async, plus there is no need to check whether we don't download the template multiple time, or process it. I'm happy to apply all refactoring in the related PR I opened earlier.

@gziolo
Copy link
Member

gziolo commented May 11, 2020

When I run npm i, I can see local changes applied, so you should run it as well and commit.

Before, it would be nice to align the version of uuid with what the project defines:

"uuid": "7.0.2",

Later, we can update it for the whole project.

@gziolo
Copy link
Member

gziolo commented May 11, 2020

I test it locally with:

$ npx wp-create-block -t gutenberg-esnext-template

and it magically works. Super exciting 🎉

packages/create-block/README.md Outdated Show resolved Hide resolved
packages/create-block/README.md Outdated Show resolved Hide resolved
packages/create-block/lib/index.js Outdated Show resolved Hide resolved
packages/create-block/lib/scaffold.js Outdated Show resolved Hide resolved
packages/create-block/lib/templates.js Outdated Show resolved Hide resolved
@gziolo
Copy link
Member

gziolo commented May 13, 2020

@fabiankaegy, I landed #22235 so you can expect some conflicts when rebasing but in general, it should be easier to integrate with 3rd party templates.

@fabiankaegy
Copy link
Member Author

@fabiankaegy, I landed #22235 so you can expect some conflicts when rebasing but in general, it should be easier to integrate with 3rd party templates.

Cool 👍 I saw that you integrated it and I will make sure to go over the changes and also fix @aduth's suggestions :)

@gziolo
Copy link
Member

gziolo commented Jul 6, 2020

Work continues in #23712.

@gziolo gziolo closed this Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress [Tool] Create Block /packages/create-block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants