Block Deprecations: Provide extra data for isEligible check#48815
Merged
aaronrobertshaw merged 4 commits intotrunkfrom Mar 15, 2023
Merged
Block Deprecations: Provide extra data for isEligible check#48815aaronrobertshaw merged 4 commits intotrunkfrom
aaronrobertshaw merged 4 commits intotrunkfrom
Conversation
aaronrobertshaw
added
[Type] Enhancement
aaronrobertshaw
requested review from
ajitbohra,
fabiankaegy,
juanmaguitar and
ryanwelcher
as code owners
Contributor
Author
Contributor
Author
|
@youknowriad would you mind weighing in on this change or suggesting a better alternative? |
|
Size Change: +1.83 kB (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 2532a46. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4422539506
|
Contributor
|
I think we've found in the past that changing |
youknowriad
reviewed
Mar 7, 2023
Contributor
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
To be honest, I don't like the current isEligible API, and I'd say that our deprecation behavior/algorithm is not perfect at all but this is going to be a huge effort to improve. Now passing the block like done in this PR feels a bit random. Maybe a new API is a bit better if this proves to be really necessary.
Contributor
Author
|
Thanks @youknowriad 👍
Could this PR around the layout At first glance, changing the default layout for the Group block definitely had wider ranging impacts. The media and text block default alignment seemed reduced in scope but I'm happy to be warned of unconsidered pitfalls though.
100% agreed. |
dmsnell
reviewed
Mar 8, 2023
Member
dmsnell
left a comment
dmsnell
left a comment
There was a problem hiding this comment.
Thanks for the ping @youknowriad. Sharing my approval here optimistically, to note my approval of the idea, not to signal that you ought to merge this without the review and feedback from others 😉
Now passing the block like done in this PR feels a bit random. Maybe a new API is a bit better if this proves to be really necessary.
Given that deprecations already have trouble with the designed API I'm more inclined to let the extra parameters in. To that end I think rawBlock is more appropriate than block because of the implications of block at this point.
We can call rawBlock the blockNode (hat-tip to @mcsf who I think coined that term) or the "stage-1 parse of the block," which is what we get when parsing the serialized HTML.
The block object here, or the "stage-2 parse," is the result of feeding that block node into the code for the given block type.
The distinction might be important here because we're in a situation where we know something didn't load as expected and that block is likely invalid (according to what the editor calls "invalid"). It can be misleading because it may have lost correspondence with the serialized HTML. For example, the list block updated to use inner blocks, so old list blocks will have the innerHTML in that block node with the <ul> (or <ol>) and <li> tags, but will render an empty list into block, because there are no inner blocks.
A deprecation would need to restore the original content that in a fraction of the cases will be wiped out before running the deprecation code.
our deprecation behavior/algorithm is not perfect at all but this is going to be a huge effort to improve
this is another reason that being loose here feels favorable: an overhaul of the block updating and validation-handling code might wipe out a number of these fixes through very different means and I suspect there's little likelihood we can anticipate now how those will appear later.
Contributor
Author
|
Thanks for weighing in @dmsnell you make some great points 👍 It still appears to me that there could be some benefit in proceeding with the addition of the extra args to I don't think there is a huge rush on this so it can wait for sufficient reviews for everyone to be confident in the change. |
talldan
reviewed
Mar 14, 2023
talldan
approved these changes
Mar 15, 2023
Contributor
talldan
left a comment
talldan
left a comment
There was a problem hiding this comment.
Thanks for reworking the docs.
I left a few suggestions for improving consistency, as there were a few different terms being used (like 'serialized HTML' vs 'saved markup') that mean the same thing, but I leave these up to you.
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
Contributor
Author
Thanks. The suggestions look good. I've committed those and will merge once the e2es pass. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Related:
none#48404What?
Provides additional data from which deprecations can check if they are eligible.
Why?
The information currently passed to the
isEligiblecheck is insufficient in some cases to determine if a deprecation should in fact run.One use case requiring additional information can be found in:
none#48404In the above example, when the default
alignattribute has its default switched tonone, existing blocks using the defaultwidealign are not determined to be invalid. This is due to the custom CSS class name support picking up the deprecatedalignwideclass from the markup and adding it to theclassNameattribute.As the deprecated block is still determined to be valid, the deprecation's
isEligiblecallback must be able to return true in this scenario. The catch is presently theisEligiblefunction is passed only the raw parsed attributes and inner blocks. The raw parsed attributes do not yet have any amendments made to theclassNameattribute by the custom class names support.How?
isEligiblefunction. That being and object now including the current block e.g.{ block }.isEligiblefunction definitions.Alternative Approaches
Pass only
rawBlockand currentblockas extra paramsThis would avoid the creation of a new object and potential minimise any performance hit. It would make for a rather awkward signature and API.
isEligibleV2This approach would allow deprecations to define their eligibility check via a new function signature.
e.g.
isEligibleV2( rawBlock, currentBlock ).This would still provide the existing data via
rawBlock.attributesandcurrentBlock.innerBlocksbut also allow checks to be made against the markup or finalized block attributes.Some downsides are the multiple function signatures, the potential need to deprecate one eventually, potential confusion etc. The upside is primarily the clean function signature and only passing the extra data when a deprecation desires it.
Adding a disallow list to Custom CSS Classname support
This is a more heavy-handed and hacky approach with a greater potential performance hit.
Blocks could define a disallow list for custom class names. That list can be checked during the determination of custom class names from the block's markup. A CSS class is only added to the
classNameattribute if it wasn't in the disallow list.Using the media and text example,
alignwidecould be disallowed, which would then cause unmigrated blocks to be invalidated triggering the necessary deprecation without the need for theisEligiblecheck.Testing Instructions
npm run test:unit test/integration/full-content/full-content.test.jsnone#48404 which is now based on this PR