-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
/** | ||
* Internal dependencies | ||
*/ | ||
import metadata from './block.json'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -3,5 +3,6 @@ module.exports = function( api ) { | |||
|
|||
return { | |||
presets: [ '@wordpress/babel-preset-default' ], | |||
plugins: [ 'babel-plugin-inline-json-import' ], |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 | ||
} |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 }; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. That would be optimal to use dead-code elimination. I'm not quite sure how it would work with deprecations which depend on attributes
though :)
Gallery block is a great example:
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/gallery/index.js#L21-L71
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/gallery/index.js#L229
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/gallery/index.js#L289
https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/gallery/index.js#L324-L334
So, the contents of Am I understanding correctly? |
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. |
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 |
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
This is one of the possible flows we could implement:
|
9a0ad89
to
c904146
Compare
@youknowriad @aduth - do you feel like we can land this PR as is and continue with the following roadmap:
Some tasks can be done in parallel which might make the development process more convenient. |
There was a problem hiding this 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
c904146
to
a47680a
Compare
a47680a
to
98786ad
Compare
Can we merge it as is and work on follow-up PRs? |
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, mobileit 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:Future
Moving forward we can add a few improvements by creating our own plugin:
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 toprocess.env.GUTENBERG_PHASE
) and don’t include code behind this flag in JS bundle when its value is set toserver
through some webpack magic. To better illustrate this idea, I hand-crafted example output which custom Babel plugin could generate: