Process Block Type: Copy deprecation to a new object instead of mutating when stabilizing supports #66849
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
…ing when stabilizing supports
andrewserong
added
[Type] Bug
andrewserong
requested review from
talldan,
ramonjd,
tellthemachines and
aaronrobertshaw
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +18 B (0%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
3 tasks
|
Flaky tests detected in 4805a05. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11734379549
|
|
Appreciate the quick turnaround on this fix @andrewserong 🚀 The current approach in this PR appears to solve the primary concern and issue. I wonder if we could make the overall stabilization of supports a little more robust for other plugins? Could a plugin perhaps create a series of blocks that they registered via JS but use a shared typography supports definition or something that they import into their blocks' config? In other words, if the config objects for individual supports, e.g. typography, color, spacing, border etc., were also immutable the new I'm not trying to argue we need to cover every weird and wonderful way these objects might be constructed but maybe tweaking Here's a very rough and dirty example of what I meanfunction stabilizeSupports( rawSupports ) {
if ( ! rawSupports ) {
return rawSupports;
}
const newSupports = {};
for ( const [ key, value ] of Object.entries( rawSupports ) ) {
if ( key === 'typography' && typeof value === 'object' && value !== null ) {
newSupports.typography = Object.fromEntries(
Object.entries( value ).map( ( [ typographyKey, typographyValue ] ) => [
TYPOGRAPHY_SUPPORTS_EXPERIMENTAL_TO_STABLE[ typographyKey ] || typographyKey,
typographyValue,
] )
);
} else {
newSupports[ key ] = value;
}
}
return newSupports;
}I don't have a strong opinion on this, so feel free to disregard if you think it is overkill. |
|
I'll take @aaronrobertshaw's advice on this one. I'm happy to rubber stamp it, but admittedly, I'm not 100% across the scope. |
…t instead of updating the reference for the typography key
|
Thanks for the quick reviews!
Good thinking @aaronrobertshaw, I like the code snippet you've provided — overall it feels both safer and more "correct" to me if we're carefully constructing the new object rather than creating it and then swapping out the reference for In practice, I wasn't able to reproduce any issues with the approach before applying that snippet — because we're already creating a fresh object, when All that said I have more confidence in your suggestion than what we currently have, and it sets a good example for future changes to that function, so IMO even if it doesn't explicitly fix anything immediate, it's still a good quality change. I've pushed an update in 3fe635d Let me know if anyone would like any other changes, happy to keep iterating here! My overall hope, aside from fixing the reported bug, is that we can build the block supports stabilization in a way that feels stable and robust, so any and all code quality ideas are very welcome 🙂 |
aaronrobertshaw
approved these changes
Nov 8, 2024
There was a problem hiding this comment.
I was just about to approve the first iteration when the new commit came through, you're too quick @andrewserong! ⚡
I've tested the latest and it's working well for me.
✅ Could replicate the original issue with Jetpack active
✅ This PR resolves the issue
✅ Deprecations still function (block fixture tests are all passing as well)
✅ Stabilization of typography supports tests pass
LGTM 🚢
andrewserong
deleted the
fix/process-block-type-skip-try-not-to-mutate-deprecation-directly
branch
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
…ing when stabilizing supports (WordPress#66849) * Process Block Type: Copy deprecation to a new object instead of mutating when stabilizing supports * Update stabilizeSupports to carefully construct the newSupports object instead of updating the reference for the typography key Co-authored-by: andrewserong <andrewserong@git.wordpress.org> Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: Karen Attfield <karenlattfield@gmail.com>
|
Quick update: As discussed in #67729 (comment), the approach to block support stabilization needs more time so will likely be punted from 6.8. There is a revert PR available (#68163) and anyone interested in stabilized block supports is advised to follow that for the latest 🙏 |
ramonjd
added a commit
that referenced
this pull request
Dec 24, 2024
…alization and default controls supports (#68163) * Revert "Global Styles: Fix handling of booleans when stabilizing block supports (#67552)" This reverts commit 4335c45. * Revert "Block Supports: Extend stabilization to common experimental block support flags (#67018)" This reverts commit 5513d7a. * Revert "Borders: Stabilize border block supports within block processing (#66918)" This reverts commit b5ee9da. * Revert "Process Block Type: Copy deprecation to a new object instead of mutating when stabilizing supports (#66849)" This reverts commit 457fcf8. * Revert "Typography: Stabilize typography block supports within block processing (#63401)" This reverts commit 48341a1. Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org>
Gulamdastgir-Momin
pushed a commit
to Gulamdastgir-Momin/gutenberg
that referenced
this pull request
Jan 23, 2025
…alization and default controls supports (WordPress#68163) * Revert "Global Styles: Fix handling of booleans when stabilizing block supports (WordPress#67552)" This reverts commit 4335c45. * Revert "Block Supports: Extend stabilization to common experimental block support flags (WordPress#67018)" This reverts commit 5513d7a. * Revert "Borders: Stabilize border block supports within block processing (WordPress#66918)" This reverts commit b5ee9da. * Revert "Process Block Type: Copy deprecation to a new object instead of mutating when stabilizing supports (WordPress#66849)" This reverts commit 457fcf8. * Revert "Typography: Stabilize typography block supports within block processing (WordPress#63401)" This reverts commit 48341a1. Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: talldan <talldanwp@git.wordpress.org>
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.
What?
Follows #63401 and attempts a fix for the issue flagged in #63401 (comment)
When applying block supports stabilization while processing block types, try to avoid mutating the deprecation, in case the passed in object does not support setting keys. This can be the case in some plugins as was flagged in the linked comment, and in the Jetpack plugin (Automattic/jetpack#40055 and Automattic/jetpack#40070).
Thank you to @coder-karen for flagging this issue!
Why?
As seen in the Jetpack plugin in Automattic/jetpack#40070, some plugins structure their deprecations in such a way that they're imported rather than constructed from ordinary JS objects. I.e. in the Jetpack plugin, it uses a pattern like the following:
Where in each of those files, the keys are exported like so:
So, the object for each deprecation is not a regular JS object. Arguably the above approach isn't what's expected from the perspective of Gutenberg's code, but given that plugins are already writing things this way, let's ensure that it's supported, so as not to break plugins and websites!
How?
For each deprecation, create a new object and expand the keys into the new object. This should ensure that we're updating
supportsin a safe(r) way.Update the test for stabilizing block supports in the deprecation by calling
Object.freeze( blockSettings.deprecated[ 0 ] );— this emulates the approach seen in the Jetpack plugin by ensuring that thedeprecatedobject's keys cannot be mutated.Testing Instructions
trunkI get a white screen of death with an error message, whereas with this PR, the block displays as expected in the editor.Screenshots or screencast
The following are screenshots from running the Jetpack plugin on a post that includes the Slideshow block from that plugin. (If you're testing Jetpack, you'll need to switch on Jetpack Blocks under Jetpack Plugin Settings > Writing > Composing > Jetpack Blocks.)