From b18724063c0de98d15a43cca5d848d5c2e9df4fc Mon Sep 17 00:00:00 2001 From: Riad Benguella Date: Fri, 1 Jun 2018 14:49:10 +0100 Subject: [PATCH] Shared Blocks: Refactor fetching/saving/updating to avoid cycle dependency --- core-blocks/block/edit.js | 20 +++--- editor/store/actions.js | 14 ++--- editor/store/effects.js | 39 +++--------- editor/store/reducer.js | 24 +++----- editor/store/selectors.js | 28 ++++++++- editor/store/test/effects.js | 109 +++++++++------------------------ editor/store/test/reducer.js | 25 ++++---- editor/store/test/selectors.js | 10 ++- 8 files changed, 100 insertions(+), 169 deletions(-) diff --git a/core-blocks/block/edit.js b/core-blocks/block/edit.js index deb56593f1827f..285003e3599d57 100644 --- a/core-blocks/block/edit.js +++ b/core-blocks/block/edit.js @@ -11,6 +11,7 @@ import { Placeholder, Spinner, Disabled } from '@wordpress/components'; import { withSelect, withDispatch } from '@wordpress/data'; import { __ } from '@wordpress/i18n'; import { BlockEdit } from '@wordpress/editor'; +import { serialize } from '@wordpress/blocks'; /** * Internal dependencies @@ -72,14 +73,11 @@ class SharedBlockEdit extends Component { } save() { - const { sharedBlock, onUpdateTitle, updateAttributes, block, onSave } = this.props; + const { onChange, block, onSave } = this.props; const { title, changedAttributes } = this.state; - if ( title !== sharedBlock.title ) { - onUpdateTitle( title ); - } - - updateAttributes( block.uid, changedAttributes ); + const content = serialize( { ...block, attributes: { ...block.attributes, ...changedAttributes } } ); + onChange( { title, content } ); onSave(); this.stopEditing(); @@ -138,7 +136,7 @@ export default compose( [ getSharedBlock, isFetchingSharedBlock, isSavingSharedBlock, - getBlock, + getParsedSharedBlock, } = select( 'core/editor' ); const { ref } = ownProps.attributes; const sharedBlock = getSharedBlock( ref ); @@ -147,22 +145,20 @@ export default compose( [ sharedBlock, isFetching: isFetchingSharedBlock( ref ), isSaving: isSavingSharedBlock( ref ), - block: sharedBlock ? getBlock( sharedBlock.uid ) : null, + block: getParsedSharedBlock( ref ), }; } ), withDispatch( ( dispatch, ownProps ) => { const { fetchSharedBlocks, - updateBlockAttributes, - updateSharedBlockTitle, + updateSharedBlock, saveSharedBlock, } = dispatch( 'core/editor' ); const { ref } = ownProps.attributes; return { fetchSharedBlock: partial( fetchSharedBlocks, ref ), - updateAttributes: updateBlockAttributes, - onUpdateTitle: partial( updateSharedBlockTitle, ref ), + onChange: partial( updateSharedBlock, ref ), onSave: partial( saveSharedBlock, ref ), }; } ), diff --git a/editor/store/actions.js b/editor/store/actions.js index 24d86603d87b2a..d9d85e33a0ca79 100644 --- a/editor/store/actions.js +++ b/editor/store/actions.js @@ -581,8 +581,6 @@ export function fetchSharedBlocks( id ) { /** * Returns an action object used in signalling that shared blocks have been * received. `results` is an array of objects containing: - * - `sharedBlock` - Details about how the shared block is persisted. - * - `parsedBlock` - The original block. * * @param {Object[]} results Shared blocks received. * @@ -591,7 +589,7 @@ export function fetchSharedBlocks( id ) { export function receiveSharedBlocks( results ) { return { type: 'RECEIVE_SHARED_BLOCKS', - results, + results: castArray( results ), }; } @@ -628,16 +626,16 @@ export function deleteSharedBlock( id ) { * Returns an action object used in signalling that a shared block's title is * to be updated. * - * @param {number} id The ID of the shared block to update. - * @param {string} title The new title. + * @param {number} id The ID of the shared block to update. + * @param {Object} changes The updated shared block properties. * * @return {Object} Action object. */ -export function updateSharedBlockTitle( id, title ) { +export function updateSharedBlock( id, changes ) { return { - type: 'UPDATE_SHARED_BLOCK_TITLE', + type: 'UPDATE_SHARED_BLOCK', id, - title, + changes, }; } diff --git a/editor/store/effects.js b/editor/store/effects.js index b67c697b3ba271..f511adeb52b42e 100644 --- a/editor/store/effects.js +++ b/editor/store/effects.js @@ -2,7 +2,7 @@ * External dependencies */ import { BEGIN, COMMIT, REVERT } from 'redux-optimist'; -import { get, includes, last, map, castArray, uniqueId } from 'lodash'; +import { get, includes, last, uniqueId } from 'lodash'; /** * WordPress dependencies @@ -29,7 +29,6 @@ import { setupEditorState, resetAutosave, resetPost, - receiveBlocks, receiveSharedBlocks, replaceBlock, replaceBlocks, @@ -420,13 +419,7 @@ export default { result.then( ( sharedBlockOrBlocks ) => { - dispatch( receiveSharedBlocks( map( - castArray( sharedBlockOrBlocks ), - ( sharedBlock ) => ( { - sharedBlock, - parsedBlock: parse( sharedBlock.content )[ 0 ], - } ) - ) ) ); + dispatch( receiveSharedBlocks( sharedBlockOrBlocks ) ); dispatch( { type: 'FETCH_SHARED_BLOCKS_SUCCESS', @@ -445,9 +438,6 @@ export default { } ); }, - RECEIVE_SHARED_BLOCKS( action ) { - return receiveBlocks( map( action.results, 'parsedBlock' ) ); - }, SAVE_SHARED_BLOCK( action, store ) { // TODO: these are potentially undefined, this fix is in place // until there is a filter to not use shared blocks if undefined @@ -460,10 +450,7 @@ export default { const { dispatch } = store; const state = store.getState(); - const { uid, title, isTemporary } = getSharedBlock( state, id ); - const { name, attributes, innerBlocks } = getBlock( state, uid ); - const content = serialize( createBlock( name, attributes, innerBlocks ) ); - + const { isTemporary, title, content } = getSharedBlock( state, id ); const data = isTemporary ? { title, content } : { id, title, content }; const path = isTemporary ? `/wp/v2/${ basePath }` : `/wp/v2/${ basePath }/${ id }`; const method = isTemporary ? 'POST' : 'PUT'; @@ -519,10 +506,7 @@ export default { } ); // Remove the parsed block. - dispatch( removeBlocks( [ - ...associatedBlockUids, - sharedBlock.uid, - ] ) ); + dispatch( removeBlocks( associatedBlockUids ) ); wp.apiRequest( { path: `/wp/v2/${ basePath }/${ id }`, method: 'DELETE' } ).then( () => { @@ -552,8 +536,8 @@ export default { const state = store.getState(); const oldBlock = getBlock( state, action.uid ); const sharedBlock = getSharedBlock( state, oldBlock.attributes.ref ); - const referencedBlock = getBlock( state, sharedBlock.uid ); - const newBlock = createBlock( referencedBlock.name, referencedBlock.attributes ); + const parsedBlock = parse( sharedBlock.content )[ 0 ]; + const newBlock = createBlock( parsedBlock.name, parsedBlock.attributes ); store.dispatch( replaceBlock( oldBlock.uid, newBlock ) ); }, CONVERT_BLOCK_TO_SHARED( action, store ) { @@ -562,15 +546,11 @@ export default { const parsedBlock = getBlock( getState(), action.uid ); const sharedBlock = { id: uniqueId( 'shared' ), - uid: parsedBlock.uid, title: __( 'Untitled shared block' ), + content: serialize( parsedBlock ), }; - dispatch( receiveSharedBlocks( [ { - sharedBlock, - parsedBlock, - } ] ) ); - + dispatch( receiveSharedBlocks( sharedBlock ) ); dispatch( saveSharedBlock( sharedBlock.id ) ); dispatch( replaceBlock( @@ -580,9 +560,6 @@ export default { layout: parsedBlock.attributes.layout, } ) ) ); - - // Re-add the original block to the store, since replaceBlock() will have removed it - dispatch( receiveBlocks( [ parsedBlock ] ) ); }, CREATE_NOTICE( { notice: { content, spokenMessage } } ) { const message = spokenMessage || content; diff --git a/editor/store/reducer.js b/editor/store/reducer.js index 38c3cf6d352a3f..3f634898f0cb34 100644 --- a/editor/store/reducer.js +++ b/editor/store/reducer.js @@ -921,35 +921,26 @@ export const sharedBlocks = combineReducers( { switch ( action.type ) { case 'RECEIVE_SHARED_BLOCKS': { return reduce( action.results, ( nextState, result ) => { - const { id, title } = result.sharedBlock; - const { uid } = result.parsedBlock; - - const value = { uid, title }; - - if ( ! isEqual( nextState[ id ], value ) ) { + if ( ! isEqual( nextState[ result.id ], result ) ) { if ( nextState === state ) { nextState = { ...nextState }; } - nextState[ id ] = value; + nextState[ result.id ] = result; } return nextState; }, state ); } - case 'UPDATE_SHARED_BLOCK_TITLE': { - const { id, title } = action; - - if ( ! state[ id ] || state[ id ].title === title ) { - return state; - } + case 'UPDATE_SHARED_BLOCK': { + const { id, changes } = action; return { ...state, [ id ]: { ...state[ id ], - title, + ...changes, }, }; } @@ -965,7 +956,10 @@ export const sharedBlocks = combineReducers( { const value = state[ id ]; return { ...omit( state, id ), - [ updatedId ]: value, + [ updatedId ]: { + id: updatedId, + ...value, + }, }; } diff --git a/editor/store/selectors.js b/editor/store/selectors.js index c5793bdd7fe7b7..93e7a262e52f57 100644 --- a/editor/store/selectors.js +++ b/editor/store/selectors.js @@ -22,7 +22,7 @@ import createSelector from 'rememo'; /** * WordPress dependencies */ -import { serialize, getBlockType, getBlockTypes, hasBlockSupport, hasChildBlocks } from '@wordpress/blocks'; +import { parse, serialize, getBlockType, getBlockTypes, hasBlockSupport, hasChildBlocks } from '@wordpress/blocks'; import { __ } from '@wordpress/i18n'; import { moment } from '@wordpress/date'; import deprecated from '@wordpress/deprecated'; @@ -1487,7 +1487,7 @@ export const getInserterItems = createSelector( return false; } - const referencedBlock = getBlock( state, sharedBlock.uid ); + const referencedBlock = getParsedSharedBlock( state, sharedBlock.id ); if ( ! referencedBlock ) { return false; } @@ -1507,7 +1507,7 @@ export const getInserterItems = createSelector( const buildSharedBlockInserterItem = ( sharedBlock ) => { const id = `core/block/${ sharedBlock.id }`; - const referencedBlock = getBlock( state, sharedBlock.uid ); + const referencedBlock = getParsedSharedBlock( state, sharedBlock.id ); const referencedBlockType = getBlockType( referencedBlock.name ); const { time, count = 0 } = getInsertUsage( state, id ) || {}; @@ -1608,6 +1608,28 @@ export const getSharedBlock = createSelector( ], ); +/** + * Returns the parsed block saved as shared block with the given ID. + * + * @param {Object} state Global application state. + * @param {number|string} ref The shared block's ID. + * + * @return {Object} The parsed block. + */ +export const getParsedSharedBlock = createSelector( + ( state, ref ) => { + const sharedBlock = getSharedBlock( state, ref ); + if ( ! sharedBlock ) { + return null; + } + + return parse( sharedBlock.content )[ 0 ]; + }, + ( state, ref ) => [ + get( state.sharedBlocks.data, [ ref, 'content' ] ), + ], +); + /** * Returns whether or not the shared block with the given ID is being saved. * diff --git a/editor/store/test/effects.js b/editor/store/test/effects.js index 310916655a0b44..d6fed6d4ab4a74 100644 --- a/editor/store/test/effects.js +++ b/editor/store/test/effects.js @@ -11,6 +11,7 @@ import { unregisterBlockType, registerBlockType, createBlock, + serialize, } from '@wordpress/blocks'; /** @@ -556,15 +557,9 @@ describe( 'effects', () => { expect( dispatch ).toHaveBeenCalledWith( receiveSharedBlocks( [ { - sharedBlock: { - id: 123, - title: 'My cool block', - content: '', - }, - parsedBlock: expect.objectContaining( { - name: 'core/test-block', - attributes: { name: 'Big Bird' }, - } ), + id: 123, + title: 'My cool block', + content: '', }, ] ) ); @@ -592,19 +587,11 @@ describe( 'effects', () => { return promise.then( () => { expect( dispatch ).toHaveBeenCalledWith( - receiveSharedBlocks( [ - { - sharedBlock: { - id: 123, - title: 'My cool block', - content: '', - }, - parsedBlock: expect.objectContaining( { - name: 'core/test-block', - attributes: { name: 'Big Bird' }, - } ), - }, - ] ) + receiveSharedBlocks( { + id: 123, + title: 'My cool block', + content: '', + } ) ); expect( dispatch ).toHaveBeenCalledWith( { type: 'FETCH_SHARED_BLOCKS_SUCCESS', @@ -636,22 +623,6 @@ describe( 'effects', () => { } ); } ); - describe( '.RECEIVE_SHARED_BLOCKS', () => { - const handler = effects.RECEIVE_SHARED_BLOCKS; - - it( 'should receive parsed blocks', () => { - const action = receiveSharedBlocks( [ - { - parsedBlock: { uid: 'broccoli' }, - }, - ] ); - - expect( handler( action ) ).toEqual( receiveBlocks( [ - { uid: 'broccoli' }, - ] ) ); - } ); - } ); - describe( '.SAVE_SHARED_BLOCK', () => { const handler = effects.SAVE_SHARED_BLOCK; @@ -665,13 +636,9 @@ describe( 'effects', () => { return promise; } ); - const sharedBlock = { id: 123, title: 'My cool block' }; - const parsedBlock = createBlock( 'core/test-block', { name: 'Big Bird' } ); + const sharedBlock = { id: 123, title: 'My cool block', content: '' }; - const state = reduce( [ - receiveSharedBlocks( [ { sharedBlock, parsedBlock } ] ), - receiveBlocks( [ parsedBlock ] ), - ], reducer, undefined ); + const state = reduce( [ receiveSharedBlocks( sharedBlock ) ], reducer, undefined ); const dispatch = jest.fn(); const store = { getState: () => state, dispatch }; @@ -699,13 +666,8 @@ describe( 'effects', () => { set( global, [ 'wp', 'api', 'getPostTypeRoute' ], () => 'blocks' ); set( global, [ 'wp', 'apiRequest' ], () => promise ); - const sharedBlock = { id: 123, title: 'My cool block' }; - const parsedBlock = createBlock( 'core/test-block', { name: 'Big Bird' } ); - - const state = reduce( [ - receiveSharedBlocks( [ { sharedBlock, parsedBlock } ] ), - receiveBlocks( [ parsedBlock ] ), - ], reducer, undefined ); + const sharedBlock = { id: 123, title: 'My cool block', content: '' }; + const state = reduce( [ receiveSharedBlocks( sharedBlock ) ], reducer, undefined ); const dispatch = jest.fn(); const store = { getState: () => state, dispatch }; @@ -731,13 +693,11 @@ describe( 'effects', () => { set( global, [ 'wp', 'apiRequest' ], () => promise ); const associatedBlock = createBlock( 'core/block', { ref: 123 } ); - const sharedBlock = { id: 123, title: 'My cool block' }; - const parsedBlock = createBlock( 'core/test-block', { name: 'Big Bird' } ); + const sharedBlock = { id: 123, title: 'My cool block', content: '' }; const state = reduce( [ + receiveSharedBlocks( [ sharedBlock ] ), resetBlocks( [ associatedBlock ] ), - receiveSharedBlocks( [ { sharedBlock, parsedBlock } ] ), - receiveBlocks( [ parsedBlock ] ), ], reducer, undefined ); const dispatch = jest.fn(); @@ -752,7 +712,7 @@ describe( 'effects', () => { } ); expect( dispatch ).toHaveBeenCalledWith( - removeBlocks( [ associatedBlock.uid, parsedBlock.uid ] ) + removeBlocks( [ associatedBlock.uid ] ) ); return promise.then( () => { @@ -770,13 +730,9 @@ describe( 'effects', () => { set( global, [ 'wp', 'api', 'getPostTypeRoute' ], () => 'blocks' ); set( global, [ 'wp', 'apiRequest' ], () => promise ); - const sharedBlock = { id: 123, title: 'My cool block' }; - const parsedBlock = createBlock( 'core/test-block', { name: 'Big Bird' } ); + const sharedBlock = { id: 123, title: 'My cool block', content: '' }; - const state = reduce( [ - receiveSharedBlocks( [ { sharedBlock, parsedBlock } ] ), - receiveBlocks( [ parsedBlock ] ), - ], reducer, undefined ); + const state = reduce( [ receiveSharedBlocks( [ sharedBlock ] ) ], reducer, undefined ); const dispatch = jest.fn(); const store = { getState: () => state, dispatch }; @@ -814,24 +770,22 @@ describe( 'effects', () => { const handler = effects.CONVERT_BLOCK_TO_STATIC; it( 'should convert a shared block into a static block', () => { - const associatedBlock = createBlock( 'core/block', { ref: 123 } ); - const sharedBlock = { id: 123, title: 'My cool block' }; - const parsedBlock = createBlock( 'core/test-block', { name: 'Big Bird' } ); + const sharedBlock = { id: 123, title: 'My cool block', content: '' }; + const parsedBlock = createBlock( 'core/block', { ref: 123 } ); const state = reduce( [ - resetBlocks( [ associatedBlock ] ), - receiveSharedBlocks( [ { sharedBlock, parsedBlock } ] ), - receiveBlocks( [ parsedBlock ] ), + resetBlocks( [ parsedBlock ] ), + receiveSharedBlocks( [ sharedBlock ] ), ], reducer, undefined ); const dispatch = jest.fn(); const store = { getState: () => state, dispatch }; - handler( convertBlockToStatic( associatedBlock.uid ), store ); + handler( convertBlockToStatic( parsedBlock.uid ), store ); expect( dispatch ).toHaveBeenCalledWith( { type: 'REPLACE_BLOCKS', - uids: [ associatedBlock.uid ], + uids: [ parsedBlock.uid ], blocks: [ expect.objectContaining( { name: 'core/test-block', @@ -847,7 +801,7 @@ describe( 'effects', () => { const handler = effects.CONVERT_BLOCK_TO_SHARED; it( 'should convert a static block into a shared block', () => { - const staticBlock = createBlock( 'core/block', { ref: 123 } ); + const staticBlock = createBlock( 'core/test-block', { name: 'Big Bird' } ); const state = reducer( undefined, resetBlocks( [ staticBlock ] ) ); const dispatch = jest.fn(); @@ -857,12 +811,9 @@ describe( 'effects', () => { expect( dispatch ).toHaveBeenCalledWith( receiveSharedBlocks( [ { - sharedBlock: { - id: expect.stringMatching( /^shared/ ), - uid: staticBlock.uid, - title: 'Untitled shared block', - }, - parsedBlock: staticBlock, + id: expect.stringMatching( /^shared/ ), + title: 'Untitled shared block', + content: serialize( staticBlock ), } ] ) ); @@ -881,10 +832,6 @@ describe( 'effects', () => { ], time: expect.any( Number ), } ); - - expect( dispatch ).toHaveBeenCalledWith( - receiveBlocks( [ staticBlock ] ), - ); } ); } ); } ); diff --git a/editor/store/test/reducer.js b/editor/store/test/reducer.js index 32aa4e875bfca4..bce0944429f4da 100644 --- a/editor/store/test/reducer.js +++ b/editor/store/test/reducer.js @@ -1924,20 +1924,17 @@ describe( 'state', () => { it( 'should add received shared blocks', () => { const state = sharedBlocks( {}, { type: 'RECEIVE_SHARED_BLOCKS', - results: [ { - sharedBlock: { + results: [ + { id: 123, title: 'My cool block', }, - parsedBlock: { - uid: 'foo', - }, - } ], + ], } ); expect( state ).toEqual( { data: { - 123: { uid: 'foo', title: 'My cool block' }, + 123: { id: 123, title: 'My cool block' }, }, isFetching: {}, isSaving: {}, @@ -1947,21 +1944,23 @@ describe( 'state', () => { it( 'should update a shared block', () => { const initialState = { data: { - 123: { uid: '', title: '' }, + 123: { title: '' }, }, isFetching: {}, isSaving: {}, }; const state = sharedBlocks( initialState, { - type: 'UPDATE_SHARED_BLOCK_TITLE', + type: 'UPDATE_SHARED_BLOCK', id: 123, - title: 'My block', + changes: { + title: 'My block', + }, } ); expect( state ).toEqual( { data: { - 123: { uid: '', title: 'My block' }, + 123: { title: 'My block' }, }, isFetching: {}, isSaving: {}, @@ -1971,7 +1970,7 @@ describe( 'state', () => { it( 'should update the shared block\'s id if it was temporary', () => { const initialState = { data: { - shared1: { uid: '', title: '' }, + shared1: { title: '' }, }, isSaving: {}, }; @@ -1984,7 +1983,7 @@ describe( 'state', () => { expect( state ).toEqual( { data: { - 123: { uid: '', title: '' }, + 123: { id: 123, title: '' }, }, isFetching: {}, isSaving: {}, diff --git a/editor/store/test/selectors.js b/editor/store/test/selectors.js index 831ac8d72d451b..9f96eaf52b5542 100644 --- a/editor/store/test/selectors.js +++ b/editor/store/test/selectors.js @@ -2969,16 +2969,14 @@ describe( 'selectors', () => { const state = { editor: { present: { - blocksByUID: { - block1: { name: 'core/test-block-a' }, - }, + blocksByUID: {}, blockOrder: {}, edits: {}, }, }, sharedBlocks: { data: { - 1: { uid: 'block1', title: 'Shared Block 1' }, + 1: { id: 1, content: '', title: 'Shared Block 1' }, }, }, currentPost: {}, @@ -3032,8 +3030,8 @@ describe( 'selectors', () => { }, sharedBlocks: { data: { - 1: { uid: 'block1', title: 'Shared Block 1' }, - 2: { uid: 'block1', title: 'Shared Block 2' }, + 1: { id: 1, content: '', title: 'Shared Block 1' }, + 2: { id: 2, content: '', title: 'Shared Block 2' }, }, }, currentPost: {},