Introduce block metadata files for dynamic blocks #14238

gziolo · 2019-03-05T11:01:04Z

Description

This PR in its initial form tries to explore how the current proposal shared in the block registration RFC (#13693) could look with all dynamics blocks registered by Gutenberg core.

Dynamic blocks covered:

All other blocks should be covered separately.

TODO

Fix core blocks override - as a temp solution I renamed all functions names (related PR Blocks: Re-register core blocks via build copy prefixing #13521).
Bring back client-side block registration based on JSON definition.
Evaluate renderCallback implementation.
Expose all specified fields from the server to the client (icon (slug), other fields from RFC not used so far)

aduth · 2019-03-05T20:44:43Z

lib/blocks.php

+ *
+ * @param string $file   Path to the translation file to load.
+ */
+function gutenberg_register_block_type_from_metadata( $file ) {


Is this something we see becoming a part of core implementation? I think for any new functions like this, I'm wondering if we should bother with the namespacing convention, since it was a pain point for deprecating Gutenberg-specific functions toward and after WordPress 5.0.

Wondering if instead it's enough to just if ( ! function_exists( 'register_block_type_from_metadata' ) ) {

I had it named this way initially. I wasn't sure what the process looks like so I opted for gutenberg_ prefixed version. I'm fine with updating it as suggested as soon as we figure out how this definition should look and work like.

aduth · 2019-03-05T20:53:49Z

lib/blocks.php

+ * @param string $file   Path to the translation file to load.
+ */
+function gutenberg_register_block_type_from_metadata( $file ) {
+	$metadata = json_decode( file_get_contents( $file ), true );


Technically this implementation seems to be fine for a file which doesn't exist (file_get_contents returns false) or a file which is invalid JSON (json_decode returns false), only because empty is very tolerant about what it receives. Could be a potential source of error in the future, at least if not tested against.

Yes, it should use more checks with proper error messages. I will make those checks more granular 👍

Also make sure to use wp_json_encode() :)

Usages of file_get_contents() in Core are typically prefix with an error control operator @file_get_conents() and there are more resilient read functions in the WP_Filesystem class.

I'm sure this part needs more work if we decide to introduce such API for reading JSON file. This PR is more exploratory. I expect it's going to be divided in more solid smaller PRs which will implement more granular features. If someone wants to work on this part, we can find ways to split this task :)

aduth · 2019-03-05T21:00:49Z

lib/blocks.php

+		$render_callback_file = dirname( $file ) . '/' . $metadata['renderCallback'];
+		if ( file_exists( $render_callback_file ) ) {
+			require $render_callback_file;
+			$settings['render_callback'] = 'gutenberg_render_block_' . str_replace( array( '/', '-' ), '_', $block_name );


I think it requires anonymous functions (PHP 5.4.0+), but I wonder if rather than relying on a specific naming convention for functions, we use the file returns semantics:

https://gist.github.com/aduth/ef27bfaae19a9959a263f1600b37c71d

I didn't think too much about so far. I definitely would prefer to avoid using the specific conventions for naming functions. Another solution is to use a function which registers such callbacks as follows:

register_block_render_callback( 'core/block', 'render_block_core_block' );

which would be placed inside a PHP file which would also contain the definition. We would also have to change the way such file is required. Maybe we load metadata definitions in one go. Then at the time, we are about to process render_callbacks for all registered blocks we require all files referenced as renderCallbacks?

The register_block_render_callback could impose more requirements on the runtime which consumes from the file (i.e. the function needs to be defined somewhere), in case that might deter us.

A yet-other idea is treating it more like a template file, expecting it to output HTML directly. I'm not sure this is a good way to go though, and it's not obvious how the template would read from attributes (unless they were just some $attributes global defined for use by the file).

Some of those existing render callbacks reference other custom functions so this might get tricky. I don't know, maybe this would work. I'd love to see some thoughts from developers better familiar with patterns used in other parts of WordPress core like @pento or @danielbachhuber.

I'm not 100% sure I follow. Why is $metadata['renderCallback'] a file and not simply a callable function?

Probably the best example of prior art to follow is register_shortcode() (which accepts a callable function). I wouldn't be opposed to taking the template file approach instead, but core doesn't currently have the pattern of injecting variables into a template (see the long conversation in 21676)

aduth · 2019-03-05T21:03:34Z

packages/block-library/src/archives/edit.js

 	ServerSideRender,
 } from '@wordpress/editor';

 export default function ArchivesEdit( { attributes, setAttributes } ) {
-	const { align, showPostCounts, displayAsDropdown } = attributes;


Is this something we could extract separately? Can we not implement supports.align for server-registered blocks currently?

Yes, I will tackle it and some other preparatory tasks in their own PRs. I'm using this PR to identify all work that is required to get us closer to have the ability to register blocks with metadata. However, I'm discovering that we have some issues with the existing blocks. className and align is often not defined on the server and therefore not exposed for render_callback method.

aduth · 2019-03-05T21:08:26Z

packages/block-library/src/archives/block.json

+	"title": "Archives",
+	"category": "widgets",
+	"icon": {
+		"src": "icon.js"


This one kind of sticks out as far as being inconsistent with other file references like in edit and renderCallback. I see from #13693 this is done as such to support properties like foreground and background color assignments.

In all, I think it's okay, though I also wonder on a few variations:

Optionally also allow icon to be assigned as a string if it only needs to assign src

Create a pattern around "file reference with options", something like a tuple of [ file, options ] ({ "icon": [ "icon.js", { "background": "red" } ] })

Was looking at some similar specifications, where src is a pretty accepted convention for naming this type of resource (example).

Optionally also allow icon to be assigned as a string if it only needs to assign src

It is reserved for the slug at the moment, which aligns with how you can register block's icon as of today. I would be happy to use the file name as a shortcut but then we need to figure out how to provide the slug differently.

It is reserved for the slug at the moment, which aligns with how you can register block's icon as of today. I would be happy to use the file name as a shortcut but then we need to figure out how to provide the slug differently.

Oh right. That makes sense. I guess the other option is to inspect the shape of the string to see if it "appears" as a file path (e.g. has file extension, or starts with ./)

aduth · 2019-03-05T21:11:24Z

lib/blocks.php

+	}
+	$registry = WP_Block_Type_Registry::get_instance();
+	if ( $registry->is_registered( $block_name ) ) {
+		$registry->unregister( $block_name );


I guess this is part of it "overriding" ? Not something to be included with a final implementation.

Definitely, it was only added to make it work until it's fixed in master :)

aduth · 2019-03-05T21:18:23Z

packages/block-library/src/categories/icon.js

+/**
+ * WordPress dependencies
+ */
+import { SVG, Path } from '@wordpress/components';


This is a good example of: How do we communicate in the manifest specific dependencies of the block (#13693 (comment), assets.json, etc).

We could have some NPM-style dependencies property in block.json, but we still have some challenges:

How does WordPress resolve those?

Or does it? (Do we ship something already bundled with dependencies resolved?)

I could imagine, for example, that the icon file referenced is one which already has everything bundled. This becomes very wasteful for shared dependencies, so I think we'd need some system there, but it's not clear to me how that looks in a way which is agnostic to the WordPress script loader / globals.

i18n and assets are two issues we need to resolve to move forward. They are both very complex :)

I'm starting to question whether we should be splitting JS code into several files: edit.js, save.js, icon.js, transforms.js, etc. It only makes it harder to manage their dependencies without bigger benefits. If you want to insert a given block into the editor, you will have to load all those assets anyway.

It might be also be not enough to define only renderCallback, what if a plugin developer doesn't need a render callback but wants to expose REST API endpoint? How do they do it? What if plugin bundles multiple blocks into one JS file?

aduth · 2019-03-05T21:23:43Z

packages/block-library/src/block/block.json

@@ -0,0 +1,18 @@
+{
+	"name": "core/block",
+	"title": "Reusable Block",


Have you thought about how translation will work? From #13693:

Localized properties are automatically wrapped in __ function calls on the backend and the frontend of WordPress. These translations are added as an inline script to the wp-block-library script handle.

https://github.com/WordPress/gutenberg/pull/13693/files#diff-4b5f9e280074f644def145cb4cc4fb6eR372

A challenge here is in knowing the textdomain to use if we were to call __ automatically when registering the block. I could see this being as simple as an additional textdomain property within the block manifest (which is maybe not great because it's not really inherent to a block). Or an argument when calling to register a block from a manifest file. Or some hacky wizardry with debug_backtrace + get_plugin_data to extract the Text Domain field from the plugin header.

I think this is the biggest issue I identified so far. I still don't have a good idea of how this could work. textdomain as another attribute might be an option. I think it should be rather explicitly defined than depend on some traversal techniques. If this code would be placed in PHP, all translated string would have to provide this argument anyway.

I was also wondering if we can dynamically wrap with i18n functions when loading metadata. E.g.:

$settings = array( 'title' => __( $metadata['title'], $metadata['textdomain'] ), );

/cc @swissspidy

gziolo · 2019-03-06T10:14:52Z

lib/blocks.php

+		$render_callback_file = dirname( $file ) . '/' . $metadata['renderCallback'];
+		if ( file_exists( $render_callback_file ) ) {
+			require $render_callback_file;
+			$settings['render_callback'] = 'gutenberg_render_block_' . str_replace( array( '/', '-' ), '_', $block_name );


I didn't think too much about so far. I definitely would prefer to avoid using the specific conventions for naming functions. Another solution is to use a function which registers such callbacks as follows:

register_block_render_callback( 'core/block', 'render_block_core_block' );

which would be placed inside a PHP file which would also contain the definition. We would also have to change the way such file is required. Maybe we load metadata definitions in one go. Then at the time, we are about to process render_callbacks for all registered blocks we require all files referenced as renderCallbacks?

gziolo · 2019-03-06T10:16:13Z

lib/blocks.php

+ * @param string $file   Path to the translation file to load.
+ */
+function gutenberg_register_block_type_from_metadata( $file ) {
+	$metadata = json_decode( file_get_contents( $file ), true );


Yes, it should use more checks with proper error messages. I will make those checks more granular 👍

gziolo · 2019-03-06T10:17:23Z

lib/blocks.php

+ *
+ * @param string $file   Path to the translation file to load.
+ */
+function gutenberg_register_block_type_from_metadata( $file ) {


I had it named this way initially. I wasn't sure what the process looks like so I opted for gutenberg_ prefixed version. I'm fine with updating it as suggested as soon as we figure out how this definition should look and work like.

gziolo · 2019-03-06T10:18:10Z

lib/blocks.php

+	}
+	$registry = WP_Block_Type_Registry::get_instance();
+	if ( $registry->is_registered( $block_name ) ) {
+		$registry->unregister( $block_name );


Definitely, it was only added to make it work until it's fixed in master :)

gziolo · 2019-03-06T10:23:12Z

packages/block-library/src/archives/block.json

+	"title": "Archives",
+	"category": "widgets",
+	"icon": {
+		"src": "icon.js"


Optionally also allow icon to be assigned as a string if it only needs to assign src

It is reserved for the slug at the moment, which aligns with how you can register block's icon as of today. I would be happy to use the file name as a shortcut but then we need to figure out how to provide the slug differently.

gziolo · 2019-03-06T10:39:06Z

packages/block-library/src/archives/edit.js

 	ServerSideRender,
 } from '@wordpress/editor';

 export default function ArchivesEdit( { attributes, setAttributes } ) {
-	const { align, showPostCounts, displayAsDropdown } = attributes;


Yes, I will tackle it and some other preparatory tasks in their own PRs. I'm using this PR to identify all work that is required to get us closer to have the ability to register blocks with metadata. However, I'm discovering that we have some issues with the existing blocks. className and align is often not defined on the server and therefore not exposed for render_callback method.

gziolo · 2019-03-06T10:46:07Z

packages/block-library/src/block/block.json

@@ -0,0 +1,18 @@
+{
+	"name": "core/block",
+	"title": "Reusable Block",


I think this is the biggest issue I identified so far. I still don't have a good idea of how this could work. textdomain as another attribute might be an option. I think it should be rather explicitly defined than depend on some traversal techniques. If this code would be placed in PHP, all translated string would have to provide this argument anyway.

I was also wondering if we can dynamically wrap with i18n functions when loading metadata. E.g.:

$settings = array( 'title' => __( $metadata['title'], $metadata['textdomain'] ), );

/cc @swissspidy

gziolo · 2019-03-06T11:01:40Z

packages/block-library/src/categories/icon.js

+/**
+ * WordPress dependencies
+ */
+import { SVG, Path } from '@wordpress/components';


i18n and assets are two issues we need to resolve to move forward. They are both very complex :)

I'm starting to question whether we should be splitting JS code into several files: edit.js, save.js, icon.js, transforms.js, etc. It only makes it harder to manage their dependencies without bigger benefits. If you want to insert a given block into the editor, you will have to load all those assets anyway.

It might be also be not enough to define only renderCallback, what if a plugin developer doesn't need a render callback but wants to expose REST API endpoint? How do they do it? What if plugin bundles multiple blocks into one JS file?

gziolo · 2019-03-06T11:03:20Z

packages/block-library/src/search/block.json

+	"attributes": {
+		"label": {
+			"type": "string",
+			"default": "Search"


label and buttonText are tricky, too. Their default values were originally wrapped with i18n function. How do we apply it through JSON file?

gziolo · 2019-04-08T11:38:10Z

At this moment, it'd be probably better to close this one and use it as an example later when all static blocks are fully converted to use block.json metadata format.

gziolo · 2019-04-10T10:22:04Z

The larger part of this PR is already covered with merged PRs, there are some open PRs like #14882, #14899 which will tackle remaining parts. The only valuable code left is PHP function for registering block.json metadata should be copied and moved to its own PR.

Introduce block metadata files for dynamic blocks

5c05cd8

gziolo added [Feature] Block API API that allows to express the block paradigm. [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible labels Mar 5, 2019

gziolo self-assigned this Mar 5, 2019

gziolo added 3 commits March 5, 2019 12:36

Add two more defintions for dynamic blocks

7519ab6

Convert all remaining dynamic blocks to use metadata format

33a6265

Add a way to override core block registration based on metadata

625c7e7

This was referenced Mar 5, 2019

Add the block registration RFC #13693

Merged

Blocks: Re-register core blocks via build copy prefixing #13521

Merged

gziolo requested review from aduth and youknowriad March 5, 2019 14:57

gziolo added the [Status] In Progress Tracking issues with work in progress label Mar 5, 2019

aduth reviewed Mar 5, 2019

View reviewed changes

gziolo added 2 commits March 6, 2019 10:45

Convert Shortcode block to use metadata format

cff3053

Convert Search block to use metadata format

981e136

gziolo commented Mar 6, 2019

View reviewed changes

gziolo added [Feature] Block Directory Related to the Block Directory, a repository of block plugins [Feature] Block API API that allows to express the block paradigm. and removed [Feature] Block API API that allows to express the block paradigm. labels Mar 19, 2019

gziolo added [Status] Blocked Used to indicate that a current effort isn't able to move forward and removed [Status] In Progress Tracking issues with work in progress labels Apr 8, 2019

gziolo closed this Apr 10, 2019

gziolo deleted the try/block-metadata branch April 10, 2019 10:22

gziolo removed the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Apr 10, 2019

gziolo mentioned this pull request Jun 18, 2019

Block API: Extend the block registration API technical proposal #16209

Closed

14 tasks

koke mentioned this pull request Jan 10, 2020

[RNMobile] Shortcode block support #19534

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce block metadata files for dynamic blocks #14238

Introduce block metadata files for dynamic blocks #14238

gziolo commented Mar 5, 2019 •

edited

Loading

aduth Mar 5, 2019

gziolo Mar 6, 2019

aduth Mar 5, 2019

gziolo Mar 6, 2019

danielbachhuber Mar 7, 2019

0aveRyan Mar 8, 2019

gziolo Mar 8, 2019

aduth Mar 5, 2019

gziolo Mar 6, 2019

aduth Mar 6, 2019

gziolo Mar 7, 2019

danielbachhuber Mar 7, 2019

aduth Mar 5, 2019

gziolo Mar 6, 2019

aduth Mar 5, 2019

gziolo Mar 6, 2019

aduth Mar 6, 2019

aduth Mar 5, 2019

gziolo Mar 6, 2019

aduth Mar 5, 2019

gziolo Mar 6, 2019

aduth Mar 5, 2019

gziolo Mar 6, 2019

gziolo Mar 6, 2019

gziolo Mar 6, 2019

gziolo Mar 6, 2019

gziolo Mar 6, 2019

gziolo Mar 6, 2019

gziolo Mar 6, 2019

gziolo Mar 6, 2019

gziolo Mar 6, 2019

gziolo Mar 6, 2019

gziolo commented Apr 8, 2019

gziolo commented Apr 10, 2019 •

edited

Loading

Introduce block metadata files for dynamic blocks #14238

Introduce block metadata files for dynamic blocks #14238

Conversation

gziolo commented Mar 5, 2019 • edited Loading

Description

TODO

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

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

gziolo commented Apr 8, 2019

gziolo commented Apr 10, 2019 • edited Loading

gziolo commented Mar 5, 2019 •

edited

Loading

gziolo commented Apr 10, 2019 •

edited

Loading