scripts: Add build-blocks-manifest command #65866

mreishus · 2024-10-03T20:46:26Z

What?

Adds a new wp-scripts build-blocks-manifest command.

Why?

To help plugin authors take advantage of the new function wp_register_block_metadata_collection() offered in https://core.trac.wordpress.org/changeset/59132 . It requires all block.json files to be compiled into one php manifest file, which this command does.

How?

Looks for block.json files, keys them off of the parent directory they belong to, then passes to json2php (new dependency alert) to write to a php file.

Testing Instructions

In the Gutenberg repo: Run npm run build:blocks-manifest and inspect the build/blocks-manifest.php file.
As wp-scripts: I'm not sure, this is my first time working on this, but I hope it will 'just work' due to the PR following the conventions.

Testing Instructions for Keyboard

Screenshots or screencast

mreishus · 2024-10-03T20:47:05Z

Tag @felixarntz ^
Is this what you had in mind? This is my first time working in wp-scripts.

felixarntz

@mreishus Thanks for jumping on this to create the PR, it looks great to me so far. Kudos for already adding such extensive documentation! ❤️

I'm personally not the best candidate to review the JS command implementation, would be great to get reviews from people that were previously more involved in @wordpress/scripts. Maybe @gziolo you can help here?

felixarntz · 2024-10-04T16:52:15Z

packages/scripts/scripts/build-blocks-manifest.js

+const { getArgFromCLI } = require( '../utils' );
+
+// Set default paths
+const defaultInputDir = 'build';


Should the default be build or src? I can see why you chose build, but a counter argument would be that this means you always have to run npm run build before you run npm run build:blocks-manifest.

To my knowledge, the block.json files are copied as is from src to build, so in that case I think src is a better default. WDYT?

Hmm, I think I was influenced from my recent work in the WooCommerce plugin. That repo generates block.json files as a part of its main build process that are in directories named differently than their source values in assets/. Over there, the only way I could generate the right information was to pull from build/ while also writing to build/. That may be atypical though.

I was about to move it to src here, but I found that in order to keep the npm run build:blocks-manifest command working in the root, I'd have to look through 4 src directories:

./packages/blocks/src/ ./packages/widgets/src/ ./packages/block-library/src/ ./packages/edit-widgets/src/

We don't have a way to specify multiple input dirs. It may be the build->build after is a useful pattern for monorepos?

This one is interesting. When you use wp-scripts build then block.json files get copied from src to build and sometimes they even can get modified:

gutenberg/packages/scripts/config/webpack.config.js

Lines 314 to 345 in 79940e0

transform( content, absoluteFrom ) {

const convertExtension = ( path ) => {

return path.replace( /\.m?(j|t)sx?$/, '.js' );

};

if ( basename( absoluteFrom ) === 'block.json' ) {

const blockJson = JSON.parse( content.toString() );

[

getBlockJsonScriptFields( blockJson ),

getBlockJsonModuleFields( blockJson ),

].forEach( ( fields ) => {

if ( fields ) {

for ( const [

key,

value,

] of Object.entries( fields ) ) {

if ( Array.isArray( value ) ) {

blockJson[ key ] =

value.map( convertExtension );

} else if (

typeof value === 'string'

) {

blockJson[ key ] =

convertExtension( value );

}

}

}

} );

return JSON.stringify( blockJson, null, 2 );

}

In effect, I think it's a valid approach to source the block.json from the build folder to put them in the build folder. In the long run, these probably should be an option on the wp-scripts build --use-manifest. However, we can get there in steps.

That all makes sense, let's keep using build for now.

github-actions · 2024-10-07T14:36:47Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mreishus <mreishus@git.wordpress.org>
Co-authored-by: gziolo <gziolo@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>
Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

gziolo · 2024-10-08T12:39:14Z

package.json

@@ -272,6 +272,7 @@
 	"scripts": {
 		"build": "npm run build:packages && wp-scripts build",
 		"build:analyze-bundles": "npm run build -- --webpack-bundle-analyzer",
+		"build:blocks-manifest": "wp-scripts build-blocks-manifest",


This could go next in another PR to make it all simpler to land 😄

This could be removed for now as it isn’t connected anywhere.

Thank you, removed here: e24747e

sirreal

The code looks reasonable to me. It would be good to add a CHANGELOG entry.

A basic test would be nice, it could be a snapshot of the expected result. DEWP has some testing like this that could serve as inspiration:

https://github.com/WordPress/gutenberg/blob/trunk/packages/dependency-extraction-webpack-plugin/test/build.js

sirreal

I tested this out in Gutenberg and it seems to work well. I did list a couple nice-to-haves in my previous review but nothing blocking.

felixarntz

Giving this a 2nd approval, pending the minor feedback. Thanks @mreishus!

mreishus · 2024-10-09T19:09:34Z

added snapshot test 389b128
rebased and solved lockfile conflict (by dropping the npm install commit and making a new one)
added changelog 840742a

jsnajdr · 2024-10-09T21:05:53Z

packages/scripts/scripts/build-blocks-manifest.js

+const blockJsonFiles = glob( './**/block.json', {
+	cwd: path.resolve( process.cwd(), inputDir ),
+	absolute: true,
+} );


The script should first check if the build directory exists and abort with error if it doesn't.

It makes sense to run this script only if the blocks were already built and the build directory was created. If you forgot to run the build first, you should be told about it, instead of silently creating an empty manifest file.

👍 Check e673f31

felixarntz · 2024-10-15T21:51:37Z

@jsnajdr Could you please take another look at this? Ideally, we could merge this before Thursday, as I am currently working on a dev note post for the change for WordPress 6.7, and it would make sense to have this available to reference for developers already in that post.

See https://make.wordpress.org/core/?p=115864&preview=1&_ppp=848634d99d for the public preview of that dev note. @gziolo @mreishus Could you please also review my draft and let me know your feedback?

gziolo · 2024-10-16T07:33:16Z

Ideally, we could merge this before Thursday, as I am currently working on a dev note post for the change for WordPress 6.7, and it would make sense to have this available to reference for developers already in that post.

Merging this PR is not enough to publish it to npm. The next package release will happen next week on Wednesday. Let me know if you would like to publish it separately on demand earlier.

Could you please also review my draft and let me know your feedback?

I'm checking the draft next.

gziolo · 2024-10-16T07:47:29Z

See https://make.wordpress.org/core/?p=115864&preview=1&_ppp=848634d99d for the public preview of that dev note.

I'm not sure what is the best place for feedback. I can start here and move it elsewhere upon request:

It needs to be clarified wp_register_block_metadata_collection() is an additional step on top of the existing registration process. In effect, I would include the snippet illustrating how the blocks get registered using:

wp_register_block_metadata_collection(
    WP_PLUGIN_DIR . '/my-block-library/build',
    WP_PLUGIN_DIR . '/my-block-library/build/blocks-manifest.php'
);
register_block_type( WP_PLUGIN_DIR . '/my-block-library/build/pricing' );
register_block_type( WP_PLUGIN_DIR . '/my-block-library/build/services', array( 'render_callback' => 'my_block_library_render_callback' ) ); // It should still be possible to pass additional settings, so might be worth highlighting.
register_block_type( WP_PLUGIN_DIR . '/my-block-library/build/testimonial/block.json' ); // Does it work with full path, too?

As noted in the code snippet, some notes about the path defined to the block.json file when using the collection would be helpful.
Totally optional, illustrate that passing additional settings when registering the block is still compatible with the metadata collection.

mreishus · 2024-10-16T14:28:01Z

register_block_type( WP_PLUGIN_DIR . '/my-block-library/build/testimonial/block.json' ); // Does it work with full path, too?

Indeed, it works with full paths as well.

For the draft, it looks good! I agree with the "clarify it's an additional step" from @gziolo. For myself, I have no suggestions, other than optionally, you may want to also include https://make.wordpress.org/core/2022/10/07/improved-php-performance-for-core-blocks-registration/ when showing previous context.

Also, I'm sure you guys already know this, but just to be un-ambiguous, I don't have the ability to merge this PR. :)

gziolo · 2024-10-17T11:14:17Z

Everything works as intended:

gziolo · 2024-10-17T11:21:09Z

Once PR lands in trunk, I will cherry pick to wp/latest and trigger npm publishing.

jsnajdr

This looks good to me, thanks @mreishus for implementing the suggestion to abort right away when the build directory doesn't exist.

* scripts: Add build-blocks-manifest command * Remove package.json entry * add test * npm install in root to update lockfile * Add changelog entry * error on empty and missing dirs, update tests * Update CHANGELOG.md --------- Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl> Co-authored-by: mreishus <mreishus@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: felixarntz <flixos90@git.wordpress.org> Co-authored-by: sirreal <jonsurrell@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>

gziolo · 2024-10-17T13:25:50Z

Publishing to npm with https://github.com/WordPress/gutenberg/actions/runs/11385894200.

* scripts: Add build-blocks-manifest command * Remove package.json entry * add test * npm install in root to update lockfile * Add changelog entry * error on empty and missing dirs, update tests * Update CHANGELOG.md --------- Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl> Co-authored-by: mreishus <mreishus@git.wordpress.org> Co-authored-by: gziolo <gziolo@git.wordpress.org> Co-authored-by: felixarntz <flixos90@git.wordpress.org> Co-authored-by: sirreal <jonsurrell@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>

mreishus mentioned this pull request Oct 3, 2024

Add command to @wordpress/scripts to create block manifest PHP file #65758

Closed

felixarntz reviewed Oct 4, 2024

View reviewed changes

mreishus marked this pull request as ready for review October 7, 2024 14:36

mreishus requested review from gziolo, ntwb, nerrad, ajitbohra and ryanwelcher as code owners October 7, 2024 14:36

gziolo added [Tool] WP Scripts /packages/scripts [Type] New API New API to be used by plugin developers or package users. labels Oct 8, 2024

gziolo reviewed Oct 8, 2024

View reviewed changes

gziolo requested review from sirreal and jsnajdr October 8, 2024 12:39

sirreal reviewed Oct 9, 2024

View reviewed changes

sirreal approved these changes Oct 9, 2024

View reviewed changes

felixarntz approved these changes Oct 9, 2024

View reviewed changes

gziolo approved these changes Oct 9, 2024

View reviewed changes

mreishus force-pushed the add/blocks-metadata branch from f52f2f9 to 2693402 Compare October 9, 2024 19:08

mreishus requested a review from sirreal October 9, 2024 20:53

jsnajdr reviewed Oct 9, 2024

View reviewed changes

mreishus requested a review from jsnajdr October 10, 2024 21:51

mreishus added 6 commits October 15, 2024 08:36

scripts: Add build-blocks-manifest command

5c664f9

Remove package.json entry

a12642a

add test

c96095c

npm install in root to update lockfile

454ebd4

Add changelog entry

5396782

error on empty and missing dirs, update tests

a92f4a1

mreishus force-pushed the add/blocks-metadata branch from e673f31 to a92f4a1 Compare October 15, 2024 13:36

gziolo added 2 commits October 17, 2024 13:15

Merge branch 'trunk' into add/blocks-metadata

e7ef075

Update CHANGELOG.md

060ad49

gziolo enabled auto-merge (squash) October 17, 2024 11:19

jsnajdr approved these changes Oct 17, 2024

View reviewed changes

gziolo merged commit 76cf075 into WordPress:trunk Oct 17, 2024
62 checks passed

github-actions bot added this to the Gutenberg 19.6 milestone Oct 17, 2024

mreishus deleted the add/blocks-metadata branch October 17, 2024 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scripts: Add build-blocks-manifest command #65866

scripts: Add build-blocks-manifest command #65866

mreishus commented Oct 3, 2024

mreishus commented Oct 3, 2024

felixarntz left a comment

felixarntz Oct 4, 2024

mreishus Oct 7, 2024

mreishus Oct 7, 2024

gziolo Oct 8, 2024

felixarntz Oct 9, 2024

github-actions bot commented Oct 7, 2024 •

edited

Loading

gziolo Oct 8, 2024

gziolo Oct 9, 2024

mreishus Oct 9, 2024

sirreal left a comment

sirreal left a comment

felixarntz left a comment

mreishus commented Oct 9, 2024 •

edited

Loading

jsnajdr Oct 9, 2024

mreishus Oct 9, 2024

felixarntz commented Oct 15, 2024

gziolo commented Oct 16, 2024

gziolo commented Oct 16, 2024

mreishus commented Oct 16, 2024

gziolo commented Oct 17, 2024

gziolo commented Oct 17, 2024

jsnajdr left a comment

gziolo commented Oct 17, 2024

	transform( content, absoluteFrom ) {
	const convertExtension = ( path ) => {
	return path.replace( /\.m?(j\|t)sx?$/, '.js' );
	};

	if ( basename( absoluteFrom ) === 'block.json' ) {
	const blockJson = JSON.parse( content.toString() );

	[
	getBlockJsonScriptFields( blockJson ),
	getBlockJsonModuleFields( blockJson ),
	].forEach( ( fields ) => {
	if ( fields ) {
	for ( const [
	key,
	value,
	] of Object.entries( fields ) ) {
	if ( Array.isArray( value ) ) {
	blockJson[ key ] =
	value.map( convertExtension );
	} else if (
	typeof value === 'string'
	) {
	blockJson[ key ] =
	convertExtension( value );
	}
	}
	}
	} );

	return JSON.stringify( blockJson, null, 2 );
	}

scripts: Add build-blocks-manifest command #65866

scripts: Add build-blocks-manifest command #65866

Conversation

mreishus commented Oct 3, 2024

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

mreishus commented Oct 3, 2024

felixarntz left a 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

github-actions bot commented Oct 7, 2024 • edited 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

sirreal left a comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

mreishus commented Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz commented Oct 15, 2024

gziolo commented Oct 16, 2024

gziolo commented Oct 16, 2024

mreishus commented Oct 16, 2024

gziolo commented Oct 17, 2024

gziolo commented Oct 17, 2024

jsnajdr left a comment

Choose a reason for hiding this comment

gziolo commented Oct 17, 2024

github-actions bot commented Oct 7, 2024 •

edited

Loading

mreishus commented Oct 9, 2024 •

edited

Loading