WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.37k stars 4.15k forks source link

Block Directory: Activation of a block with no title property will result in an error #38245

Closed t-hamano closed 2 years ago

t-hamano commented 2 years ago

Description

Adding a block plugin from the block directory will cause an error under certain conditions. It will probably occur for all blocks that do not specify the title property in the registerBlockType function.

This is also happening in the block plugin I publish, but the title property is listed in block.json and is not specified in registerBlockType.

It determines that there is no problem when I enter the slug of the plugin in Block Plugin Checker page,

I think the checking rules should be unified for both registerBlockType and block directory.

Also, should I specify the title property in registerBlockType as well as in block.json to solve this error for the time being?

Step-by-step reproduction instructions

  1. Toggle bock inserter.
  2. Enter the appropriate text in the search box.
  3. Activate some of the blocks that were extracted in the block directory.
  4. You should get a similar error if registerBlockType does not have a title property.

Screenshots, screen recording, code snippet

issue

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

ntsekouras commented 2 years ago

Thanks for reporting this! I agree that they should be consolidated.

The error comes from processBlockType action here: https://github.com/WordPress/gutenberg/blob/trunk/packages/blocks/src/store/actions.js#L109. @gziolo has worked a lot on these parts so maybe he has some thoughts to share.

gziolo commented 2 years ago

I think it might be an issue with the installation process. As far as I can tell, the block should work correctly after page refresh because it's registered on the server from block.json that contains the title:

https://plugins.trac.wordpress.org/browser/flexible-table-block/trunk/classes/class-enqueue.php#L30

https://plugins.trac.wordpress.org/browser/flexible-table-block/trunk/src/block.json#L4

As a temporary solution, you can include all metadata also in JavaScript, although in practice it shouldn't be necessary.

@ryelle, do you know why it might be an issue?

t-hamano commented 2 years ago

Yes, the plugin will be installed correctly and you will be able to use the block after reloading the browser.

I'll deal with this by including metadata in the JavaScript for the time being.

gziolo commented 2 years ago

I would have to test it but in this line:

https://plugins.trac.wordpress.org/browser/flexible-table-block/trunk/src/index.js#L37

You should be able to pass metadata object as a first param. It will be used when there is no metadata passed from the server. In this case it would be only when the block gets installed from Block Directory.

ryelle commented 2 years ago

As far as I can tell, the block should work correctly after page refresh because it's registered on the server from block.json that contains the title […] As a temporary solution, you can include all metadata also in JavaScript, although in practice it shouldn't be necessary.

Since blocks installed via the block directory only load the client assets, we don't have any of that server-side registration data - so adding the title in the JS registerBlockType is necessary here. Loading the whole metadata object from block.json would also work.

gziolo commented 2 years ago

It looks like we have some details from the block.json that we could move into the block registry on the client so it could use them when registering:

https://github.com/WordPress/gutenberg/blob/769245a643dce408f6165fc0ee5183f3546499cb/packages/block-directory/src/store/actions.js#L80-L85

A good example is here where we use some legacy API to inject definitions loaded directly from block.json for the mobile apps:

https://github.com/WordPress/gutenberg/blob/769245a643dce408f6165fc0ee5183f3546499cb/packages/blocks/src/api/registration.js#L253-L255

aurooba commented 2 years ago

@gziolo would that also load the attributes from block.json? Because in the case of #38588, the title wasn't the issue but the existence of default values for certain attributes. So if we can load both the name and attributes from block.json that would be lovely.

gziolo commented 2 years ago

@gziolo would that also load the attributes from block.json? Because in the case of #38588, the title wasn't the issue but the existence of default values for certain attributes. So if we can load both the name and attributes from block.json that would be lovely.

Yes. The endpoint that Block Directory uses should return most of the metadata exposed by blocks.

gziolo commented 2 years ago

I tried to see if we could do a quick workaround using the data from the Block Directory with:

diff --git a/packages/block-directory/src/store/actions.js b/packages/block-directory/src/store/actions.js
index 2ab21b6cf9..a956cf6118 100644
--- a/packages/block-directory/src/store/actions.js
+++ b/packages/block-directory/src/store/actions.js
@@ -1,7 +1,10 @@
 /**
  * WordPress dependencies
  */
-import { store as blocksStore } from '@wordpress/blocks';
+import {
+   store as blocksStore,
+   unstable__bootstrapServerSideBlockDefinitions, // eslint-disable-line camelcase
+} from '@wordpress/blocks';
 import { __, sprintf } from '@wordpress/i18n';
 import apiFetch from '@wordpress/api-fetch';
 import { store as noticesStore } from '@wordpress/notices';
@@ -82,6 +85,10 @@ export const installBlockType = ( block ) => async ( {
            links: { ...block.links, ...links },
        } );

+       unstable__bootstrapServerSideBlockDefinitions( {
+           [ block.name ]: block,
+       } );
+
        await loadAssets();
        const registeredBlocks = registry.select( blocksStore ).getBlockTypes();
        if ( ! registeredBlocks.some( ( i ) => i.name === block.name ) ) {

However, it doesn't return what we need:

Screenshot 2022-02-08 at 14 07 26

I still think we could run the REST API call to fetch the data after plugin activation.

gziolo commented 2 years ago

@t-hamano or @aurooba you both did a great job updating your plugins and now I need to find another plugin to ensure the fix I'm working on is going to help others πŸ˜„

aurooba commented 2 years ago

@gziolo give me an hour - I'm just waking up. I'll reverse mine. I haven't announced that it's working through the search now. :)

aurooba commented 2 years ago

@gziolo I've reverted my plugin. Keep in mind, my plugin always had the title field in the index.js field, what it didn't have is the attributes. So mine will be helpful in testing that. :)

ryelle commented 2 years ago

However, it doesn't return what we need:

@gziolo What data do you need? We do return a blocks property from the wp.org api that matches the blockType format (see example here), maybe that could be added into the response on the local site's endpoint? It won't help in all cases, since sometimes the block directory can't parse out the data (see this example) - but anyone using block.json should be parsed correctly (if not, that's a bug in wp.org's code).

aurooba commented 2 years ago

Since you need to have block.json in order to be able to use the Block Library search, I think @ryelle would be perfect!

gziolo commented 2 years ago

What data do you need? We do return a blocks property from the wp.org api that matches the blockType format (see example here), maybe that could be added into the response on the local site's endpoint? It won't help in all cases, since sometimes the block directory can't parse out the data (see this example) - but anyone using block.json should be parsed correctly (if not, that's a bug in wp.org's code).

That's a lot of data that won't be necessary until you install the block:

https://github.com/WordPress/gutenberg/blob/fe8707ba2e3ce1cae44ed1d556115d5e785ca897/packages/blocks/src/api/registration.js#L184-L200

I made it work with a simple REST API call to fetch a single block type. I need to build a similar mapping and it should be all we need. I see mostly benefits of using a separate request:

We need all that data from the server for the block to be fully functional.

I've reverted my plugin. Keep in mind, my plugin always had the title field in the index.js field, what it didn't have is the attributes. So mine will be helpful in testing that. :)

Wow, thank you. That's super helpful @aurooba. πŸ™‡πŸ» I will open PR soon. By the way, I think I need to submit my first block to the directory to have more flexibility to break things on demand 🀣

aurooba commented 2 years ago

@gziolo yesss submit a block! :)

gziolo commented 2 years ago

Fix is ready https://github.com/WordPress/gutenberg/pull/38697. It should cover all cases.