Block Editor: Ensure that blockType is defined when accessing apiVersion
#34346
Conversation
gziolo
added
[Type] Code Quality
| @@ -111,11 +111,11 @@ function BlockListBlock( { | |||
|
|
|||
| const blockType = getBlockType( name ); | |||
| const lightBlockWrapper = | |||
| blockType.apiVersion > 1 || | |||
| blockType?.apiVersion > 1 || | |||
| hasBlockSupport( blockType, 'lightBlockWrapper', false ); | |||
There was a problem hiding this comment.
@ellatrix, does lightBlockWrapper handling exist only for backward compatibility? I don't see it used with core blocks anymore. I see it referenced in multiple places in the block editor package. Could we set apiVersion to 2 in a hook during registration to simplify all those checks?
There was a problem hiding this comment.
Yes, it's for backward compatibility.
Could we set apiVersion to 2 in a hook during registration to simplify all those checks?
What do you mean?
There was a problem hiding this comment.
registerBlockType filter could detect if there is lightBlockWrapper set in supports and when it's true then inject apiVersion with the version 2. Would it be compatible?
There was a problem hiding this comment.
Hmm, have we considered updating the selector for getBlockType to return some default not found type, or do we depend on undefined?
Normally we should return the core/missing block type at this point in block list, but it is possible to unregister the block type. Presumably someone can also break it by updating setUnregisteredTypeHandlerName to a nonsense value.
unregister.missing.mp4
There was a problem hiding this comment.
Hmm, have we considered updating the selector for getBlockType to return some default not found type, or do we depend on undefined?
That would be a breaking change. In the current setup setUnregisteredTypeHandlerName is optional for the blocks registry. In practice, it might unconsciously become mandatory. The challenge is that @wordpress/blocks can't provide some fallback blocks like the default one, the grouping one, freeform content, and the unregistered type. Maybe that should be the responsibility of the @wordpress/block-editor package to define some of those handlers so unregistering a given handler would rollback to the built-in one, or it wouldn't take an effect. We could alternatively refactor code to take into account that those handlers might get unset or the corresponding block type can be removed. @youknowriad and @mtias should know better what was the initial design here and what would be the best way to make it bugs free.
There was a problem hiding this comment.
That makes sense to avoid a breaking change. If we want to make this a little nicer for callers, one alternative would be to return a default unknown type in the case that the unregistered type handler name is not configured (which supports no properties/has no attributes) in a new selector, say getBlockTypeOrUnknownBlock. In its implementation might simply be something like: return getBlockType( type ) ?? UNKNOWN_BLOCK_TYPE. It wouldn't actually be registered, since I think it's fair to add that level of customization.
I think the current design is fine too provided that all callers know that undefined may be returned from getBlockType. This is easy enough to search for in core usage. We're missing some other usages where we blindly pass blockType to another selector, and that selector does not check for undefined. There's also a few spots in code where we also assume that 'core/missing' is registered.
We can of course guard these in follow up PRs:
gutenberg/packages/block-editor/src/components/block-list/block.native.js
Lines 321 to 323 in 35e16bb
gutenberg/packages/block-editor/src/hooks/align.js
Lines 134 to 135 in 35e16bb
gutenberg/packages/blocks/src/api/factory.js
Lines 290 to 291 in 35e16bb
https://github.com/WordPress/gutenberg/blob/trunk/packages/blocks/src/api/serializer.js#L346-L347
gutenberg/packages/blocks/src/api/utils.js
Lines 53 to 56 in 35e16bb
gutenberg/packages/blocks/src/api/raw-handling/shortcode-converter.js
Lines 95 to 98 in 35e16bb
gutenberg/packages/blocks/src/store/selectors.js
Lines 128 to 129 in 35e16bb
There was a problem hiding this comment.
does
lightBlockWrapperhandling exist only for backward compatibility? I don't see it used with core blocks anymore. I see it referenced in multiple places in the block editor package. Could we setapiVersionto2in a hook during registration to simplify all those checks?
I opened PR #34459 that removes usage of lightBlockWrapper.
|
Size Change: +15 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
|
To manually test this, create a new post, and in the code editor add: Go back to code view, and we should see the missing block: Unregister the missing block type in console: Try doing some actions like going to code view or trying to click the missing block. Theoretically the editor should not WSOD, or throw a JS error |
There was a problem hiding this comment.
Thanks @gziolo. Going to approve this one since the guards here make sense 👍. There are other usages we should bulletproof for blockType (see https://github.com/WordPress/gutenberg/pull/34346/files#r699496884) though some of them will be harder to reach.
I think we should file an issue with a list and ask for help to address them eventually. One thing that would help is TypeScript validation, but I guess we are far from getting coverage for |
Description
Related to #32815.
This PR tries to minimize the chances to see the following error:
How has this been tested?
I'm not sure if it is possible to validate with manual testing.
Types of changes
Code quality.
Checklist:
*.native.jsfiles for terms that need renaming or removal).