Blocks: Add a data store to manage the block/categories registration #6679

youknowriad · 2018-05-10T09:43:54Z

This PR introduces a store to the blocks module, this allows things like:

using withSelect to retrieve blocks/categories...
reacting to block registration
pave the way to an addCategories action.

This has been on my mind for a long time now, but I thought it would be a good first step towards the refactoring needed to introduce the server-side block awareness (registerBlockType and implementBlockType). My idea is to update the retrieval of blockTypes etc... to rely on the selector and we could change the underlying implementation of the selector if we split block registration into "defining the existence of a block" and "implement the block in the editor".

At the moment, the external API remains the same, there's no deprecation or anything (I prefer to leave this for the server-side awareness work which would clarify the API needed). So basically, I just updated the existing APIs to call select/dispatch instead of relying on the local variables. So as a follow-up task, I'd like to update our current usage of the block module API and use withSelect instead

jorgefilipecosta · 2018-05-10T10:53:15Z

blocks/api/registration.js

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


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.

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.

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.

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.

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 🙂

jorgefilipecosta · 2018-05-10T11:10:39Z

blocks/store/actions.js

+export function addBlockTypes( blockTypes ) {
+	return {
+		type: 'ADD_BLOCK_TYPES',
+		blockTypes: castArray( blockTypes ),


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.

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.

jorgefilipecosta

This PR seems to work well I did not catch any problem. And I think this is a good step in the direction of using our data module is the unique source of data 👍
I would like to know if anyone has other thoughts on the use of redux to save function references, or if there is any downside we did not found.

aduth

Seems like a nice refactoring.

aduth · 2018-05-11T12:30:46Z

blocks/api/registration.js

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


Why do we call it the "fallback type" now, but the current API refers to it as the "unknown type" ?

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.

I feel like fallback is a better name for consistency with the defaultBlockType name.

so instead of unknownTypeHandlerName and defaultBlockTypeName
we would have fallbackBlockTypeName and defaultBlockTypeName.

I can update to add the "name" suffix to the actions as well, it's already used in the selectors.

But then should we want to update the block API to expose setFallbackBlockTypeName as well? Also, is it xBlockTypeName or xBlockName ? Latter is less verbose and should be equally meaningful.

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.

aduth · 2018-05-11T12:32:15Z

blocks/api/registration.js

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


Same note type vs. name.

aduth · 2018-05-11T12:35:21Z

blocks/store/reducer.js

+ *
+ * @return {Object} Updated state.
+ */
+export function blockTypes( state = { types: [] }, action ) {


Seems like storing this as a deep object needlessly complicates things. Why not blockTypes as the object of blockName: blockType, and separate reducers for defaultBlockName, fallbackBlockName ? Or if we do want it to be nested, create via combineReducers ?

So there are two questions here:

Why it's an array instead of an object: I wanted to keep the order of the registration similar to the order of getBlockTypes.

Why it's nested: I thought originally it was needed to be able to use part of the st To be able to reset the defaultBlockName and fallbackBlockName when we remove the block, but you're right it's not I'll update using combineReducers

aduth · 2018-05-11T12:35:31Z

blocks/store/selectors.js

+	return state.blockTypes.types;
+}
+
+export function getBlockType( state, name ) {


We should optimize the reducer state to enable easier lookup by name (i.e. shape as object with block name as key).

Not certain if it's a valid thing we want (keeping the order of the registered blocks) but that was the reasoning behind state as array.

If lookup of block type by name is a common operation (which I suspect it is), we'd want to optimize it in some form. Either memoization, or storing order as a separate state value.

{ "blockTypes": { "core/paragraph": { "attributes": {} } }, "blockOrder": [ "core/paragraph" ] }

Actually, it looks like we we're not keeping the same order previously since we were using an object variable to hold the block list, I'm just going to use an object for now and let's see later if we really want to keep the order.

… fallbackBlockName

aduth · 2018-05-11T18:03:59Z

blocks/store/reducer.js

+				return null;
+			}
+			return state;
+		case 'SET_FALLBACK_BLOCK_NAME':


My mind goes to abstracting a higher-order reducer to encapsulate the common logic 😄

export function createBlockNameSetterReducer( setActionType ) { return ( state = null, action ) => { switch ( action.type ) { case 'REMOVE_BLOCK_TYPES': if ( action.names.indexOf( state ) !== -1 ) { return null; } return state; case setActionType: return action.name || null; } return state; }; } export const defaultBlockName = createBlockNameSetterReducer( 'SET_DEFAULT_BLOCK_NAME' ); export const fallbackBlockName = createBlockNameSetterReducer( 'SET_FALLBACK_BLOCK_NAME' );

Maybe premature 😉

aduth · 2018-05-11T18:05:11Z

blocks/store/reducer.js

+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 );


It's not entirely obvious what's going on here. Is this something _.uniqBy would work well for?

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.

aduth · 2018-05-11T18:06:14Z

blocks/store/selectors.js

+ */
+export const getBlockTypes = createSelector(
+	( state ) => Object.values( state.blockTypes ),
+	( state ) => state.blockTypes


Minor: We've not updated yet, but rememo@3.0.0 requires the second argument function return an array:

https://github.com/aduth/rememo/blob/master/CHANGELOG.md

gziolo · 2018-05-14T07:40:17Z

blocks/store/reducer.js

@@ -44,48 +44,32 @@ export function blockTypes( state = {}, action ) {
 }

 /**
- * Reducer keeping track of the default block name.
+ * Higher-order Reducer creating a reducer keeping track of give block name.


It should be of the given block - I think.

youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label May 10, 2018

youknowriad self-assigned this May 10, 2018

youknowriad requested a review from a team May 10, 2018 09:43

jorgefilipecosta reviewed May 10, 2018

View reviewed changes

youknowriad changed the title ~~Blocks: Add a data store to manager the block/categories registration~~ Blocks: Add a data store to manage the block/categories registration May 10, 2018

youknowriad added 2 commits May 10, 2018 16:03

Blocks: Add a data store to manager the block/categories registration

d1981c2

Move the default/fallback blocktypes to the blocks store

e55bfb2

youknowriad force-pushed the try/blocks-store branch from a5b7cfe to e55bfb2 Compare May 10, 2018 15:03

jorgefilipecosta reviewed May 10, 2018

View reviewed changes

youknowriad mentioned this pull request May 11, 2018

From v2.1.0 wp.blocks.getBlockTypes() returns an empty Array #4848

Closed

2 tasks

aduth reviewed May 11, 2018

View reviewed changes

youknowriad added 3 commits May 11, 2018 16:33

Simplify the blockTypes reducer: Avoid nesting

4a6ef5a

Rename defaultBlockType and fallbackBlockType to defaultBlockName and…

f8cc10f

… fallbackBlockName

Map block types per name

e8ef768

aduth approved these changes May 11, 2018

View reviewed changes

youknowriad added 2 commits May 14, 2018 08:22

Factorize the block name reducers

ff24c10

Use dependants array syntax

2a522d4

gziolo reviewed May 14, 2018

View reviewed changes

Fix Typo

9606965

youknowriad merged commit 09e2da3 into master May 14, 2018

youknowriad deleted the try/blocks-store branch May 14, 2018 07:52

youknowriad mentioned this pull request May 16, 2018

Fix a race condition in withAPIData() #6303

Closed

mtias added this to the 2.9 milestone May 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blocks: Add a data store to manage the block/categories registration #6679

Blocks: Add a data store to manage the block/categories registration #6679

youknowriad commented May 10, 2018 •

edited

Loading

jorgefilipecosta May 10, 2018 •

edited

Loading

jorgefilipecosta May 10, 2018

youknowriad May 10, 2018

jorgefilipecosta May 10, 2018

noisysocks May 11, 2018

jorgefilipecosta May 10, 2018

youknowriad May 10, 2018

jorgefilipecosta left a comment

aduth left a comment

aduth May 11, 2018

aduth May 11, 2018 •

edited

Loading

youknowriad May 11, 2018 •

edited

Loading

aduth May 11, 2018 •

edited

Loading

youknowriad May 11, 2018

aduth May 11, 2018

aduth May 11, 2018

youknowriad May 11, 2018

aduth May 11, 2018

aduth May 11, 2018

youknowriad May 11, 2018

aduth May 11, 2018

youknowriad May 11, 2018

aduth May 11, 2018

aduth May 11, 2018

youknowriad May 12, 2018

aduth May 11, 2018

gziolo May 14, 2018

Blocks: Add a data store to manage the block/categories registration #6679

Blocks: Add a data store to manage the block/categories registration #6679

Conversation

youknowriad commented May 10, 2018 • edited Loading

jorgefilipecosta May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta left a comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth May 11, 2018 • edited Loading

Choose a reason for hiding this comment

youknowriad May 11, 2018 • edited Loading

Choose a reason for hiding this comment

aduth May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented May 10, 2018 •

edited

Loading

jorgefilipecosta May 10, 2018 •

edited

Loading

aduth May 11, 2018 •

edited

Loading

youknowriad May 11, 2018 •

edited

Loading

aduth May 11, 2018 •

edited

Loading