-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Blocks: Add a data store to manage the block/categories registration #6679
Changes from 2 commits
d1981c2
e55bfb2
4a6ef5a
f8cc10f
e8ef768
ff24c10
2a522d4
9606965
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,13 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { __ } from '@wordpress/i18n'; | ||
|
||
/** | ||
* Block categories are defined groups for organizing blocks. | ||
* | ||
* @var {Array} categories | ||
*/ | ||
const categories = [ | ||
{ slug: 'common', title: __( 'Common Blocks' ) }, | ||
{ slug: 'formatting', title: __( 'Formatting' ) }, | ||
{ slug: 'layout', title: __( 'Layout Elements' ) }, | ||
{ slug: 'widgets', title: __( 'Widgets' ) }, | ||
{ slug: 'embed', title: __( 'Embeds' ) }, | ||
{ slug: 'shared', title: __( 'Shared Blocks' ) }, | ||
]; | ||
import { select } from '@wordpress/data'; | ||
|
||
/** | ||
* Returns all the block categories. | ||
* | ||
* @return {Array} Block categories. | ||
*/ | ||
export function getCategories() { | ||
return categories; | ||
return select( 'core/blocks' ).getCategories(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,11 +9,7 @@ import { get, isFunction, some } from 'lodash'; | |
* WordPress dependencies | ||
*/ | ||
import { applyFilters } from '@wordpress/hooks'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { getCategories } from './categories'; | ||
import { select, dispatch } from '@wordpress/data'; | ||
|
||
/** | ||
* Defined behavior of a block type. | ||
|
@@ -40,29 +36,6 @@ import { getCategories } from './categories'; | |
* interacted with in an editor. | ||
*/ | ||
|
||
/** | ||
* Block type definitions keyed by block name. | ||
* | ||
* @type {Object.<string,WPBlockType>} | ||
*/ | ||
const blocks = {}; | ||
|
||
const categories = getCategories(); | ||
|
||
/** | ||
* Name of block handling unknown types. | ||
* | ||
* @type {?string} | ||
*/ | ||
let unknownTypeHandlerName; | ||
|
||
/** | ||
* Name of the default block. | ||
* | ||
* @type {?string} | ||
*/ | ||
let defaultBlockName; | ||
|
||
/** | ||
* Constant mapping post formats to the expected default block. | ||
* | ||
|
@@ -106,7 +79,7 @@ export function registerBlockType( name, settings ) { | |
); | ||
return; | ||
} | ||
if ( blocks[ name ] ) { | ||
if ( select( 'core/blocks' ).getBlockType( name ) ) { | ||
console.error( | ||
'Block "' + name + '" is already registered.' | ||
); | ||
|
@@ -139,7 +112,10 @@ export function registerBlockType( name, settings ) { | |
); | ||
return; | ||
} | ||
if ( 'category' in settings && ! some( categories, { slug: settings.category } ) ) { | ||
if ( | ||
'category' in settings && | ||
! some( select( 'core/blocks' ).getCategories(), { slug: settings.category } ) | ||
) { | ||
console.error( | ||
'The block "' + name + '" must have a registered category.' | ||
); | ||
|
@@ -161,7 +137,9 @@ export function registerBlockType( name, settings ) { | |
settings.icon = 'block-default'; | ||
} | ||
|
||
return blocks[ name ] = settings; | ||
dispatch( 'core/blocks' ).addBlockTypes( settings ); | ||
|
||
return settings; | ||
} | ||
|
||
/** | ||
|
@@ -173,14 +151,14 @@ export function registerBlockType( name, settings ) { | |
* unregistered; otherwise `undefined`. | ||
*/ | ||
export function unregisterBlockType( name ) { | ||
if ( ! blocks[ name ] ) { | ||
const oldBlock = select( 'core/blocks' ).getBlockType( name ); | ||
if ( ! oldBlock ) { | ||
console.error( | ||
'Block "' + name + '" is not registered.' | ||
); | ||
return; | ||
} | ||
const oldBlock = blocks[ name ]; | ||
delete blocks[ name ]; | ||
dispatch( 'core/blocks' ).removeBlockTypes( name ); | ||
return oldBlock; | ||
} | ||
|
||
|
@@ -190,7 +168,7 @@ export function unregisterBlockType( name ) { | |
* @param {string} name Block name. | ||
*/ | ||
export function setUnknownTypeHandlerName( name ) { | ||
unknownTypeHandlerName = name; | ||
dispatch( 'core/blocks' ).setFallbackBlockType( name ); | ||
} | ||
|
||
/** | ||
|
@@ -200,7 +178,7 @@ export function setUnknownTypeHandlerName( name ) { | |
* @return {?string} Blog name. | ||
*/ | ||
export function getUnknownTypeHandlerName() { | ||
return unknownTypeHandlerName; | ||
return select( 'core/blocks' ).getFallbackBlockTypeName(); | ||
} | ||
|
||
/** | ||
|
@@ -209,7 +187,7 @@ export function getUnknownTypeHandlerName() { | |
* @param {string} name Block name. | ||
*/ | ||
export function setDefaultBlockName( name ) { | ||
defaultBlockName = name; | ||
dispatch( 'core/blocks' ).setDefaultBlockType( name ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same note type vs. name. |
||
} | ||
|
||
/** | ||
|
@@ -218,7 +196,7 @@ export function setDefaultBlockName( name ) { | |
* @return {?string} Block name. | ||
*/ | ||
export function getDefaultBlockName() { | ||
return defaultBlockName; | ||
return select( 'core/blocks' ).getDefaultBlockTypeName(); | ||
} | ||
|
||
/** | ||
|
@@ -243,7 +221,7 @@ export function getDefaultBlockForPostFormat( postFormat ) { | |
* @return {?Object} Block type. | ||
*/ | ||
export function getBlockType( name ) { | ||
return blocks[ name ]; | ||
return select( 'core/blocks' ).getBlockType( name ); | ||
} | ||
|
||
/** | ||
|
@@ -252,7 +230,7 @@ export function getBlockType( name ) { | |
* @return {Array} Block settings. | ||
*/ | ||
export function getBlockTypes() { | ||
return Object.values( blocks ); | ||
return select( 'core/blocks' ).getBlockTypes(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getBlockTypes is used for example to get the transforms. So it seems we are saving function references in the state. Maybe we can just save the serializable block information in the state (title icon etc...), and have an external object that maps the block to the non-serializable information like functions in the transforms. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can make use of resolvers or something similar, so it looks like everything is in the store, and we can select and dispatch as we do now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I personally think it's ok to save non-serializable data in the state. The benefits outgrow the downsides. Having serializable information in state is only needed if you want to persist state or dispatch things over the network. I think the blocks store is not something we'd want to persist or dispatch over the network if we implement collaborative editing for instance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree saving the function references in state simplifies the things. Most sources discourage saving non-serializable info state, but objectively it looks like the only thing we lose is the ability to persist and rehydrate data https://github.com/markerikson/redux/blob/create-faq-page/docs/FAQ.md#can-i-put-functions-promises-or-other-non-serializable-items-in-my-store-state, so I'm ok with doing this tradeoff as I did not found other concrete disavantages. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm happy so long as it doesn't break the very useful time travelling feature in Redux DevTools. I just tested it then and it seems OK 🙂 |
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { castArray } from 'lodash'; | ||
|
||
/** | ||
* Returns an action object used in signalling that block types have been added. | ||
* | ||
* @param {Array|Object} blockTypes Block types received. | ||
* | ||
* @return {Object} Action object. | ||
*/ | ||
export function addBlockTypes( blockTypes ) { | ||
return { | ||
type: 'ADD_BLOCK_TYPES', | ||
blockTypes: castArray( blockTypes ), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the fact that this action receives multiple blocks. It opens the door to bulk registration, instead of dispatching one action per register block. But, right now we call registerBlockType per block so an action is dispatched per block and at the start, we are dispatching many actions. Right after landing this, I think we should change our registration mechanism to make use of the possibility of using ADD_BLOCK_TYPES with multiple blocks. If a general approach does not look feasible at least, we can change our registerCoreBlocks to register all core blocks with a single action. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that sounds like a good enhancement. I didn't want to change the Public API yet because the work that needs to be done on the server-side awareness of blocks will inevitably change the API, so it's better to deprecate things once. |
||
}; | ||
} | ||
|
||
/** | ||
* Returns an action object used to remove a registered block type. | ||
* | ||
* @param {string|Array} names Block name. | ||
* | ||
* @return {Object} Action object. | ||
*/ | ||
export function removeBlockTypes( names ) { | ||
return { | ||
type: 'REMOVE_BLOCK_TYPES', | ||
names: castArray( names ), | ||
}; | ||
} | ||
|
||
/** | ||
* Returns an action object used to set the default block type. | ||
* | ||
* @param {string} name Block name. | ||
* | ||
* @return {Object} Action object. | ||
*/ | ||
export function setDefaultBlockType( name ) { | ||
return { | ||
type: 'SET_DEFAULT_BLOCK_TYPE', | ||
name, | ||
}; | ||
} | ||
|
||
/** | ||
* Returns an action object used to set the fallback block type. | ||
* | ||
* @param {string} name Block name. | ||
* | ||
* @return {Object} Action object. | ||
*/ | ||
export function setFallbackBlockType( name ) { | ||
return { | ||
type: 'SET_FALLBACK_BLOCK_TYPE', | ||
name, | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { registerStore } from '@wordpress/data'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import reducer from './reducer'; | ||
import * as selectors from './selectors'; | ||
import * as actions from './actions'; | ||
|
||
registerStore( 'core/blocks', { reducer, selectors, actions } ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { filter, map } from 'lodash'; | ||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { combineReducers } from '@wordpress/data'; | ||
import { __ } from '@wordpress/i18n'; | ||
|
||
/** | ||
* Module Constants | ||
*/ | ||
export const DEFAULT_CATEGORIES = [ | ||
{ slug: 'common', title: __( 'Common Blocks' ) }, | ||
{ slug: 'formatting', title: __( 'Formatting' ) }, | ||
{ slug: 'layout', title: __( 'Layout Elements' ) }, | ||
{ slug: 'widgets', title: __( 'Widgets' ) }, | ||
{ slug: 'embed', title: __( 'Embeds' ) }, | ||
{ slug: 'shared', title: __( 'Shared Blocks' ) }, | ||
]; | ||
|
||
/** | ||
* Reducer managing the block types | ||
* | ||
* @param {Object} state Current state. | ||
* @param {Object} action Dispatched action. | ||
* | ||
* @return {Object} Updated state. | ||
*/ | ||
export function blockTypes( state = { types: [] }, action ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like storing this as a deep object needlessly complicates things. Why not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So there are two questions here:
|
||
switch ( action.type ) { | ||
case 'ADD_BLOCK_TYPES': | ||
const addedNames = map( action.blockTypes, ( blockType ) => blockType.name ); | ||
const previousState = filter( | ||
state.types, | ||
( blockType ) => addedNames.indexOf( blockType.name ) === -1 | ||
); | ||
return { | ||
...state, | ||
types: [ ...previousState, ...action.blockTypes ], | ||
}; | ||
case 'REMOVE_BLOCK_TYPES': | ||
return { | ||
...state, | ||
types: filter( state.types, ( blockType ) => action.names.indexOf( blockType.name ) === -1 ), | ||
defaultBlockType: action.names.indexOf( state.defaultBlockType ) !== -1 ? undefined : state.defaultBlockType, | ||
fallbackBlockType: action.names.indexOf( state.fallbackBlockType ) !== -1 ? undefined : state.fallbackBlockType, | ||
}; | ||
case 'SET_DEFAULT_BLOCK_TYPE': | ||
return { | ||
...state, | ||
defaultBlockType: action.name, | ||
}; | ||
case 'SET_FALLBACK_BLOCK_TYPE': | ||
return { | ||
...state, | ||
fallbackBlockType: action.name, | ||
}; | ||
} | ||
|
||
return state; | ||
} | ||
|
||
/** | ||
* Reducer managing the categories | ||
* | ||
* @param {Object} state Current state. | ||
* @param {Object} action Dispatched action. | ||
* | ||
* @return {Object} Updated state. | ||
*/ | ||
export function categories( state = DEFAULT_CATEGORIES, action ) { | ||
if ( action.type === 'ADD_CATEGORIES' ) { | ||
const addedSlugs = map( action.categories, ( category ) => category.slug ); | ||
const previousState = filter( state, ( category ) => addedSlugs.indexOf( category.slug ) === -1 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not entirely obvious what's going on here. Is this something There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. uniqBy won't keep the order, while it's not important for blocks, it's important for categories to keep the same order in the inserter. |
||
return [ ...previousState, ...action.categories ]; | ||
} | ||
|
||
return state; | ||
} | ||
|
||
export default combineReducers( { | ||
blockTypes, | ||
categories, | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we call it the "fallback type" now, but the current API refers to it as the "unknown type" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the choice of referring to it by "name" was intentional; type is the object, we're setting, storing, and retrieving it as a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like fallback is a better name for consistency with the defaultBlockType name.
so instead of
unknownTypeHandlerName
anddefaultBlockTypeName
we would have
fallbackBlockTypeName
anddefaultBlockTypeName
.I can update to add the "name" suffix to the actions as well, it's already used in the selectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then should we want to update the block API to expose
setFallbackBlockTypeName
as well? Also, is itxBlockTypeName
orxBlockName
? Latter is less verbose and should be equally meaningful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can use the latter. Though, I don't want to change the API for now, Let's avoid deprecations until we figure out server-side awareness.