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

Batch container block settings action calls #43958

Merged
merged 12 commits into from
Sep 19, 2022
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ function BlockListCompact( props ) {
};

return (
<View style={ containerStyle }>
<View style={ containerStyle } testID="block-list-wrapper">
{ blockClientIds.map( ( currentClientId ) => (
<BlockListBlock
clientId={ currentClientId }
Expand Down
7 changes: 7 additions & 0 deletions packages/block-editor/src/components/inner-blocks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ function UncontrolledInnerBlocks( props ) {
const context = useSelect(
( select ) => {
const block = select( blockEditorStore ).getBlock( clientId );

// This check is here to avoid the Redux zombie bug where a child subscription
// is called before a parent, causing potential JS errors when the child has been removed.
if ( ! block ) {
return;
}

const blockType = getBlockType( block.name );

if ( ! blockType || ! blockType.providesContext ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,42 +47,53 @@ export default function useInnerBlockTemplateSync(
( select ) => select( blockEditorStore ).getBlocks( clientId ),
[ clientId ]
);
const { getBlocks } = useSelect( blockEditorStore );

// Maintain a reference to the previous value so we can do a deep equality check.
const existingTemplate = useRef( null );
useLayoutEffect( () => {
// Only synchronize innerBlocks with template if innerBlocks are empty
// or a locking "all" or "contentOnly" exists directly on the block.
if (
innerBlocks.length === 0 ||
templateLock === 'all' ||
templateLock === 'contentOnly'
) {
// There's an implicit dependency between useInnerBlockTemplateSync and useNestedSettingsUpdate
// The former needs to happen after the latter and since the latter is using microtasks to batch updates (performance optimization),
// We need to schedule this one in a microtask as well.
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
// Exemple: If you remove queueMicrotask here, ctrl + click to insert quote block won't close the inserter.
window.queueMicrotask( () => {
// Only synchronize innerBlocks with template if innerBlocks are empty
// or a locking "all" or "contentOnly" exists directly on the block.
const currentInnerBlocks = getBlocks( clientId );
const shouldApplyTemplate =
currentInnerBlocks.length === 0 ||
templateLock === 'all' ||
templateLock === 'contentOnly';

const hasTemplateChanged = ! isEqual(
template,
existingTemplate.current
);
if ( hasTemplateChanged ) {
existingTemplate.current = template;
const nextBlocks = synchronizeBlocksWithTemplate(
innerBlocks,
template

if ( ! shouldApplyTemplate || ! hasTemplateChanged ) {
return;
}

existingTemplate.current = template;
const nextBlocks = synchronizeBlocksWithTemplate(
currentInnerBlocks,
template
);

if ( ! isEqual( nextBlocks, currentInnerBlocks ) ) {
replaceInnerBlocks(
clientId,
nextBlocks,
currentInnerBlocks.length === 0 &&
templateInsertUpdatesSelection &&
nextBlocks.length !== 0,
// This ensures the "initialPosition" doesn't change when applying the template
// If we're supposed to focus the block, we'll focus the first inner block
// otherwise, we won't apply any auto-focus.
// This ensures for instance that the focus stays in the inserter when inserting the "buttons" block.
getSelectedBlocksInitialCaretPosition()
);
if ( ! isEqual( nextBlocks, innerBlocks ) ) {
replaceInnerBlocks(
clientId,
nextBlocks,
innerBlocks.length === 0 &&
templateInsertUpdatesSelection &&
nextBlocks.length !== 0,
// This ensures the "initialPosition" doesn't change when applying the template
// If we're supposed to focus the block, we'll focus the first inner block
// otherwise, we won't apply any auto-focus.
// This ensures for instance that the focus stays in the inserter when inserting the "buttons" block.
getSelectedBlocksInitialCaretPosition()
);
}
}
}
} );
}, [ innerBlocks, template, templateLock, clientId ] );
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* WordPress dependencies
*/
import { useLayoutEffect, useMemo } from '@wordpress/element';
import { useSelect, useDispatch } from '@wordpress/data';
import { useSelect, useDispatch, useRegistry } from '@wordpress/data';
import isShallowEqual from '@wordpress/is-shallow-equal';

/**
Expand All @@ -13,6 +13,8 @@ import { getLayoutType } from '../../layouts';

/** @typedef {import('../../selectors').WPDirectInsertBlock } WPDirectInsertBlock */

const pendingSettingsUpdates = new WeakMap();

/**
* This hook is a side effect which updates the block-editor store when changes
* happen to inner block settings. The given props are transformed into a
Expand Down Expand Up @@ -46,6 +48,7 @@ export default function useNestedSettingsUpdate(
layout
) {
const { updateBlockListSettings } = useDispatch( blockEditorStore );
const registry = useRegistry();

const { blockListSettings, parentLock } = useSelect(
( select ) => {
Expand Down Expand Up @@ -98,7 +101,29 @@ export default function useNestedSettingsUpdate(
}

if ( ! isShallowEqual( blockListSettings, newSettings ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What setting causes isShallowEqual to be false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none, it's just the first time this gets called, it needs to be batched across the 500 blocks that call it in the same render commit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok. Why is that not initialised/preloaded in the store though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, am bit slow today. Because the information is in a React component of course.

updateBlockListSettings( clientId, newSettings );
// This is an optimization for the initial rendering of a block list
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
// To avoid triggering updateBlockListSettings for each container block
// causing X re-renderings for X container blocks,
// we batch all the updatedBlockListSettings in a single "data" batch
// which results in a single re-render.
if ( ! pendingSettingsUpdates.get( registry ) ) {
pendingSettingsUpdates.set( registry, [] );
}
pendingSettingsUpdates
.get( registry )
.push( [ clientId, newSettings ] );
window.queueMicrotask( () => {
if ( pendingSettingsUpdates.get( registry )?.length ) {
registry.batch( () => {
pendingSettingsUpdates
.get( registry )
.forEach( ( args ) => {
updateBlockListSettings( ...args );
} );
pendingSettingsUpdates.set( registry, [] );
} );
}
} );
}
}, [
clientId,
Expand All @@ -112,5 +137,6 @@ export default function useNestedSettingsUpdate(
orientation,
updateBlockListSettings,
layout,
registry,
] );
}
3 changes: 3 additions & 0 deletions packages/block-editor/src/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1714,9 +1714,12 @@ export function automaticChangeStatus( state, action ) {
case 'SET_BLOCK_VISIBILITY':
case 'START_TYPING':
case 'STOP_TYPING':
case 'UPDATE_BLOCK_LIST_SETTINGS':
return state;
}

// TODO: This is a source of bug, as each time there's a change in timing,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to add more info here about what the bug actually is, for future reference. Maybe the link to the PR? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no identified bug, it's just a bad pattern (returning undefined from a reducer) that can cause bugs quickly and these bugs are very hard to track.

// or a new action is added, this could break.
// Reset the state by default (for any action not handled).
}

Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1435,7 +1435,7 @@ export function getTemplateLock( state, rootClientId ) {

const blockListSettings = getBlockListSettings( state, rootClientId );
if ( ! blockListSettings ) {
return null;
return undefined;
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
}

return blockListSettings.templateLock;
Expand Down
8 changes: 4 additions & 4 deletions packages/block-editor/src/store/test/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -3253,7 +3253,7 @@ describe( 'selectors', () => {
expect( getTemplateLock( state ) ).toBe( 'all' );
} );

it( 'should return null if the specified clientId was not found', () => {
it( 'should return undefined if the specified clientId was not found', () => {
const state = {
settings: { templateLock: 'all' },
blockListSettings: {
Expand All @@ -3263,10 +3263,10 @@ describe( 'selectors', () => {
},
};

expect( getTemplateLock( state, 'ribs' ) ).toBe( null );
expect( getTemplateLock( state, 'ribs' ) ).toBe( undefined );
} );

it( 'should return null if template lock was not set on the specified block', () => {
it( 'should return undefined if template lock was not set on the specified block', () => {
const state = {
settings: { templateLock: 'all' },
blockListSettings: {
Expand All @@ -3276,7 +3276,7 @@ describe( 'selectors', () => {
},
};

expect( getTemplateLock( state, 'ribs' ) ).toBe( null );
expect( getTemplateLock( state, 'ribs' ) ).toBe( undefined );
} );

it( 'should return the template lock for the specified clientId', () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`List V2 block adds one item to the list 1`] = `
exports[`List block adds one item to the list 1`] = `
"<!-- wp:list -->
<ul><!-- wp:list-item -->
<li>First list item</li>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->"
`;

exports[`List V2 block changes the indentation level 1`] = `
exports[`List block changes the indentation level 1`] = `
"<!-- wp:list -->
<ul><!-- wp:list-item -->
<li>Item 1<!-- wp:list -->
Expand All @@ -20,7 +20,7 @@ exports[`List V2 block changes the indentation level 1`] = `
<!-- /wp:list -->"
`;

exports[`List V2 block changes to ordered list 1`] = `
exports[`List block changes to ordered list 1`] = `
"<!-- wp:list {\\"ordered\\":true} -->
<ol><!-- wp:list-item -->
<li>Item 1</li>
Expand All @@ -36,7 +36,7 @@ exports[`List V2 block changes to ordered list 1`] = `
<!-- /wp:list -->"
`;

exports[`List V2 block changes to reverse ordered list 1`] = `
exports[`List block changes to reverse ordered list 1`] = `
"<!-- wp:list {\\"ordered\\":true,\\"reversed\\":true} -->
<ol reversed><!-- wp:list-item -->
<li>Item 1</li>
Expand All @@ -52,15 +52,15 @@ exports[`List V2 block changes to reverse ordered list 1`] = `
<!-- /wp:list -->"
`;

exports[`List V2 block inserts block 1`] = `
exports[`List block inserts block 1`] = `
"<!-- wp:list -->
<ul><!-- wp:list-item -->
<li></li>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->"
`;

exports[`List V2 block removes the indentation level 1`] = `
exports[`List block removes the indentation level 1`] = `
"<!-- wp:list -->
<ul><!-- wp:list-item -->
<li>Item 1</li>
Expand All @@ -72,7 +72,7 @@ exports[`List V2 block removes the indentation level 1`] = `
<!-- /wp:list -->"
`;

exports[`List V2 block sets a start value to an ordered list 1`] = `
exports[`List block sets a start value to an ordered list 1`] = `
"<!-- wp:list {\\"ordered\\":true,\\"start\\":25} -->
<ol start=\\"25\\"><!-- wp:list-item -->
<li>Item 1</li>
Expand All @@ -88,7 +88,7 @@ exports[`List V2 block sets a start value to an ordered list 1`] = `
<!-- /wp:list -->"
`;

exports[`List V2 block shows different indentation levels 1`] = `
exports[`List block shows different indentation levels 1`] = `
"<!-- wp:list -->
<ul><!-- wp:list-item -->
<li>List item 1</li>
Expand Down Expand Up @@ -119,27 +119,3 @@ exports[`List V2 block shows different indentation levels 1`] = `
<!-- /wp:list-item --></ul>
<!-- /wp:list -->"
`;

exports[`List block inserts block 1`] = `
"<!-- wp:list -->
<ul><!-- wp:list-item -->
<li></li>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->"
`;

exports[`List block renders a list with a few items 1`] = `
"<!-- wp:list -->
<ul><!-- wp:list-item -->
<li>Item 1</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>Item 2</li>
<!-- /wp:list-item -->

<!-- wp:list-item -->
<li>Item 3</li>
<!-- /wp:list-item --></ul>
<!-- /wp:list -->"
`;
Loading