From 4f979461eb263838a93d5c2aadc2871e8d35beb8 Mon Sep 17 00:00:00 2001 From: Riad Benguella <benguella@gmail.com> Date: Wed, 30 Nov 2022 12:17:08 +0100 Subject: [PATCH] Update the attributes reducer to use a map instead of a regular object (#46146) --- packages/block-editor/src/store/reducer.js | 192 +++++++++--------- packages/block-editor/src/store/selectors.js | 8 +- .../src/store/test/performance.js | 32 +++ .../block-editor/src/store/test/reducer.js | 181 +++++++++++------ 4 files changed, 247 insertions(+), 166 deletions(-) create mode 100644 packages/block-editor/src/store/test/performance.js diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js index 924b563f5710e2..cbbf833ff7b2a6 100644 --- a/packages/block-editor/src/store/reducer.js +++ b/packages/block-editor/src/store/reducer.js @@ -111,23 +111,6 @@ function getFlattenedBlockAttributes( blocks ) { return flattenBlocks( blocks, ( block ) => block.attributes ); } -/** - * Returns an object against which it is safe to perform mutating operations, - * given the original object and its current working copy. - * - * @param {Object} original Original object. - * @param {Object} working Working object. - * - * @return {Object} Mutation-safe object. - */ -function getMutateSafeObject( original, working ) { - if ( original === working ) { - return { ...original }; - } - - return working; -} - /** * Returns true if the two object arguments have the same keys, or false * otherwise. @@ -177,7 +160,7 @@ function buildBlockTree( state, blocks ) { for ( const block of flattenedBlocks ) { result[ block.clientId ] = Object.assign( result[ block.clientId ], { ...state.byClientId[ block.clientId ], - attributes: state.attributes[ block.clientId ], + attributes: state.attributes.get( block.clientId ), innerBlocks: block.innerBlocks.map( ( subBlock ) => result[ subBlock.clientId ] ), @@ -281,7 +264,9 @@ const withBlockTree = [ action.clientId ]: { ...newState.tree[ action.clientId ], ...newState.byClientId[ action.clientId ], - attributes: newState.attributes[ action.clientId ], + attributes: newState.attributes.get( + action.clientId + ), }, }, [ action.clientId ], @@ -293,7 +278,7 @@ const withBlockTree = ( result, clientId ) => { result[ clientId ] = { ...newState.tree[ clientId ], - attributes: newState.attributes[ clientId ], + attributes: newState.attributes.get( clientId ), }; return result; }, @@ -417,15 +402,15 @@ const withBlockTree = break; } case 'SAVE_REUSABLE_BLOCK_SUCCESS': { - const updatedBlockUids = Object.entries( newState.attributes ) - .filter( ( [ clientId, attributes ] ) => { - return ( - newState.byClientId[ clientId ].name === - 'core/block' && - attributes.ref === action.updatedId - ); - } ) - .map( ( [ clientId ] ) => clientId ); + const updatedBlockUids = []; + newState.attributes.forEach( ( attributes, clientId ) => { + if ( + newState.byClientId[ clientId ].name === 'core/block' && + attributes.ref === action.updatedId + ) { + updatedBlockUids.push( clientId ); + } + } ); newState.tree = updateParentInnerBlocksInTree( newState, @@ -434,7 +419,7 @@ const withBlockTree = ...updatedBlockUids.reduce( ( result, clientId ) => { result[ clientId ] = { ...newState.byClientId[ clientId ], - attributes: newState.attributes[ clientId ], + attributes: newState.attributes.get( clientId ), innerBlocks: newState.tree[ clientId ].innerBlocks, }; @@ -602,7 +587,9 @@ const withBlockReset = ( reducer ) => ( state, action ) => { const newState = { ...state, byClientId: getFlattenedBlocksWithoutAttributes( action.blocks ), - attributes: getFlattenedBlockAttributes( action.blocks ), + attributes: new Map( + Object.entries( getFlattenedBlockAttributes( action.blocks ) ) + ), order: mapBlockOrder( action.blocks ), parents: mapBlockParents( action.blocks ), controlledInnerBlocks: {}, @@ -724,21 +711,16 @@ const withSaveReusableBlock = ( reducer ) => ( state, action ) => { } state = { ...state }; - - state.attributes = mapValues( - state.attributes, - ( attributes, clientId ) => { - const { name } = state.byClientId[ clientId ]; - if ( name === 'core/block' && attributes.ref === id ) { - return { - ...attributes, - ref: updatedId, - }; - } - - return attributes; + state.attributes = new Map( state.attributes ); + state.attributes.forEach( ( attributes, clientId ) => { + const { name } = state.byClientId[ clientId ]; + if ( name === 'core/block' && attributes.ref === id ) { + state.attributes.set( clientId, { + ...attributes, + ref: updatedId, + } ); } - ); + } ); } return reducer( state, action ); @@ -830,84 +812,98 @@ export const blocks = pipe( return state; }, - attributes( state = {}, action ) { + attributes( state = new Map(), action ) { switch ( action.type ) { case 'RECEIVE_BLOCKS': - case 'INSERT_BLOCKS': - return { - ...state, - ...getFlattenedBlockAttributes( action.blocks ), - }; + case 'INSERT_BLOCKS': { + const newState = new Map( state ); + Object.entries( + getFlattenedBlockAttributes( action.blocks ) + ).forEach( ( [ key, value ] ) => { + newState.set( key, value ); + } ); + return newState; + } - case 'UPDATE_BLOCK': + case 'UPDATE_BLOCK': { // Ignore updates if block isn't known or there are no attribute changes. if ( - ! state[ action.clientId ] || + ! state.get( action.clientId ) || ! action.updates.attributes ) { return state; } - return { - ...state, - [ action.clientId ]: { - ...state[ action.clientId ], - ...action.updates.attributes, - }, - }; + const newState = new Map( state ); + newState.set( action.clientId, { + ...state.get( action.clientId ), + ...action.updates.attributes, + } ); + return newState; + } case 'UPDATE_BLOCK_ATTRIBUTES': { // Avoid a state change if none of the block IDs are known. - if ( action.clientIds.every( ( id ) => ! state[ id ] ) ) { + if ( action.clientIds.every( ( id ) => ! state.get( id ) ) ) { return state; } - const next = action.clientIds.reduce( - ( accumulator, id ) => ( { - ...accumulator, - [ id ]: Object.entries( - action.uniqueByBlock - ? action.attributes[ id ] - : action.attributes ?? {} - ).reduce( ( result, [ key, value ] ) => { - // Consider as updates only changed values. - if ( value !== result[ key ] ) { - result = getMutateSafeObject( - state[ id ], - result - ); - result[ key ] = value; - } - - return result; - }, state[ id ] ), - } ), - {} - ); - - if ( - action.clientIds.every( - ( id ) => next[ id ] === state[ id ] - ) - ) { - return state; + let hasChange = false; + const newState = new Map( state ); + for ( const clientId of action.clientIds ) { + const updatedAttributeEntries = Object.entries( + action.uniqueByBlock + ? action.attributes[ clientId ] + : action.attributes ?? {} + ); + if ( updatedAttributeEntries.length === 0 ) { + continue; + } + let hasUpdatedAttributes = false; + const existingAttributes = state.get( clientId ); + const newAttributes = {}; + updatedAttributeEntries.forEach( ( [ key, value ] ) => { + if ( existingAttributes[ key ] !== value ) { + hasUpdatedAttributes = true; + newAttributes[ key ] = value; + } + } ); + hasChange = hasChange || hasUpdatedAttributes; + if ( hasUpdatedAttributes ) { + newState.set( clientId, { + ...existingAttributes, + ...newAttributes, + } ); + } } - return { ...state, ...next }; + return hasChange ? newState : state; } - case 'REPLACE_BLOCKS_AUGMENTED_WITH_CHILDREN': + case 'REPLACE_BLOCKS_AUGMENTED_WITH_CHILDREN': { if ( ! action.blocks ) { return state; } - return { - ...omit( state, action.replacedClientIds ), - ...getFlattenedBlockAttributes( action.blocks ), - }; + const newState = new Map( state ); + action.replacedClientIds.forEach( ( clientId ) => { + newState.delete( clientId ); + } ); + Object.entries( + getFlattenedBlockAttributes( action.blocks ) + ).forEach( ( [ key, value ] ) => { + newState.set( key, value ); + } ); + return newState; + } - case 'REMOVE_BLOCKS_AUGMENTED_WITH_CHILDREN': - return omit( state, action.removedClientIds ); + case 'REMOVE_BLOCKS_AUGMENTED_WITH_CHILDREN': { + const newState = new Map( state ); + action.removedClientIds.forEach( ( clientId ) => { + newState.delete( clientId ); + } ); + return newState; + } } return state; diff --git a/packages/block-editor/src/store/selectors.js b/packages/block-editor/src/store/selectors.js index 7ddb6603f2ed50..396748b6475b8e 100644 --- a/packages/block-editor/src/store/selectors.js +++ b/packages/block-editor/src/store/selectors.js @@ -69,8 +69,8 @@ export function getBlockName( state, clientId ) { const socialLinkName = 'core/social-link'; if ( Platform.OS !== 'web' && block?.name === socialLinkName ) { - const attributes = state.blocks.attributes[ clientId ]; - const { service } = attributes; + const attributes = state.blocks.attributes.get( clientId ); + const { service } = attributes ?? {}; return service ? `${ socialLinkName }-${ service }` : socialLinkName; } @@ -105,7 +105,7 @@ export function getBlockAttributes( state, clientId ) { return null; } - return state.blocks.attributes[ clientId ]; + return state.blocks.attributes.get( clientId ); } /** @@ -152,7 +152,7 @@ export const __unstableGetBlockWithoutInnerBlocks = createSelector( }, ( state, clientId ) => [ state.blocks.byClientId[ clientId ], - state.blocks.attributes[ clientId ], + state.blocks.attributes.get( clientId ), ] ); diff --git a/packages/block-editor/src/store/test/performance.js b/packages/block-editor/src/store/test/performance.js new file mode 100644 index 00000000000000..2ab191ff7408c9 --- /dev/null +++ b/packages/block-editor/src/store/test/performance.js @@ -0,0 +1,32 @@ +/** + * Internal dependencies + */ +import reducer from '../reducer'; + +describe( 'performance', () => { + const state = reducer( undefined, { type: '@@init' } ); + const blocks = []; + for ( let i = 0; i < 100000; i++ ) { + blocks.push( { + clientId: `block-${ i }`, + attributes: { content: `paragraph ${ i }` }, + innerBlocks: [], + } ); + } + const preparedState = reducer( state, { + type: 'RESET_BLOCKS', + blocks, + } ); + + it( 'should update blocks', () => { + const updatedState = reducer( preparedState, { + type: 'UPDATE_BLOCK_ATTRIBUTES', + clientIds: [ 'block-10' ], + attributes: { + content: 'updated paragraph 10', + }, + } ); + + expect( updatedState ).toBeDefined(); + } ); +} ); diff --git a/packages/block-editor/src/store/test/reducer.js b/packages/block-editor/src/store/test/reducer.js index 9f2e623c70111a..0aa98565efd75f 100644 --- a/packages/block-editor/src/store/test/reducer.js +++ b/packages/block-editor/src/store/test/reducer.js @@ -222,12 +222,14 @@ describe( 'state', () => { isValid: true, }, }, - attributes: { - chicken: {}, - 'chicken-child': { - attr: true, - }, - }, + attributes: new Map( + Object.entries( { + chicken: {}, + 'chicken-child': { + attr: true, + }, + } ) + ), order: { '': [ 'chicken' ], chicken: [ 'chicken-child' ], @@ -276,13 +278,15 @@ describe( 'state', () => { isValid: true, }, }, - attributes: { - chicken: {}, - [ newChildBlockId ]: { - attr: false, - attr2: 'perfect', - }, - }, + attributes: new Map( + Object.entries( { + chicken: {}, + [ newChildBlockId ]: { + attr: false, + attr2: 'perfect', + }, + } ) + ), order: { '': [ 'chicken' ], chicken: [ newChildBlockId ], @@ -308,9 +312,11 @@ describe( 'state', () => { isValid: true, }, }, - attributes: { - chicken: {}, - }, + attributes: new Map( + Object.entries( { + chicken: {}, + } ) + ), order: { '': [ 'chicken' ], chicken: [], @@ -358,13 +364,15 @@ describe( 'state', () => { isValid: true, }, }, - attributes: { - chicken: {}, - [ newChildBlockId ]: { - attr: false, - attr2: 'perfect', - }, - }, + attributes: new Map( + Object.entries( { + chicken: {}, + [ newChildBlockId ]: { + attr: false, + attr2: 'perfect', + }, + } ) + ), order: { '': [ 'chicken' ], chicken: [ newChildBlockId ], @@ -416,15 +424,17 @@ describe( 'state', () => { isValid: true, }, }, - attributes: { - chicken: {}, - 'chicken-child': { - attr: true, - }, - 'chicken-child-2': { - attr2: 'ok', - }, - }, + attributes: new Map( + Object.entries( { + chicken: {}, + 'chicken-child': { + attr: true, + }, + 'chicken-child-2': { + attr2: 'ok', + }, + } ) + ), order: { '': [ 'chicken' ], chicken: [ 'chicken-child', 'chicken-child-2' ], @@ -492,20 +502,22 @@ describe( 'state', () => { isValid: true, }, }, - attributes: { - chicken: {}, - [ newChildBlockId1 ]: { - attr: false, - attr2: 'perfect', - }, - [ newChildBlockId2 ]: { - attr: true, - attr2: 'not-perfect', - }, - [ newChildBlockId3 ]: { - attr2: 'hello', - }, - }, + attributes: new Map( + Object.entries( { + chicken: {}, + [ newChildBlockId1 ]: { + attr: false, + attr2: 'perfect', + }, + [ newChildBlockId2 ]: { + attr: true, + attr2: 'not-perfect', + }, + [ newChildBlockId3 ]: { + attr2: 'hello', + }, + } ) + ), order: { '': [ 'chicken' ], chicken: [ @@ -569,11 +581,13 @@ describe( 'state', () => { isValid: true, }, }, - attributes: { - chicken: {}, - 'chicken-child': {}, - 'chicken-grand-child': {}, - }, + attributes: new Map( + Object.entries( { + chicken: {}, + 'chicken-child': {}, + 'chicken-grand-child': {}, + } ) + ), order: { '': [ 'chicken' ], chicken: [ 'chicken-child' ], @@ -619,10 +633,12 @@ describe( 'state', () => { isValid: true, }, }, - attributes: { - chicken: {}, - [ newChildBlockId ]: {}, - }, + attributes: new Map( + Object.entries( { + chicken: {}, + [ newChildBlockId ]: {}, + } ) + ), order: { '': [ 'chicken' ], chicken: [ newChildBlockId ], @@ -647,7 +663,7 @@ describe( 'state', () => { expect( state ).toEqual( { byClientId: {}, - attributes: {}, + attributes: new Map(), order: {}, parents: {}, isPersistentChange: true, @@ -1019,7 +1035,7 @@ describe( 'state', () => { isValid: true, } ); - expect( state.attributes.chicken ).toEqual( { + expect( state.attributes.get( 'chicken' ) ).toEqual( { content: 'ribs', } ); expect( state.tree[ '' ].innerBlocks[ 0 ] ).toBe( @@ -1064,7 +1080,7 @@ describe( 'state', () => { isValid: false, } ); - expect( state.attributes.chicken ).toEqual( { + expect( state.attributes.get( 'chicken' ) ).toEqual( { ref: 3, } ); @@ -1427,7 +1443,7 @@ describe( 'state', () => { name: 'core/test-block', }, } ); - expect( state.attributes ).toEqual( { + expect( Object.fromEntries( state.attributes ) ).toEqual( { ribs: {}, } ); expect( state.tree[ '' ].innerBlocks ).toHaveLength( 1 ); @@ -1474,7 +1490,7 @@ describe( 'state', () => { name: 'core/test-block', }, } ); - expect( state.attributes ).toEqual( { + expect( Object.fromEntries( state.attributes ) ).toEqual( { ribs: {}, } ); } ); @@ -1795,7 +1811,42 @@ describe( 'state', () => { }, } ); - expect( state.attributes.kumquat.updated ).toBe( true ); + expect( state.attributes.get( 'kumquat' ).updated ).toBe( + true + ); + } ); + + it( 'should not updated equal attributes', () => { + const original = deepFreeze( + blocks( undefined, { + type: 'RESET_BLOCKS', + blocks: [ + { + clientId: 'kumquat', + attributes: {}, + innerBlocks: [], + }, + ], + } ) + ); + const state = blocks( original, { + type: 'UPDATE_BLOCK_ATTRIBUTES', + clientIds: [ 'kumquat' ], + attributes: { + updated: true, + }, + } ); + const updatedState = blocks( state, { + type: 'UPDATE_BLOCK_ATTRIBUTES', + clientIds: [ 'kumquat' ], + attributes: { + updated: true, + }, + } ); + + expect( state.attributes.get( 'kumquat' ) ).toBe( + updatedState.attributes.get( 'kumquat' ) + ); } ); it( 'should return with attribute block updates when attributes are unique by block', () => { @@ -1820,7 +1871,9 @@ describe( 'state', () => { uniqueByBlock: true, } ); - expect( state.attributes.kumquat.updated ).toBe( true ); + expect( state.attributes.get( 'kumquat' ).updated ).toBe( + true + ); } ); it( 'should accumulate attribute block updates', () => { @@ -1846,7 +1899,7 @@ describe( 'state', () => { }, } ); - expect( state.attributes.kumquat ).toEqual( { + expect( state.attributes.get( 'kumquat' ) ).toEqual( { updated: true, moreUpdated: true, } ); @@ -1914,7 +1967,7 @@ describe( 'state', () => { clientIds: [ 'kumquat' ], } ); - expect( state.attributes.kumquat ).toEqual( {} ); + expect( state.attributes.get( 'kumquat' ) ).toEqual( {} ); } ); } );