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

Fix: Shared Blocks: State memory leak, each time inserter is opened new blocks are added #8153

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions editor/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,15 +112,20 @@ export function resetBlocks( blocks ) {
* Returns an action object used in signalling that blocks have been received.
* Unlike resetBlocks, these should be appended to the existing known set, not
* replacing.
* If the new blocks that are being received are replacing existing blocks,
* during the action dispatch it is possible to specify existing blocks that
* are removed before adding the new ones.
*
* @param {Object[]} blocks Array of block objects.
* @param {Object[]} blocks Array of block objects.
* @param {?Array<string>} clientIdsToRemove Array of clientIds.
*
* @return {Object} Action object.
*/
export function receiveBlocks( blocks ) {
export function receiveBlocks( blocks, clientIdsToRemove = [] ) {
return {
type: 'RECEIVE_BLOCKS',
blocks,
clientIdsToRemove,
};
}

Expand Down
25 changes: 22 additions & 3 deletions editor/store/effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import { BEGIN, COMMIT, REVERT } from 'redux-optimist';
import { get, includes, last, map, castArray, uniqueId, pick } from 'lodash';
import { filter, get, includes, last, map, castArray, uniqueId, pick } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -493,8 +493,27 @@ export default {
error,
} ) );
},
RECEIVE_SHARED_BLOCKS( action ) {
return receiveBlocks( map( action.results, 'parsedBlock' ) );
RECEIVE_SHARED_BLOCKS( action, { getState } ) {
const state = getState();
const markSharedBlock = ( blocks = [] ) => {
return map( blocks, ( block ) => {
const newBlock = {
...block,
isPartOfSharedBlock: true,
};
if ( block.innerBlocks ) {
newBlock.innerBlocks = markSharedBlock( block.innerBlocks );
}
return newBlock;
} );
};

const blocksToAdd = markSharedBlock( map( action.results, 'parsedBlock' ) )
const blocksToRemove = map(
filter( state.editor.present.blocksByClientId, 'isPartOfSharedBlock' ),
'clientId'
);
return receiveBlocks( blocksToAdd, blocksToRemove );
},
SAVE_SHARED_BLOCK( action, store ) {
// TODO: these are potentially undefined, this fix is in place
Expand Down
26 changes: 15 additions & 11 deletions editor/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ export const editor = flow( [

case 'RECEIVE_BLOCKS':
return {
...state,
...omit( state, action.clientIdsToRemove ),
...getFlattenedBlocks( action.blocks ),
};

Expand Down Expand Up @@ -405,14 +405,26 @@ export const editor = flow( [
},

blockOrder( state = {}, action ) {
const removeClientIdsFromState = ( initialState, clientIds ) => {
return flow( [
// Remove inner block ordering for removed blocks
( nextState ) => omit( nextState, clientIds ),

// Remove deleted blocks from other blocks' orderings
( nextState ) => mapValues( nextState, ( subState ) => (
without( subState, ...clientIds )
) ),
] )( initialState );
};

switch ( action.type ) {
case 'RESET_BLOCKS':
case 'SETUP_EDITOR_STATE':
return mapBlockOrder( action.blocks );

case 'RECEIVE_BLOCKS':
return {
...state,
...removeClientIdsFromState( state, action.clientIdsToRemove ),
...omit( mapBlockOrder( action.blocks ), '' ),
};

Expand Down Expand Up @@ -520,15 +532,7 @@ export const editor = flow( [
}

case 'REMOVE_BLOCKS':
return flow( [
// Remove inner block ordering for removed blocks
( nextState ) => omit( nextState, action.clientIds ),

// Remove deleted blocks from other blocks' orderings
( nextState ) => mapValues( nextState, ( subState ) => (
without( subState, ...action.clientIds )
) ),
] )( state );
removeClientIdsFromState( state, action.clientIds );
}

return state;
Expand Down