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.

Sign up for GitHub

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

Block Editor: Add a new hook for getting a stable block context object #47028

Merged
merged 5 commits into from
Jan 16, 2023
Merged

Block Editor: Add a new hook for getting a stable block context object #47028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
572587e
Block Editor: Add a new hook for getting a stable block context object
Mamaduka Jan 10, 2023
7ceabc9
Update native files
Mamaduka Jan 12, 2023
2a4af31
Remove unused getBlockContext
Mamaduka Jan 12, 2023
1904d75
The block can be undefined
Mamaduka Jan 12, 2023
71e5776
Address feedback
Mamaduka Jan 12, 2023
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
39 changes: 0 additions & 39 deletions packages/block-editor/src/components/inner-blocks/get-block-context.js
View file
Open in desktop

This file was deleted.

27 changes: 4 additions & 23 deletions packages/block-editor/src/components/inner-blocks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { forwardRef, useMemo } from '@wordpress/element';
import { useSelect } from '@wordpress/data';
import {
getBlockSupport,
getBlockType,
store as blocksStore,
__unstableGetInnerBlocksProps as getInnerBlocksProps,
} from '@wordpress/blocks';
Expand All @@ -23,7 +22,7 @@ import ButtonBlockAppender from './button-block-appender';
import DefaultBlockAppender from './default-block-appender';
import useNestedSettingsUpdate from './use-nested-settings-update';
import useInnerBlockTemplateSync from './use-inner-block-template-sync';
import getBlockContext from './get-block-context';
import useBlockContext from './use-block-context';
import { BlockListItems } from '../block-list';
import { BlockContextProvider } from '../block-context';
import { useBlockEditContext } from '../block-edit/context';
Expand Down Expand Up @@ -75,28 +74,10 @@ function UncontrolledInnerBlocks( props ) {
templateInsertUpdatesSelection
);

const { context, name } = useSelect(
const context = useBlockContext( clientId );
const name = 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 (
Object.keys( blockType?.providesContext ?? {} ).length === 0
) {
return { name: block.name };
}

return {
context: getBlockContext( block.attributes, blockType ),
name: block.name,
};
return select( blockEditorStore ).getBlock( clientId )?.name;
},
[ clientId ]
);
Expand Down
74 changes: 27 additions & 47 deletions packages/block-editor/src/components/inner-blocks/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import {
getBlockType,
__unstableGetInnerBlocksProps as getInnerBlocksProps,
} from '@wordpress/blocks';
import { __unstableGetInnerBlocksProps as getInnerBlocksProps } from '@wordpress/blocks';
import { useRef } from '@wordpress/element';

/**
Expand All @@ -15,7 +11,7 @@ import ButtonBlockAppender from './button-block-appender';
import DefaultBlockAppender from './default-block-appender';
import useNestedSettingsUpdate from './use-nested-settings-update';
import useInnerBlockTemplateSync from './use-inner-block-template-sync';
import getBlockContext from './get-block-context';
import useBlockContext from './use-block-context';

/**
* Internal dependencies
Expand All @@ -26,7 +22,6 @@ import { useBlockEditContext } from '../block-edit/context';
import useBlockSync from '../provider/use-block-sync';
import { BlockContextProvider } from '../block-context';
import { defaultLayout, LayoutProvider } from '../block-list/layout';
import { store as blockEditorStore } from '../../store';

/**
* This hook is used to lightly mark an element as an inner blocks wrapper
Expand Down Expand Up @@ -100,10 +95,7 @@ function UncontrolledInnerBlocks( props ) {
useCompactList,
} = props;

const block = useSelect(
( select ) => select( blockEditorStore ).getBlock( clientId ),
[ clientId ]
) || { innerBlocks: [] };
const context = useBlockContext( clientId );

useNestedSettingsUpdate( clientId, allowedBlocks, templateLock );

Expand All @@ -116,43 +108,31 @@ function UncontrolledInnerBlocks( props ) {

const BlockListComponent = useCompactList ? BlockListCompact : BlockList;

let blockList = (
<BlockListComponent
marginVertical={ marginVertical }
marginHorizontal={ marginHorizontal }
rootClientId={ clientId }
renderAppender={ renderAppender }
renderFooterAppender={ renderFooterAppender }
withFooter={ false }
orientation={ orientation }
parentWidth={ parentWidth }
horizontalAlignment={ horizontalAlignment }
horizontal={ horizontal }
contentResizeMode={ contentResizeMode }
contentStyle={ contentStyle }
onAddBlock={ onAddBlock }
onDeleteBlock={ onDeleteBlock }
filterInnerBlocks={ filterInnerBlocks }
gridProperties={ gridProperties }
blockWidth={ blockWidth }
/>
return (
<LayoutProvider value={ layout }>
<BlockContextProvider value={ context }>
<BlockListComponent
marginVertical={ marginVertical }
marginHorizontal={ marginHorizontal }
rootClientId={ clientId }
renderAppender={ renderAppender }
renderFooterAppender={ renderFooterAppender }
withFooter={ false }
orientation={ orientation }
parentWidth={ parentWidth }
horizontalAlignment={ horizontalAlignment }
horizontal={ horizontal }
contentResizeMode={ contentResizeMode }
contentStyle={ contentStyle }
onAddBlock={ onAddBlock }
onDeleteBlock={ onDeleteBlock }
filterInnerBlocks={ filterInnerBlocks }
gridProperties={ gridProperties }
blockWidth={ blockWidth }
/>
</BlockContextProvider>
</LayoutProvider>
);

// Wrap context provider if (and only if) block has context to provide.
const blockType = getBlockType( block.name );
if ( blockType && blockType.providesContext ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This original condition was always true, so there's no harm in wrapping the block in a context provider similar to the web version. See #46729.

const context = getBlockContext( block.attributes, blockType );

blockList = (
<LayoutProvider value={ layout }>
Copy link
Member

Choose a reason for hiding this comment

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

Seems to me you fixed a bug here: previously, if the component didn't provide a context, but did provide a layout, the LayoutProvider wasn't rendered and the layout wasn't applied.

I see quite a few blocks, very common ones, that are like this, i.e., their block.json has supports.__experimentalLayout field, but doesn't have providesContext. Buttons, Column, Columns, Group, ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Sort of. This was only working because blockType && blockType.providesContext was always truthy.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes, so it was only potential, not real bug 👍

<BlockContextProvider value={ context }>
{ blockList }
</BlockContextProvider>
</LayoutProvider>
);
}

return blockList;
}

/**
Expand Down
47 changes: 47 additions & 0 deletions packages/block-editor/src/components/inner-blocks/use-block-context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/**
* WordPress dependencies
*/
import { store as blocksStore } from '@wordpress/blocks';
import { useSelect } from '@wordpress/data';

/**
* Internal dependencies
*/
import { store as blockEditorStore } from '../../store';

/**
* Returns a context object for a given block.
*
* @param {string} clientId The block client ID.
*
* @return {Record<string,*>} Context value.
*/
export default function useBlockContext( clientId ) {
return useSelect(
( select ) => {
const block = select( blockEditorStore ).getBlock( clientId );
if ( ! block ) {
return undefined;
}

const blockType = select( blocksStore ).getBlockType( block.name );
if ( ! blockType ) {
return undefined;
}

if ( Object.keys( blockType.providesContext ).length === 0 ) {
Copy link
Member

Choose a reason for hiding this comment

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

Any chance provideContext will not exist, causing Object.keys( undefined ) to crash?

Copy link
Member

@jsnajdr jsnajdr Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When providesContext doesn't exist in block.json, it will get defaulted to an empty object here.

There's only one edge case: if I specify providesContext: null in block.json, it will not get defaulted and Object.keys will really crash.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's worth adding a ?? {} or an additional non-nullish check just in case.

Copy link
Member

Choose a reason for hiding this comment

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

I realized there's the providesContext: null case only after your comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds like intentionally breaking a block. What if someone sets a string instead of an object?

I think this is something that block register should handle.

Copy link
Member

Choose a reason for hiding this comment

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

String is fine, it's an array-like object that has keys. Even number will be accepted. Only null or undefined will crash.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is something that block register should handle.

I don't disagree, after all, we probably have many places where we expect a string value in block.json, and do unguarded .includes or .startsWith calls. A malicious null will crash them, too.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. That's why I approved the PR in the first place and considered this a minor thing. Leaving it to your judgement @Mamaduka.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks again for the great feedback. I will merge this as it is. But happy to add extra checks in the future. Maybe as a part of block registration.

return undefined;
}

return Object.fromEntries(
Object.entries( blockType.providesContext ).map(
( [ contextName, attributeName ] ) => [
contextName,
block.attributes[ attributeName ],
]
)
);
},
[ clientId ]
);
}