Blocks: Use selectors to fetch details about for patterns and styles …

[gziolo]

…from block types

Description

Based on the comment from @aduth in #18270 (comment):

Since we're inheriting the logic, I'm not overly concerned with addressing it here, but a point for both this and the blockStyles upon which it's based:

• Do we really need to track a value for each registered block type? Can't this be something where we just manage the normalization to an empty array from the selector? Seems like it may make the implementation a little simpler, and avoid the (albeit minimal) memory usage involved with tracking a bunch of key/values for blocks without patterns.

This should also fix the copy and paste issues raised by @mcsf in #18270 (comment):

I think this should read ( pattern ) => pattern.name :)

How has this been tested?

npm run test-unit

I also tested manually in the browser by executing the following code on JS console:

wp.blocks.registerBlockStyle( 'core/quote', { name: 'test', label: 'Test' } );

and checked that this new style got properly registered for the Quote block:

Types of changes

Refactoring.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR. .

[aduth]

To consider:

• If the selectors are expected to return an array, the implementation will need to be revised to account for missing keys in the blockStyles state. Optionally, if it's necessary to distinguish between registered/non-registered, this will need to be considered (i.e. returns undefined for non-registered block, [] for registered block with no styles). It should probably be aligned to whatever the current behavior is, but if there's room for improvement here, elsewhere we've generally tried to use null as a value in these sorts of tri-state results ("unknown" null vs. "known, and empty" [])
• Moving this to selectors means we have to be more wary about returning a consistent value on subsequent calls. Elsewhere, we use a common reference to an empty array, or we could memoize the function.

[gziolo]

• If the selectors are expected to return an array, the implementation will need to be revised to account for missing keys in the blockStyles state. Optionally, if it's necessary to distinguish between registered/non-registered, this will need to be considered (i.e. returns undefined for non-registered block, [] for registered block with no styles). It should probably be aligned to whatever the current behavior is, but if there's room for improvement here, elsewhere we've generally tried to use null as a value in these sorts of tri-state results ("unknown" null vs. "known, and empty" [])

I opted for:

• undefined when the block is not registered (to make it backward compatible)
• an empty array when no styles/patterns found

I can update to null if you think it is what we want to use from now on.

• Moving this to selectors means we have to be more wary about returning a consistent value on subsequent calls. Elsewhere, we use a common reference to an empty array, or we could memoize the function.

I opted for using createSelector as we need to merge two arrays from now on.

[aduth]

• an empty array when no styles/patterns found

I can update to null if you think it is what we want to use from now on.

To clarify, I only think null is appropriate as a value when a fetch is still pending, so it can't be known whether there is or isn't any results.

If we know one way or the other, we should return that known value. For what you describe, it seems like an empty array would be appropriate.

[aduth]

• Moving this to selectors means we have to be more wary about returning a consistent value on subsequent calls. Elsewhere, we use a common reference to an empty array, or we could memoize the function.

I opted for using createSelector as we need to merge two arrays from now on.

Hmm, I wonder if this is really worthwhile then. Any time we can simply point to a reducer value, it's always going to be more optimal than trying to compute a derived result, even if that result is memoized.

I dunno if there might be a middle-ground option here where we still have a ADD_BLOCK_TYPES case in these reducers, but we only store values if patterns exist. Then the selector can infer based on whether a block type is registered and it has a key in blockStyles state, what value to return.

Roughly:

function getBlockStyles( state, blockName ) {
    if ( state.blockTypes[ blockName ] ) {
        return state.blockStyles[ blockName ] || EMPTY_ARRAY;
    }
}

Otherwise, maybe we should keep it as is it is.

[gziolo]

I dunno if there might be a middle-ground option here where we still have a ADD_BLOCK_TYPES case in these reducers, but we only store values if patterns exist. Then the selector can infer based on whether a block type is registered and it has a key in blockStyles state, what value to return.

The challenge with ADD_BLOCK_TYPES is that it has also a corresponding REMOVE_BLOCK_TYPES which is not handled in the original implementation and probably would be very difficult to handle as is. It doesn't look like we are close to finding a clearly better approach to handle this reducer. I'm inclined to keep it as is.

[youknowriad]

I know people used to disable styles by removing them from this reducer, this won't be possible anymore with this change.

[gziolo]

Interesting. Thanks for sharing.

[aduth]

I know people used to disable styles by removing them from this reducer, this won't be possible anymore with this change.

Can you elaborate on what you mean with this? Any links you can share with more information?

[gziolo]

I'm closing this PR based on the feedback received. Thank you @youknowriad and @aduth for help.