Block registration: normalize blockType.parent to array
#66250
Conversation
oandregal
added
[Type] Bug
|
Size Change: +21 B (0%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 4369ec0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11459370653
|
oandregal
force-pushed
the
update/cast-string-parent-to-array
branch
3 times, most recently
from
164b81c to
09697e3
Compare
|
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. |
oandregal
changed the title
string cast it to arrayarray
|
This is ready to review. I can confirm that using a string as a parent worked (registered the block), though it had side-effects (child blocks could be added top-level + could be added within themselves). This PR fixes that and normalises Added some testing instructions and covered both block registration + the |
| // Parent being an array hasn't been enforced in the past, | ||
| // so this is a way to maintain backwards compatibility | ||
| // with 3rd party blocks that may have been using it as a string. | ||
| if ( |
There was a problem hiding this comment.
Isn't the parent prop in metadata (if an object is passed)? If that's the case and the metadata are not passed we should get them and make the check.
Additionally can we simplify the checks here to explicitly check what we want? Something like:
if (
blockNameOrMetadata.parent &&
! Array.isArray( blockNameOrMetadata.parent ) &&
typeof blockNameOrMetadata.parent === 'string'
) {
blockNameOrMetadata.parent = [ blockNameOrMetadata.parent ];
}
There was a problem hiding this comment.
Isn't the parent prop in metadata (if an object is passed)? If that's the case and the metadata are not passed we should get them and make the check.
Oh, I missed that, thanks for bringing it up! I'm a bit confused about this for some reasons (existing code, docs and other implementations tell otherwise), but specially because the PR worked in my tests.
Additionally can we simplify the checks here to explicitly check what we want?
What we want to do is "if the parent is a string, normalize it to an array". In my view, the current code is actually more explicit, simpler (why to check both "is it not an array" and "it is a literal string?"), and more comprehensive (String objects).
There was a problem hiding this comment.
So, we actually have to use settings, just not in registerBlockType but in processBlockType. This is the data flow for block registration:
registerBlockType( nameOrMetadata, settings )addBootstrappedBlockType=> if metadata provided, creates a new tree in the store with that dataaddUnprocessedBlockType- calls
processBlockType=> takes the existing block metadata & merges it with the settings- merges metadata & settings into
blockType - dispatch
'blocks.registerBlockType'filter withblockType - validates the
blockTypeafter the filter has been dispatched
- merges metadata & settings into
- creates a new tree in the store with the blockType data
- calls
- return
blockType(settings & metadata consolidated)
I've moved it all to processBlockType (including existing validation for the parent that was done too early) in 002ac7cec3337f5f5d56c9f154a3e337b890f3c4.
There was a problem hiding this comment.
After having looked at this more deeply, there's a situation in which this could be considered a breaking change:
- 3rd party code registers the blockType with parent as a string.
- Before: parent will be a string in the store.
- After: parent will be an array in the store.
Note that I intentionally added the normalization code after the filter, as to not interfere with 3rd party code before they executed their callback. However, if the 3rd party code queries the store for the parent after registration to do something with the parent, it will no longer be a string.
I don't see a way out of this and it seems to me that this PR fixes an important issue (being able to add a child block within itself). This PR will also be merged very early in the WordPress 6.8 cycle, so we'll have time to gather feedback.
Thoughts?
There was a problem hiding this comment.
which this could be considered a breaking change
That's true, but I'm not sure if we should consider a breaking change something that was used in a wrong way and differently than documentation. We had a similar issue with description in the past, where some plugins would provide a component and we handled that the same way as you did (with a small difference deprecated vs warning).
Additionally, it sounds unusual to use parent from store and this PR seems good to me regarding this aspect.
There was a problem hiding this comment.
As long as we log the offence, I'm ok with this. Any third-party logic interested in monitoring blocks' parent setting would quickly notice that core blocks use arrays, unless that logic were really built very specifically for a third-party block using parent as a string. It's possible that someone is doing that, but not too likely.
We can play it safe and communicate this as a breaking change in dev notes, even though it isn't.
There was a problem hiding this comment.
We can play it safe and communicate this as a breaking change in dev notes, even though it isn't.
Added the label and some text for the devnote.
oandregal
force-pushed
the
update/cast-string-parent-to-array
branch
from
270308c to
4369ec0
Compare
|
I'd welcome a double check of #66250 (comment) before merging this PR — I'll let it sit for a few days and will merge if no one opposes. |
| @@ -197,5 +197,41 @@ export const processBlockType = | |||
| return; | |||
| } | |||
|
|
|||
| if ( | |||
| typeof settings?.parent === 'string' || | |||
There was a problem hiding this comment.
Copied the comment from the other PR:
I'm curious why we'd need both typeof blockType.parent === 'string' || blockType.parent instanceof String..
Otherwise, we don't cover all potential issues. Note the issue was that we checked for
.lengthin a type that wasn't an array (didn't have.some). That's the case for both literal & object strings.
I still don't see why is needed. Can you give me an example?
In general my suggestion before for simplifying the checks by what we expect and not what we don't expect. With that code parent could be a boolean etc..
And if we wanted to be thorough we should also check if an array is provided, that consists of strings.
There was a problem hiding this comment.
Ok, these are the facts:
-
I discovered that we were trying to detect arrays the wrong way while responding to Gutenberg forum issues. We were checking for
blockType.parent?.length, but that's not enough to detect arrays — it can be a string, any type of it, as it was. I prepared a hotfix but wanted to tackle the core issue: that we didn't normalize the parent data coming from 3rd parties — that's what this PR is for. I presume we have more of these in our codebase are causing issues for the same reason. -
Strings can be literal or object. Both have the
.lengthproperty and are boxed/unboxed depending on what you do. So
console.log( 'string'.length ); // Logs 6
console.log( new String( 'string' ).length ); // Logs 6log the same.
Back to our problem. Block authors should have been using arrays to declare the parent, but, in some cases, they weren't — instead they used strings of any type. They can use either type, that's why we need to cover for both.
Does this help?
| return; | ||
| } | ||
|
|
||
| if ( 1 === settings?.parent?.length && name === settings.parent[ 0 ] ) { |
There was a problem hiding this comment.
I understand this check was carried from before, but why the check for 1? Wouldn't the following be better in all respects?
| if ( 1 === settings?.parent?.length && name === settings.parent[ 0 ] ) { | |
| if ( settings?.parent?.includes?.( name ) ) { |
For blocks registered using a string for the While string values were never officially supported for the
const blockType = select( blocksStore ).getBlockType( name );
if ( name === 'my/block' ) {
// Operate with blockType.parent as if it was a string.
} |
oandregal
added
the
Needs Dev Note
|
Merged this one. Happy to continue conversation or address any follow-ups people run into. |
…66250) Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: mcsf <mcsf@git.wordpress.org>
Follow-up to #66234
What?
Casts
blockType.parentto array if it's a string.Why?
While we haven't enforced it in any way, we have been operating under the assumption that
blockType.parentis an array. Apparently, not always is and blocks may register it using a string. When processing input from 3rd parties, we should enforce the type so it's safe to operate with it during the whole lifecycle of the block (e.g.: to avoid issues like this one).How?
blockType.parentis a string #66234'blocks.registerBlockType'filter fe4e3beTesting Instructions
Update the
parentof a core block to be a string instead of array (e.g.: update the column block).Test the following scenarios:
+button):Trunk
Screen.Recording.2024-10-21.at.17.16.47.mov
This PR
Screen.Recording.2024-10-21.at.17.10.55.mov