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

Add the block registration RFC #13693

Open
wants to merge 11 commits into
base: master
from

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Feb 6, 2019

This RFC is intended to serve both as a specification and as documentation for the implementation of runtime-agnostic block type registration.

For more details, read the RFC :)

refs #2751

(This is not ready but it's a good basis to start collaborating on a PR)

@youknowriad youknowriad self-assigned this Feb 6, 2019

@youknowriad youknowriad requested review from WordPress/gutenberg-core and danielbachhuber Feb 6, 2019

@gziolo gziolo referenced this pull request Feb 6, 2019

Open

Block type: add support link #11330

3 of 4 tasks complete

@gziolo gziolo added this to the Documentation & Handbook milestone Feb 7, 2019

@danielbachhuber
Copy link
Member

danielbachhuber left a comment

Great start! I've left several comments inline. Because they'll eventually get lost when you make edits, here's a recap:

  • It'd be useful to include more detail on why runtime-agnostic registration is necessary (e.g. descriptions of the specific use-cases to be solved) so we can continue to evaluate the RFC and attached code against solving those use-cases.
  • I don't know what lazy-loading block types means, in a practical sense. Clarification there would be helpful.
  • I wonder to what degree we'd like to leave the door open for runtime-agnostic save, render, and transform callbacks. Some of it may be technically impossible but might foster creative thinking to keep a part of the conversation.
@@ -0,0 +1,369 @@
This RFC is intended to serve both as a specification and as documentation for the implementation of runtime-agnostic block type registration.

This comment has been minimized.

@danielbachhuber

danielbachhuber Feb 8, 2019

Member

Would it be useful to include more detail on why runtime-agnostic registration is perceived to be necessary, and which specific audiences this implementation will serve?

This comment has been minimized.

@youknowriad

youknowriad Feb 8, 2019

Author Contributor

Some important aspects here:

  • The block library package are served as an npm package, we don't know where these will be run
  • The block repository (one of the 9 projects) need to be able to get block information outside of WordPress

I guess this could be added somewhere


## Requirements

Behind any block type registration is some abstract concept of a unit of content. This content type can be described without consideration of any particular technology. In much the same way, we should be able to describe the core constructs of a block type in a way which can be interpreted in any runtime.

This comment has been minimized.

@danielbachhuber

danielbachhuber Feb 8, 2019

Member

Can you include a couple examples of these units of content?

Show resolved Hide resolved docs/rfc/block-registration.md Outdated
Show resolved Hide resolved docs/rfc/block-registration.md Outdated
Show resolved Hide resolved docs/rfc/block-registration.md Outdated
Show resolved Hide resolved docs/rfc/block-registration.md Outdated
```json
{
"slug": "star", // Dashicon slug, serve as a fallback if non-js contexts.
"src": "./my-file.js", // Path to a JavaScript file containing the block's icon property

This comment has been minimized.

@danielbachhuber

danielbachhuber Feb 8, 2019

Member

Minor point: the ./ in ./my-file.js is contextual and would need to be interpreted in PHP or similar before being exposed to the editor. The browser wouldn't necessarily know the starting point for ./my-file.js if this JSON was bundled into a concatenation system, etc.

This comment has been minimized.

@youknowriad

youknowriad Feb 8, 2019

Author Contributor

For me these JSON files are not meant to be loaded by the browser directly, instead a smart "bundler" should be able to traverse it and include the dependencies, like webpack does for "css/js/younameit" imports.

This comment has been minimized.

@danielbachhuber

danielbachhuber Feb 8, 2019

Member

For me these JSON files are not meant to be loaded by the browser directly, instead a smart "bundler" should be able to traverse it and include the dependencies, like webpack does for "css/js/younameit" imports.

Is this worth stating explicitly in the RFC?

This comment has been minimized.

@gziolo

gziolo Feb 21, 2019

Member

You can't bundle JSON directly into the browser. I think this is an implementation detail and initially, it might be even not a case for the WordPress client code as we will expose those definitions through PHP anyway.

It's more a future consideration for those consumers who plan to use core blocks without WordPress as a backend. I think it's fine to leave this issue to be solved for themselves. On our end we will have to process those JSON files and replicated servers-side exposure for our unit tests.

{ "renderCallback": "my-block-render-callback.php" }
```

This is a pointer to a php file returning a render callback php function. The render callback is function called when the block is rendered on the frontend. It's used to generate the frontend markup dynamically.

This comment has been minimized.

@danielbachhuber

danielbachhuber Feb 8, 2019

Member

Would we ever want to support JavaScript render callbacks?

This comment has been minimized.

@youknowriad

youknowriad Feb 8, 2019

Author Contributor

If there's a valid use-case for it. The API could be extended if needed. The important thing is that it can be changed without BC.

{ "transforms": "my-block-transforms.js" }
```

This property is a pointer to a JavaScript file containing the save function of the block transforms. The save function defines the way in which the different attributes should be combined into the final markup, which is then serialized by Gutenberg into `post_content`.

This comment has been minimized.

@danielbachhuber

danielbachhuber Feb 8, 2019

Member

I think it's really important that we eventually are able to define server-side transforms.

This comment has been minimized.

@youknowriad

youknowriad Feb 8, 2019

Author Contributor

Can you expand more?

This comment has been minimized.

@danielbachhuber

danielbachhuber Feb 8, 2019

Member

The most immediate use-case that comes to mind is:

  1. Developer creates a block type which saves markup variation A. Developer also implements styling for variation A.
  2. Editorial publish dozens of blocks with markup variation A.
  3. Due to changing business requirements, developer modifies original block type to save markup variation B.

Currently, the developer must keep the styling around for markup variation A as well as implement styling for markup variation B. There's no way to programmatically transform all blocks with variation A into variation B.

Styling isn't the only dependency on markup. If the site has analytics tied to the markup, the associated logic for both variations will need to be kept around (growing in complexity over time).

If we had server-side transform definitions, the developer would be able to run a WP-CLI command that batch transforms all markup variation A into markup variation B.

The less-preferable workaround we have right now is to use dynamic blocks, which means markup isn't ever stored in the post content.

This comment has been minimized.

@youknowriad

youknowriad Feb 8, 2019

Author Contributor

Got it this ties to the deprectatedVersions api we have right now. I do think this workflow deservers improvements but I think it should be addressed separately from this RFC. The solution might be to add a new property here but I think it's a complex problem on its own to warrant a separate RFC/PR

This comment has been minimized.

@danielbachhuber

danielbachhuber Feb 11, 2019

Member

Got it this ties to the deprectatedVersions api we have right now. I do think this workflow deservers improvements but I think it should be addressed separately from this RFC. The solution might be to add a new property here but I think it's a complex problem on its own to warrant a separate RFC/PR

Fair enough, this works for me. Do you want to open a new issue for it? I'm not sure of the best contents for it.

I've thought of another use-case today: migrating (or importing) content into WordPress from another source. I'd prefer to migrate (import) to block-native format. At the moment, I'm left with either migrating to Classic editor format or hand-forming blocks.

This comment has been minimized.

@gziolo

gziolo Feb 21, 2019

Member

Fair enough, this works for me. Do you want to open a new issue for it? I'm not sure of the best contents for it.

I've thought of another use-case today: migrating (or importing) content into WordPress from another source. I'd prefer to migrate (import) to block-native format. At the moment, I'm left with either migrating to Classic editor format or hand-forming blocks.

Yes, those are both very good points. I think with this RFC we want to start the process of moving all basic metadata to JSON file to make it easy to consume by PHP code. This will surely open new options for further investigation. Definitely, deprecations and transformations are those aspects which might be somehow encoded in a way that could work on both sides. I also want to echo what @youknowriad said, this is complex and we want to keep this RFC very focused so we could finish the very first iteration in a few weeks so we could land it in let's say optimistically in WordPress 5.3 :)

Update docs/rfc/block-registration.md
Co-Authored-By: youknowriad <benguella@gmail.com>

@youknowriad youknowriad requested a review from notnownikki as a code owner Feb 8, 2019

@gziolo
Copy link
Member

gziolo left a comment

I'm personally very happy about the current state of this proposal. This is obviously only the first step which should allow us to be able to expose the same definitions through REST API. What is proposed here is a nicely scoped actionable refactoring which we could implement and promote quite quickly. This should also remove some confusion around blocks which use render_callback to handle save method on the server because at the moment it is unclear whether many properties should be included in JS or PHP file.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Feb 11, 2019

After some thinking, it seems like this PR falls short in the way the script dependencies are discovered. At the moment, there's no way to know which dependencies are required by each JavaScript file used in the block.json.

The question is:

1- Do we optimize the scripts for WordPress (which means they use wp.* modules and any wordpress script). and if this is the case, how do we statically analyze the dependencies of these scripts? should we have a separate assets.json to solve it?

2- Or do we optimize for "npm-like" contexts, which mean the scripts use import/require syntax for the dependencies? in which case WordPress can't load these without a build tool.


Initially, I think we should take the first approach here and introduce a assets.json file where each file path maps to script/styles dependencies (handles).

Thoughts?

* Property: `deprecated`

```json
{ "deprecated": [ { attributes, save, supports } ] }

This comment has been minimized.

@talldan

talldan Feb 12, 2019

Contributor

I wondered if another option for the deprecated property could be to link to another previous version of the block.json.

Creating a deprecation could be copying the current implementation into a subfolder, referencing it, and then making changes to the block in the current folder.

This comment has been minimized.

@youknowriad

youknowriad Feb 12, 2019

Author Contributor

It seems like a good option but would require the plugin author to keep a lot more code around. I'm not against though.

This comment has been minimized.

@gziolo

gziolo Feb 25, 2019

Member

This example needs to be further adjusted as it is not valid JSON at the moment.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 18, 2019

After some thinking, it seems like this PR falls short in the way the script dependencies are discovered. At the moment, there's no way to know which dependencies are required by each JavaScript file used in the block.json.

My gut reaction is to avoid yet-another file with assets.json, though admittedly I'm not entirely clear what format and role it would take.

An earlier thought was that if the value of a script property as defined in the manifest would be simply the script handle assumed to be registered by the plugin author elsewhere, then it would be fairly trivial for WordPress to resolve the dependencies. This, of course, does not work well outside a wp-admin context for the "generic" block.

I'd not like to set any expectation that all wp.* are available, especially if we don't yet have a plan for how to determine the specific dependencies of a given script. We should assume that as additional screens are implemented, we may introduce more script handles which ought only be loaded within the context of the specific screen (i.e. not all scripts for all screens).

The second proposal is not great if it were to require a build step, but part of me wonders if that would be strictly necessary (vs. some maybe-naive string replacement script preparation). There's an interesting alignment between a behavior implied with WPDefinedPropertyFile to assign some named export, if we could pair this with dependencies to align to the ES2015 import / export semantics.

import { createElement } from '@wordpress/element';
export const edit = () => createElement( 'input' );

There's many forms we could apply to create some manifest for individual scripts to declare their inputs (dependencies) and outputs (exported properties). We could even adopt the plugin / theme comment-based approach, which has an advantage in that it may be more okay to have WordPress-specific assumptions, as long as those assumptions are merely overlooked by virtue of being a code comment.

/**
 * Exposes: edit
 * Dependencies: wp-element
 */

Finally, I wonder if we could inherit npm's dependencies pattern, either literally as in each block is a package, with perhaps its manifest defined within the package.json:

{
	"name": "@wordpress/block-library-heading",
	"block": {
		"name": "core/heading",
		"title": "Heading"
	},
	"dependencies": {
		"@wordpress/element": "*"
	}
}

...or by inspiration where block.json has a dependencies definition which takes a similar shape:

{
	"name": "core/heading",
	"title": "Heading",
	"dependencies": {
		"@wordpress/element": "*"
	}
}
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Feb 19, 2019

The second proposal is not great if it were to require a build step, but part of me wonders if that would be strictly necessary (vs. some maybe-naive string replacement script preparation).

Maybe, it does feel fragile though 🤷‍♂️

Finally, I wonder if we could inherit npm's dependencies pattern, either literally as in each block is a package, with perhaps its manifest defined within the package.json:

That's exactly what I had in mind with the assets.json file, the idea is that a block is not a single file and the same file could be shared across blocks too which means it's file dependencies and not block dependencies and that's why I proposed a separate assets.json

gziolo added some commits Feb 21, 2019


Plugins and Themes can also register [custom block categories](https://wordpress.org/gutenberg/handbook/designers-developers/developers/filters/block-filters/#managing-block-categories).

An implementation should expect and tolerate unknown categories, providing some reasonable fallback behavior (e.g. a "common" category).

This comment has been minimized.

@gziolo

gziolo Feb 25, 2019

Member

So we have it covered. Nice. We will have to double check how strict block registration is at the moment. It's particularly important for the future work of WordPress.org blocks directory where you would be able to install plugins using the definition provided when using the inserter but you wouldn't have a given category registered as it is hidden inside PHP or JS code.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 25, 2019

@aduth and @youknowriad, I was thinking a lot about how assets could be registered moving forward with this JSON-based definition. I agree that this seems like the biggest decision left to consider this RFC concluded. I was tinkering about your proposals and I couldn't find a way to simplify it. At some point, I went back to our tutorials to see what we recommend as of today. I found this part about enqueuing assets:

https://github.com/WordPress/gutenberg/blob/master/docs/designers-developers/developers/tutorials/block-tutorial/applying-styles-with-stylesheets.md#enqueueing-editor-only-block-assets

It looks like we encourage people to reference assets through their registered handles:

register_block_type( 'gutenberg-boilerplate-esnext/hello-world-step-02', array(
	'editor_script' => 'gutenberg-boilerplate-es5-step02-editor',
	'editor_style'  => 'gutenberg-boilerplate-es5-step02-editor',
) );

I thought why don't we leave it as is for start? Which means we would have the following change in the JSON definitions:

-	"edit": "blocks/notice-edit.js",
-	"save": "blocks/notice-save.js",
+	"edit": "my-plugin-block-notice-edit",
+	"save": "my-plugin-block-notice-save",
+	"assets" "./blocks/notice.php",

What happens here is we still can optionally register all assets with PHP and then load them on demand in the client through their names. All dependencies would be handled in PHP file as this is the only place that needs to know them.

Now there is a new issue, how do I register such block in JavaScript only environment which doesn't use WordPress as a backend at all 😅

So maybe the idea of having an additional assets.json file or assets field defined as an object inside this block definition is the way to go?

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Feb 25, 2019

@gziolo One thing to note on that tutorial is that it is actually an example for registering assets outside of a block context; in that case it's for registering custom block styles.

gziolo added some commits Mar 5, 2019

@gziolo gziolo referenced this pull request Mar 5, 2019

Open

Introduce block metadata files for dynamic blocks #14238

10 of 14 tasks complete

gziolo added some commits Mar 5, 2019

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Mar 5, 2019

I started some code explorations in #14238. So far I had to do some quick hacks to re-register core blocks on the server given that #13521 is still not ready to go.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Mar 11, 2019

For the record, this is how Gutenberg cloud reads assets to be used with Drupal:

https://github.com/front/g-hero-section/blob/master/package.json#L6-L11

  "gutenbergCloud": {
    "js": "build/index.js",
    "css": "build/style.css",
    "screenshot": "screenshot.png",
    "name": "Hero section"
  },

the block itself is tagged as Gutenberg block on npm through keywords and has to be requested for code review (security purposes) before it lands in Gutenberg Cloud:

https://github.com/front/g-hero-section/blob/master/package.json#L28-L31

  "keywords": [
    "gutenberg",
    "gutenberg-cloud"
  ],

WordPress plugin: https://wordpress.org/plugins/cloud-blocks/

@gziolo gziolo referenced this pull request Mar 15, 2019

Open

WIP: Create WP Plugin #10628

@youknowriad youknowriad removed this from the Documentation & Handbook milestone Mar 18, 2019

@aduth aduth referenced this pull request Mar 18, 2019

Open

WIP - Allow plugins to extend core embed blocks #14050

0 of 5 tasks complete
### Save

* Type: `string` (`WPDefinedPropertyFile`)
* Optional

This comment has been minimized.

@gziolo

gziolo Mar 19, 2019

Member

At the moment save is required. I opened #14510 to make it optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.