Block.* in save component #20721
Block.* in save component #20721
Conversation
ellatrix
requested review from
ajitbohra,
jorgefilipecosta,
Soean,
talldan and
youknowriad
as code owners
|
Size Change: +180 B (0%) Total Size: 859 kB
ℹ️ View Unchanged
|
|
@youknowriad Wondering what you think of this, or if you had something else in mind. |
packages/block-editor/src/components/block-list/block-wrapper.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/block-wrapper.js
Outdated
Show resolved
Hide resolved
| * @param {Object} attributes Block attributes. | ||
| */ | ||
| applyFilters( | ||
| 'blocks.getSaveContent.extraProps', |
There was a problem hiding this comment.
One thing I was wondering is whether we should just apply this hook to the Edit version of Block.*
There was a problem hiding this comment.
We could, but we'd need more context and then the hook namespace also doesn't make sense anymore.
|
@youknowriad I wonder if we should name it |
ellatrix
force-pushed
the
try/block-save-component
branch
from
6a6fc9f
to
424cdb2
Compare
|
@ellatrix Yeah Block.Content seems a bit weird and potentially misleading. |
| return ( | ||
| <BlockPropsFilterContext.Consumer> | ||
| { ( filter ) => { | ||
| return ( |
There was a problem hiding this comment.
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. 🤔
There was a problem hiding this comment.
I know that we have a custom serializer implementation and we don't rely on React. That might be the reason. cc @aduth
There was a problem hiding this comment.
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
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).
There was a problem hiding this comment.
I do like the idea of reducing the magic we currently have, since this sort of thing will be really hard to decipher in future maintenance as to if and why that wrapper exists.
Coming from a position of relative ignorance, I'm still not sure I "get it"... as in, why can't I just use a div in my edit function if I want to render a div ? It's more of a rhetorical question, since I'm sure there are reasons, but I don't know that we can expect these to be immediately obvious (or when it is or isn't needed).
| @@ -38,8 +42,11 @@ export default function save( { attributes } ) { | |||
| }; | |||
|
|
|||
| return ( | |||
| <div className={ className ? className : undefined } style={ style }> | |||
| <Block.Save.div | |||
There was a problem hiding this comment.
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...
tagName(prior art)is(prior art)as(prior art)- At least make all tags possible, e.g. via Proxy? (browser support)
There was a problem hiding this comment.
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.
The idea is to require it if you're using the light block wrapper. Of course, we can choose not to go for this |
|
Replaced by #25644. |
Description
This PR mirrors
Block.*for light blocks in the save component, much like we do now forInnerBlocksandRichText. The benefit of this is that we could automatically add blocks classes with this component (if not opting out), making it seem less like a magical things that gets added during serialisation.Another benefit is that a block could potentially render something around the actual block (such as an alignment wrapper), which is possible right now in the edit function, but not in the save function.
How has this been tested?
Screenshots
Types of changes
Checklist: