Skip to content

Block.* in save component #20721

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

Closed
ellatrix wants to merge 4 commits into from
Closed

Block.* in save component #20721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c987842
Block save component
ellatrix Mar 9, 2020
af104c0
Add block class
ellatrix Mar 9, 2020
424cdb2
Mark export unstable
ellatrix Mar 24, 2020
c924aa3
Try adding to columns
ellatrix Mar 24, 2020
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
26 changes: 26 additions & 0 deletions packages/block-editor/src/components/block-list/block-wrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { focus, isTextField, placeCaretAtHorizontalEdge } from '@wordpress/dom';
import { BACKSPACE, DELETE, ENTER } from '@wordpress/keycodes';
import { __, sprintf } from '@wordpress/i18n';
import { useSelect, useDispatch } from '@wordpress/data';
import { __unstableBlockPropsFilterContext as BlockPropsFilterContext } from '@wordpress/blocks';

/**
* Internal dependencies
Expand Down Expand Up @@ -235,11 +236,36 @@ const elements = [
'nav',
];

const BlockSaveComponent = forwardRef(
( { children, tagName: TagName = 'div', ...props }, wrapper ) => {
return (
<BlockPropsFilterContext.Consumer>
{ ( filter ) => {
return (
Copy link
Member Author

@ellatrix ellatrix Mar 24, 2020

Choose a reason for hiding this comment

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

I'm running into a really strange problem here. filter returns the BlockContent function from the BlockContentProvider (for InnerBlocks)... even though I'm subscribing to BlockPropsFilterContext. If I swap the two provider components, then it works, but then InnerBlocks.Content stops working. @youknowriad Any idea why context is behaving strangely during serialisation? Been staring at this for a while. 🤔

Copy link
Contributor

@youknowriad youknowriad Mar 24, 2020

Choose a reason for hiding this comment

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

I know that we have a custom serializer implementation and we don't rely on React. That might be the reason. cc @aduth

Copy link
Member

@aduth aduth Mar 25, 2020

Choose a reason for hiding this comment

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

Yeah, it could very well be an issue in the implementation of the custom serializer.

In particular, looking at this code, I can see how it might be problematic for two separate context values to coexist in the same render tree, since the second argument would replace the current context:

gutenberg/packages/element/src/serialize.js

Lines 413 to 414 in b7125c0

case Provider.$$typeof:
return renderChildren( props.children, props.value, legacyContext );

It's likely not been discovered since our use of context in serialization is pretty minimal to date.

The solution may be a bit tricky, since we context still needs to be associated 1:1 for a given provider/context pair, and we can't assume anything about the shape of that value (i.e. can't be doing any Object.assign).

I'd guess we might want to implement context here as some [Weak]Map which can be looked up by Provider class at the time if/when the Consumer is rendered.

Failing test case:

it( 'renders provided value through multiple context providers', () => {
	const {
		Consumer: FirstConsumer,
		Provider: FirstProvider,
	} = createContext();
	const { Provider: SecondProvider } = createContext();

	const result = renderElement(
		<FirstProvider value="first">
			<SecondProvider value="second">
				<FirstConsumer>
					{ ( context ) => context.value }
				</FirstConsumer>
			</SecondProvider>
		</FirstProvider>
	);

	expect( result ).toBe( 'first' );
} );

On the general point of the custom serializer, there's other issues here as well (notably, hooks, see #15873). Even if we fixed the above implementation of serializer provider rendering, I'd guess useContext likely still wouldn't work? I'm open to reevaluating this implementation, depending how much a burden it continues to be. For context on why it exists in the first place, see #5897 (#3353).

Copy link
Member

@aduth aduth Mar 25, 2020

Choose a reason for hiding this comment

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

See: #21156

<TagName ref={ wrapper } { ...filter( props ) }>
{ children }
</TagName>
);
} }
</BlockPropsFilterContext.Consumer>
);
}
);

const ExtendedBlockComponent = elements.reduce( ( acc, element ) => {
acc[ element ] = forwardRef( ( props, ref ) => {
return <BlockComponent { ...props } ref={ ref } tagName={ element } />;
} );
return acc;
}, BlockComponent );

ExtendedBlockComponent.Save = elements.reduce( ( acc, element ) => {
acc[ element ] = forwardRef( ( props, ref ) => {
return (
<BlockSaveComponent { ...props } ref={ ref } tagName={ element } />
);
} );
return acc;
}, BlockSaveComponent );

export const Block = ExtendedBlockComponent;
9 changes: 6 additions & 3 deletions packages/block-library/src/column/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { InnerBlocks } from '@wordpress/block-editor';
import {
InnerBlocks,
__experimentalBlock as Block,
} from '@wordpress/block-editor';

export default function save( { attributes } ) {
const { verticalAlignment, width } = attributes;
Expand All @@ -21,8 +24,8 @@ export default function save( { attributes } ) {
}

return (
<div className={ wrapperClasses } style={ style }>
<Block.Save.div className={ wrapperClasses } style={ style }>
<InnerBlocks.Content />
</div>
</Block.Save.div>
);
}
13 changes: 10 additions & 3 deletions packages/block-library/src/columns/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { InnerBlocks, getColorClassName } from '@wordpress/block-editor';
import {
InnerBlocks,
getColorClassName,
__experimentalBlock as Block,
} from '@wordpress/block-editor';

export default function save( { attributes } ) {
const {
Expand Down Expand Up @@ -38,8 +42,11 @@ export default function save( { attributes } ) {
};

return (
<div className={ className ? className : undefined } style={ style }>
<Block.Save.div
Copy link
Member

@aduth aduth Mar 25, 2020

Choose a reason for hiding this comment

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

Reiterating a concern I raised in previous pull requests: I can't help but foresee this becoming a never-ending maintenance burden. For what reason do we need to whitelist these tag names?

Could it be something where we use instead some prop, like...

Copy link
Contributor

@youknowriad youknowriad Mar 25, 2020

Choose a reason for hiding this comment

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

Could it be something where we use instead some prop, like...

The main idea of Block.* is that you can pass it, itself as a tagName of another component (RichText, InnerBlocks)

That said, I think it has limits, for example, it's impossible to support custom tags (which I think we should support).

So we might have to find a new approach to solve both problems.

className={ className ? className : undefined }
style={ style }
>
<InnerBlocks.Content />
</div>
</Block.Save.div>
);
}
13 changes: 9 additions & 4 deletions packages/block-library/src/image/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ import { isEmpty } from 'lodash';
/**
* WordPress dependencies
*/
import { RichText } from '@wordpress/block-editor';
import {
RichText,
__experimentalBlock as Block,
} from '@wordpress/block-editor';

export default function save( { attributes } ) {
const {
Expand Down Expand Up @@ -67,11 +70,13 @@ export default function save( { attributes } ) {

if ( 'left' === align || 'right' === align || 'center' === align ) {
return (
<div>
<Block.Save.div>
<figure className={ classes }>{ figure }</figure>
</div>
</Block.Save.div>
);
}

return <figure className={ classes }>{ figure }</figure>;
return (
<Block.Save.figure className={ classes }>{ figure }</Block.Save.figure>
);
}
3 changes: 2 additions & 1 deletion packages/block-library/src/paragraph/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
getColorClassName,
getFontSizeClass,
RichText,
__experimentalBlock as Block,
} from '@wordpress/block-editor';

export default function save( { attributes } ) {
Expand Down Expand Up @@ -51,7 +52,7 @@ export default function save( { attributes } ) {

return (
<RichText.Content
tagName="p"
tagName={ Block.Save.p }
style={ styles }
className={ className ? className : undefined }
value={ content }
Expand Down
1 change: 1 addition & 0 deletions packages/blocks/src/api/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export {
getBlockMenuDefaultClassName,
getSaveElement,
getSaveContent,
BlockPropsFilterContext as __unstableBlockPropsFilterContext,
} from './serializer';
export { isValidBlockContent } from './validation';
export { getCategories, setCategories, updateCategory } from './categories';
Expand Down
74 changes: 50 additions & 24 deletions packages/blocks/src/api/serializer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ import { isEmpty, reduce, isObject, castArray, startsWith } from 'lodash';
/**
* WordPress dependencies
*/
import { Component, cloneElement, renderToString } from '@wordpress/element';
import {
Component,
cloneElement,
renderToString,
createContext,
} from '@wordpress/element';
import { hasFilter, applyFilters } from '@wordpress/hooks';
import isShallowEqual from '@wordpress/is-shallow-equal';

Expand All @@ -17,6 +22,7 @@ import {
getBlockType,
getFreeformContentHandlerName,
getUnregisteredTypeHandlerName,
hasBlockSupport,
} from './registration';
import { normalizeBlockType } from './utils';
import BlockContentProvider from '../block-content-provider';
Expand Down Expand Up @@ -68,6 +74,8 @@ export function getBlockMenuDefaultClassName( blockName ) {
);
}

export const BlockPropsFilterContext = createContext();

/**
* Given a block type containing a save render implementation and attributes, returns the
* enhanced element to be saved or string when raw HTML expected.
Expand Down Expand Up @@ -95,27 +103,43 @@ export function getSaveElement(
}

let element = save( { attributes, innerBlocks } );
let blockPropsFilter = ( props ) => props;

if ( hasFilter( 'blocks.getSaveContent.extraProps' ) ) {
if ( hasBlockSupport( blockType, 'lightBlockWrapper', false ) ) {
blockPropsFilter = ( props ) => {
/**
* Filters the props applied to the block save result element.
*
* @param {Object} props Props applied to save element.
* @param {WPBlock} blockType Block type definition.
* @param {Object} attributes Block attributes.
*/
return applyFilters(
'blocks.getSaveContent.extraProps',
Copy link
Contributor

@youknowriad youknowriad Mar 19, 2020

Choose a reason for hiding this comment

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

One thing I was wondering is whether we should just apply this hook to the Edit version of Block.*

Copy link
Member Author

@ellatrix ellatrix Mar 24, 2020

Choose a reason for hiding this comment

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

We could, but we'd need more context and then the hook namespace also doesn't make sense anymore.

{ ...props },
blockType,
attributes
);
};
} else if ( isObject( element ) ) {
/**
* Filters the props applied to the block save result element.
*
* @param {Object} props Props applied to save element.
* @param {WPBlock} blockType Block type definition.
* @param {Object} attributes Block attributes.
*/
const props = applyFilters(
'blocks.getSaveContent.extraProps',
{ ...element.props },
blockType,
attributes
);

if (
isObject( element ) &&
hasFilter( 'blocks.getSaveContent.extraProps' )
) {
/**
* Filters the props applied to the block save result element.
*
* @param {Object} props Props applied to save element.
* @param {WPBlock} blockType Block type definition.
* @param {Object} attributes Block attributes.
*/
const props = applyFilters(
'blocks.getSaveContent.extraProps',
{ ...element.props },
blockType,
attributes
);

if ( ! isShallowEqual( props, element.props ) ) {
element = cloneElement( element, props );
if ( ! isShallowEqual( props, element.props ) ) {
element = cloneElement( element, props );
}
}
}

Expand All @@ -134,9 +158,11 @@ export function getSaveElement(
);

return (
<BlockContentProvider innerBlocks={ innerBlocks }>
{ element }
</BlockContentProvider>
<BlockPropsFilterContext.Provider value={ blockPropsFilter }>
<BlockContentProvider innerBlocks={ innerBlocks }>
{ element }
</BlockContentProvider>
</BlockPropsFilterContext.Provider>
);
}

Expand Down