Server-side render: Test for accuracy of removeBlockSupportAttributes #48898
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcsf
added
[Type] Code Quality
|
Size Change: +47 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in aae8d1a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4357732799
|
There was a problem hiding this comment.
Thanks for your efforts on this @mcsf 👍
I appreciate this is still a work in progress but was just wondering if we might be able to leverage the theme.json schema to sanity-check known styles.
It has the definitions.stylesProperties.properties object that lists all but the special cases of anchor and layout supports.
One catch with the current STYLE_KEY_PATTERN could be a block support that makes heavy use of _wp_array_get instead of $block_attributes['style']['styleName']['...'] etc.
All in all, though, it would be great to provide move certainty that ServerSideRender is kept up to date with new block supports. There's already so many pieces to introducing a block support, missing an update somewhere is easy to do.
mcsf
force-pushed
the
try/server-side-render-accuracy-tests
branch
from
9f81571
to
ac70c9f
Compare
|
Thanks for the feedback, @aaronrobertshaw!
This makes a lot more sense than the dirty thing I did. :) |
|
However, there are discrepancies:
(schema sourced using |
|
Thanks for the comparison breakdown, handy stuff. 👍 I did a little further digging and here's what I found:
In summary, maybe the theme.json schema could still be useful if we get the schema fixed up and perhaps combine the styles with either an arbitrary list for the special cases like layout & position, or the settings properties. This all highlights how fraught the process of adding block supports is currently. Having a single source of truth would be a welcome addition I think. |
|
Hey, @aaronrobertshaw, thanks a lot for the sharing those details. I just got back from a meetup and I'm catching up.
Looking at this with fresh eyes, I am now of the mind that this exploration is too costly for the payoff: we are talking about a niche prop ( On the spot it felt like a nice and quick improvement, but now I am much more concerned about the time I am stealing from you and myself on this task, so I'll close it! Anyone with a solid and cheap plan is free to reopen, of course. :) Thanks again, @aaronrobertshaw! |
I'm 100% on board with that assessment. Thanks all the same for taking the time to explore it though 🙇 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #44491 (comment)
Why?
In #44491, the
ServerSideRendercomponent gained a newskipBlockSupportAttributesprop used to strip blocks' attributes of block-supports-related attributes and styles. This is implemented in a local function,removeBlockSupportAttributes, in which those known attributes and styles were hardcoded.I raised the concern in #44491 (comment) that this could easily get out of sync, as over time block-supports features are added or edited, since there is no obvious connection between that subsystem and the
ServerSideRendercomponent.What?
This PR explores a solution to that problem by adding unit tests dedicated to
removeBlockSupportAttributes.How?
These tests are a bit atypical, since they they use Node's
fsAPI to grep through source files inlib/block-supportsto collect the names of attributes and styles that potentially correspond to block-supports features.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast