Skip to content
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

Merged
merged 8 commits into from
May 14, 2018
18 changes: 2 additions & 16 deletions blocks/api/categories.js
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();
}
58 changes: 18 additions & 40 deletions blocks/api/registration.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
*
Expand Down Expand Up @@ -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.'
);
Expand Down Expand Up @@ -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.'
);
Expand All @@ -161,7 +137,9 @@ export function registerBlockType( name, settings ) {
settings.icon = 'block-default';
}

return blocks[ name ] = settings;
dispatch( 'core/blocks' ).addBlockTypes( settings );

return settings;
}

/**
Expand All @@ -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;
}

Expand All @@ -190,7 +168,7 @@ export function unregisterBlockType( name ) {
* @param {string} name Block name.
*/
export function setUnknownTypeHandlerName( name ) {
unknownTypeHandlerName = name;
dispatch( 'core/blocks' ).setFallbackBlockName( name );
}

/**
Expand All @@ -200,7 +178,7 @@ export function setUnknownTypeHandlerName( name ) {
* @return {?string} Blog name.
*/
export function getUnknownTypeHandlerName() {
return unknownTypeHandlerName;
return select( 'core/blocks' ).getFallbackBlockName();
}

/**
Expand All @@ -209,7 +187,7 @@ export function getUnknownTypeHandlerName() {
* @param {string} name Block name.
*/
export function setDefaultBlockName( name ) {
defaultBlockName = name;
dispatch( 'core/blocks' ).setDefaultBlockName( name );
}

/**
Expand All @@ -218,7 +196,7 @@ export function setDefaultBlockName( name ) {
* @return {?string} Block name.
*/
export function getDefaultBlockName() {
return defaultBlockName;
return select( 'core/blocks' ).getDefaultBlockName();
}

/**
Expand All @@ -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 );
}

/**
Expand All @@ -252,7 +230,7 @@ export function getBlockType( name ) {
* @return {Array} Block settings.
*/
export function getBlockTypes() {
return Object.values( blocks );
return select( 'core/blocks' ).getBlockTypes();
Copy link
Member

@jorgefilipecosta jorgefilipecosta May 10, 2018

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 🙂

}

/**
Expand Down
7 changes: 5 additions & 2 deletions blocks/api/test/registration.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ describe( 'blocks', () => {
beforeAll( () => {
// Load all hooks that modify blocks
require( 'editor/hooks' );

// Initialize the block store
require( '../../store' );
} );

afterEach( () => {
Expand Down Expand Up @@ -288,7 +291,7 @@ describe( 'blocks', () => {

describe( 'getUnknownTypeHandlerName()', () => {
it( 'defaults to undefined', () => {
expect( getUnknownTypeHandlerName() ).toBeUndefined();
expect( getUnknownTypeHandlerName() ).toBeNull();
} );
} );

Expand All @@ -302,7 +305,7 @@ describe( 'blocks', () => {

describe( 'getDefaultBlockName()', () => {
it( 'defaults to undefined', () => {
expect( getDefaultBlockName() ).toBeUndefined();
expect( getDefaultBlockName() ).toBeNull();
} );
} );

Expand Down
5 changes: 5 additions & 0 deletions blocks/api/test/templates.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ import { getBlockTypes, unregisterBlockType, registerBlockType } from '../regist
import { doBlocksMatchTemplate, synchronizeBlocksWithTemplate } from '../templates';

describe( 'templates', () => {
beforeAll( () => {
// Initialize the block store
require( '../../store' );
} );

afterEach( () => {
getBlockTypes().forEach( ( block ) => {
unregisterBlockType( block.name );
Expand Down
5 changes: 5 additions & 0 deletions blocks/api/test/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ import { getBlockTypes, unregisterBlockType, registerBlockType, setDefaultBlockN
import { isUnmodifiedDefaultBlock } from '../utils';

describe( 'block helpers', () => {
beforeAll( () => {
// Initialize the block store
require( '../../store' );
} );

afterEach( () => {
setDefaultBlockName( undefined );
getBlockTypes().forEach( ( block ) => {
Expand Down
4 changes: 4 additions & 0 deletions blocks/api/test/validation.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ describe( 'validation', () => {
category: 'common',
title: 'block title',
};
beforeAll( () => {
// Initialize the block store
require( '../../store' );
} );

afterEach( () => {
setUnknownTypeHandlerName( undefined );
Expand Down
1 change: 1 addition & 0 deletions blocks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@
//
// Blocks are inferred from the HTML source of a post through a parsing mechanism
// and then stored as objects in state, from which it is then rendered for editing.
import './store';
export * from './api';
60 changes: 60 additions & 0 deletions blocks/store/actions.js
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 ),
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 name.
*
* @param {string} name Block name.
*
* @return {Object} Action object.
*/
export function setDefaultBlockName( name ) {
return {
type: 'SET_DEFAULT_BLOCK_NAME',
name,
};
}

/**
* Returns an action object used to set the fallback block name.
*
* @param {string} name Block name.
*
* @return {Object} Action object.
*/
export function setFallbackBlockName( name ) {
return {
type: 'SET_FALLBACK_BLOCK_NAME',
name,
};
}
13 changes: 13 additions & 0 deletions blocks/store/index.js
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 } );
Loading