-
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
Blocks: Ensure that metadata registered on the server for core block is preserved on the client (try 2) #29302
Blocks: Ensure that metadata registered on the server for core block is preserved on the client (try 2) #29302
Conversation
Size Change: +19 B (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
@noisysocks, as far as I can tell, we won't need to backport this version to WordPress 5.7 and it should be fine to use already backported commits. The regression reported should apply only to WordPress 5.6 and lower. |
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.
Such a speedy turnaround @gziolo ! ✨ I think this is pretty close. Smoke testing this across different versions we don't see the major regressions from last time. This change still works well with my experimental branch too.
I did perform a quick audit and I noticed we're missing the editorStyle
and style
attributes, from the server definition which may cause other issues.
Let's 🔍 more on that and maybe get a +1 for the review
if ( | ||
serverSideBlockDefinitions[ blockName ].apiVersion === | ||
undefined && | ||
definitions[ blockName ].apiVersion |
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 think I spotted a few more differences on the server definition:
- Missing
editorStyle
- Sometimes items in
supports
, get expanded in blockattributes
(I think this is okay) - Missing
style
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.
It might be worth setting up a test case with all block registration items set and then compare to see what's returned.
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.
Missing
editorStyle
Missingstyle
At the moment it's used only on the server to enqueue CSS files, for example:
https://github.com/WordPress/gutenberg/blob/master/docs/designers-developers/developers/block-api/block-metadata.md#editor-style
In the future, we might need it on the client to lazy load CSS or JS files for the block.
Edit: In fact, those fields were used in the past for installing blocks from Block Directory, but it was replaced with a different approach in #24117. However, in practice, Block Directory uses its own endpoint to get the block metadata which is further processed block.json
file:
gutenberg/packages/block-directory/src/store/resolvers.js
Lines 24 to 26 in 8ab2453
const results = yield apiFetch( { | |
path: `wp/v2/block-directory/search?term=${ filterValue }`, | |
} ); |
Sometimes items in
supports
, get expanded in blockattributes
(I think this is okay)
Yes, it's used quite extensively for features built-in in the block editor like alignment, custom class name, anchor, fonts, colors, and other stuff that Global Styles offer. Most (hopefully all) of the hooks that add those fields for attributes
respect the version created by developers.
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.
Great, sounds like this should be relatively safe to try again. ✨
I tested and can confirm that the block wrappers are correct on 5.6.2 |
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.
Thanks @gziolo! Let's maybe give folks a heads up when this gets merged to see if they spot any odd behavior we missed.
I will merge tomorrow to make sure I'm around if something goes wrong 😄 |
Description
2nd attempt to land #29213 that revert the revert from #29279. This time it should cover the following issue when using WordPress 5.6:
New changes include the polyfill logic when
apiVersion
is present inblock.json
but it isn't exposed from the server.This PR resolves the issue where metadata registered on the server from
block.json
would get overridden by the same data registered again on the client. The fix proposed ensures that for all blocks that have some metadata present, we skip all other attempts to pass the same metadata.unstable__bootstrapServerSideBlockDefinitions
was never meant to stay for so long, but here we are. It isn't part of public API so we shouldn't be worried about backward compatibility that much.It's a blocker for #29095.
It also was caught by @audrasjb when compiling the dev note for new filters added for
register_block_type_from_metadata
. I included e2e tests that ensure that one of the new filters is properly propagated to the client.How has this been tested?
There is a new e2e test added that ensures that the updated logic works correctly:
There is a new unit test added that ensures that polyfilling works as expected.
Use WordPress 5.6 and make sure that all blocks have the same class names applied as in WordPress 5.7. It's related to the
apiVersion
presence during block registration.For example, as reported by @glendaviesnz:
I tested with WordPress 5.6 and the Gutenberg demo page:
Block wrapper is correctly applied:
No `apiVersion in the response from the server:
Types of changes
Bug fix (non-breaking change which fixes an issue).
Checklist: