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

Block library: Try to use Babel plugins to inline block.json metadata #14551

Merged
merged 3 commits into from
Apr 2, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Mar 21, 2019

Description

It's based on RFC: #13693.

We discussed it with @youknowriad earlier today. I think it looks very promising, the idea is that we inline block.json on the client to keep everything backward compatible with Drupal, Calypso, mobile
it also allows us to completely remove the existing script which generated server-side rendered definitions:
https://github.com/WordPress/gutenberg/blob/master/test/integration/full-content/server-registered.json

Testing

block.json is used for deprecated Text Columns block. To ensure it still works you can paste the following HTML code in the code editor mode:

<!-- wp:text-columns -->
<div class="wp-block-text-columns alignundefined columns-2"><div class="wp-block-column"><p>Column 1</p></div><div class="wp-block-column"><p>Column 2</p></div></div>
<!-- /wp:text-columns -->

Future

Moving forward we can add a few improvements by creating our own plugin:

  • make it aware of translations
  • structure generate code in a way which allows dead code elimination for Gutenberg and WordPress
  • to do the previous, only ship metadata on the client if server-side registration isn’t used

In Gutenberg, we want to register all blocks on the server with PHP and don’t repeat it on the client. For the mobile app, there is a need to have those definitions to be loaded in JS as of today and maybe through network-based API later. Standalone Gutenberg which Riad works on would benefit from purely client-side block definitions loading. I'm fully aware, it’s all confusing 😅 With the proposed trick we can continue shipping npm modules which work out of the box even when blocks are partially defined in the JSON file. At the same time, we can introduce an env variable like process.env.BLOCK_METADATA_SOURCE (similar to process.env.GUTENBERG_PHASE) and don’t include code behind this flag in JS bundle when its value is set to server through some webpack magic. To better illustrate this idea, I hand-crafted example output which custom Babel plugin could generate:

// input
import metadata from './block.json';
{
	"name": "core/text-columns",
	"icon": "columns",
	"category": "layout",
	"attributes": {
		"content": {
			"type": "array",
			"source": "query",
			"selector": "p",
			"query": {
				"children": {
					"type": "string",
					"source": "html"
				}
			},
			"default": [ { }, { } ]
		},
		"columns": {
			"type": "number",
			"default": 2
		},
		"width": {
			"type": "string"
		}
	},
	"supports": {
		"inserter": false
	}
}
// output
var metadata = {
  name: "core/text-columns",
};
if (process.env.BLOCK_METADATA_SOURCE !== 'server') {
	metadata = Object.assign({}, {
		icon: "columns",
		category: "layout",
		attributes: {
			content: {
				type: "array",
				source: "query",
				selector: "p",
				query: {
					children: {
						type: "string",
						source: "html"
					}
				},
				"default": [{}, {}]
			},
			columns: {
				type: "number",
				"default": 2
			},
			width: {
				type: "string"
			}
		},
		supports: {
			inserter: false
		}
	}, metadata);
}
exports.metadata = metadata;

@gziolo gziolo added [Type] Enhancement A suggestion for improvement. [Feature] Block API API that allows to express the block paradigm. [Status] In Progress Tracking issues with work in progress [Package] Block library /packages/block-library [Feature] Block Directory Related to the Block Directory, a repository of block plugins labels Mar 21, 2019
@gziolo gziolo self-assigned this Mar 21, 2019
/**
* Internal dependencies
*/
import metadata from './block.json';
Copy link
Member Author

Choose a reason for hiding this comment

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

It gets transpiled into:

Screen Shot 2019-03-21 at 16 47 22

@@ -3,5 +3,6 @@ module.exports = function( api ) {

return {
presets: [ '@wordpress/babel-preset-default' ],
plugins: [ 'babel-plugin-inline-json-import' ],
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something that can be useful for our default preset?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think it would land there eventually once proved that we like this approach 👍

By the way, this plugin still depends on Babel 6. Not sure it's an issue, just noting.

},
"supports": {
"inserter": false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like I have a lot to say about these :), can we start with something very small like the "category"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I know this PR is more about the way javascript consumes this file rather than about the content of the file directly and that's why I suggest to keep it to the minimum for the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't care what we include, I opted for everything that server is able to expose to the client:

https://github.com/WordPress/wordpress-develop/blob/689ba4eec66b8d735c57940303ea0d49657f4cb3/src/wp-admin/includes/post.php#L2206

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed it's not very important for now (to filter or not) but the shape of some of these properties might need to be changed later.

Copy link
Member Author

Choose a reason for hiding this comment

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

supports removed, should I remove anything else?

supports: {
inserter: false,
},
export { metadata };
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the idea later is to have a feature flag to set metadata to undefined in Core and Gutenberg when these are provided by the server right?

Copy link
Member Author

@gziolo gziolo Mar 21, 2019

Choose a reason for hiding this comment

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

@aduth
Copy link
Member

aduth commented Mar 27, 2019

In Gutenberg, we want to register all blocks on the server with PHP and don’t repeat it on the client. For the mobile app, there is a need to have those definitions to be loaded in JS as of today and maybe through network-based API later. Standalone Gutenberg which Riad works on would benefit from purely client-side block definitions loading.

So, the contents of block.json would be bundled with the JavaScript. But in the context of WordPress, the server would register the block using the same block.json, largely to the effect that the registration can become "known" PHP-side. And we still bootstrap this into the client, which takes precedent over the bundled metadata (though likely it is the same).

Am I understanding correctly?

@gziolo
Copy link
Member Author

gziolo commented Mar 27, 2019

So, the contents of block.json would be bundled with the JavaScript. But in the context of WordPress, the server would register the block using the same block.json, largely to the effect that the registration can become "known" PHP-side. And we still bootstrap this into the client, which takes precedent over the bundled metadata (though likely it is the same).

In overall, this is exactly what I'm envisioning with one small remark. Once this data is available on the server and is discoverable through API, we should always serve it from the server. Therefore the need to have it bundled into the client no longer makes sense. To avoid code duplication and make the bundle size smaller, we can remove this metadata from the client in a WordPress context.

@aduth
Copy link
Member

aduth commented Mar 27, 2019

Therefore the need to have it bundled into the client no longer makes sense. To avoid code duplication and make the bundle size smaller, we can remove this metadata from the client in a WordPress context.

Gotcha. That would be good; the one thing which stood out to me is that we'd have the data be duplicated. I suppose the reason for doing it this way is to allow for a more iterative conversion to a system where blocks become known by fetching data from an API, rather than explicitly registering as we do today in edit-post? It's still unclear (and perhaps yet-unsolved, I'm unsure) how the JavaScript implementation of the block would become known to the editor in this system. Or am I mistaken in thinking on this process of how blocks become known to the editor? It seems like it could be the necessary path for dynamically loading new blocks via e.g. some of the inserter concepts for installing new blocks.

@gziolo
Copy link
Member Author

gziolo commented Mar 27, 2019

I suppose the reason for doing it this way is to allow for a more iterative conversion to a system where blocks become known by fetching data from an API, rather than explicitly registering as we do today in edit-post?

Yes, that is exactly what would happen at some point. Server and client would have the same list of all blocks expressed with metadata format. That would make it possible to automatically register blocks exposed by the server rather than doing it explicitly as of today. However, it's still unclear to me how we would connect metadata with block settings which can be expressed only with JavaScript. On the other hand, things like edit implementation aren't necessary until the block is inserted into the editor so it doesn't have to be registered immediately. This makes it more complicated but at the same time creates very interesting opportunities to load have logic only when it's actually needed.

It's still unclear (and perhaps yet-unsolved, I'm unsure) how the JavaScript implementation of the block would become known to the editor in this system. Or am I mistaken in thinking on this process of how blocks become known to the editor? It seems like it could be the necessary path for dynamically loading new blocks via e.g. some of the inserter concepts for installing new blocks.

This is one of the possible flows we could implement:

  • from the Inserter you call API which returns the list of block metadatas from WordPress.org Block Directory
  • based on that you somehow display them in the inserter (name, title, description, category, keywords is all you need probably)
  • when you select the block, behind the scenes it gets installed on the server, the same way as if you would go to the Plugins page
  • placeholder block is inserted into the editor
  • once it’s installed, JS and CSS gets loaded into the editor, placeholder blocks gets transformed into this newly installed block

@gziolo gziolo marked this pull request as ready for review March 27, 2019 14:35
@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Mar 27, 2019
@gziolo
Copy link
Member Author

gziolo commented Mar 27, 2019

@youknowriad @aduth - do you feel like we can land this PR as is and continue with the following roadmap:

  • use inline JSON babel plugin as the first step with one block (this PR)
  • update more client-side blocks to work with this plugin
  • replace inline JSON Babel plugin with our own version which supports i18n
  • add more fields to block.json
  • update Babel plugin to bundle block.json metadata only when not using server-side registration
  • figure out the way to register all static blocks on the server with some custom Gutenberg specific loader based on the block.json

Some tasks can be done in parallel which might make the development process more convenient.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

very very thanks and good luck

packages/block-library/README.md Outdated Show resolved Hide resolved
packages/block-library/src/index.js Outdated Show resolved Hide resolved
babel.config.js Show resolved Hide resolved
packages/blocks/src/api/registration.js Show resolved Hide resolved
packages/block-library/src/text-columns/block.json Outdated Show resolved Hide resolved
@gziolo
Copy link
Member Author

gziolo commented Apr 1, 2019

Can we merge it as is and work on follow-up PRs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Block Directory Related to the Block Directory, a repository of block plugins [Package] Block library /packages/block-library [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants