Gutenberg: Cannot insert Heading component into Cover

[senadir]

Describe the bug
when inserting a new Cover, changing the paragraph to heading using "/" break it
related: https://github.com/WordPress/gutenberg/issues/16959
possibly related: https://github.com/WordPress/gutenberg/issues/16851
To reproduce
Steps to reproduce the behavior:

1. create a new cover block
2. set a background
3. in the paragraph field, write / to insert a heading
4. it breaks

Expected behavior
Paragraph should turn into Heading block

Screenshots

Desktop (please complete the following information):

• OS: OSX
• Browser: Chrome
• Version: Latest

Additional context

• Gutenberg on master

[senadir]

Narrowed down the problem to block-list/block & hooks/align
no block is being passed the moment of transfer, so props is empty (no attributes, block name..)
https://github.com/WordPress/gutenberg/blob/cd6f74ca65382952e0446a8c119023f0961336a4/packages/block-editor/src/hooks/align.js#L142-L145
testing on Gutenberg 5.8.0 (version just prior to the version https://github.com/WordPress/gutenberg/pull/15038 was merged on) showed no problem, so I'm assuming it's a regression.
I think this comment has something to do with it
https://github.com/WordPress/gutenberg/blob/cd6f74ca65382952e0446a8c119023f0961336a4/packages/block-editor/src/components/block-list/block.js#L641-L644

cc @youknowriad

[youknowriad]

So apparently, withSelect is being called with the old clientId. It looks like this is the zombie children bug for me. Is it possible that errors are not caught properly with the useSelect refactoring?

Apparently, this regression happened in the same release that introduced useSelect too.

cc @epiqueras @nerrad

[youknowriad]

Ohhh! I think It might be because the error is not triggered in withSelect directly but it's triggered in another Higher-order component withFilters that is based on the result of the withSelect. (Makes me wonder if we should trigger an error explicitly in withSelect if there's no block. Seems like a hacky fix though :)

[senadir]

useSelect will just throw the error again at you with a stack
https://github.com/WordPress/gutenberg/blob/cd6f74ca65382952e0446a8c119023f0961336a4/packages/data/src/components/use-select/index.js#L92-L110

[epiqueras]

We need the error to be thrown inside mapSelect for the failsafe to work. I would just refactor the mapSelect of this component to check that it's not running with stale props.

[senadir]

@epiqueras can you demonstrate how can this be done? I've already tried throwing an error inside mapSelect callback inside the withSelect but it resulted in the same problem, useSelect would catch that error & throw it again.
specifically, you can catch the block inside applyWithSelect@block-list/blockif it's defined or not

[epiqueras]

Where is the error coming from? withSelect or withFilter? useSelect returns the previous result if mapSelect fails so also keep that in mind with what you're trying.

[senadir]

while doing some basic debugging with @youknowriad we found that it's in withSelect, no error is triggered since it's returning the previous block clientID (so if you try to transform a paragraph to heading, withSelect will try to get you the paragraph block clientID, even though the store has the heading block, this triggers no error in withSelect since there is no check right now, the error only surface down in withFilter when another components need to use the new block

doing a manual check inside withSelect like this will just surface the error earlier but would still fail & break the component

if ( !block) throw new Error( 'no block' ); 

if you have a minute or two maybe you can give it a try so if it persists

[epiqueras]

Can we change it so that withFilter doesn't fail and waits for the new client ID in a subsequent render?

[youknowriad]

I'm personally a bit concerned about this and why we're not capable of providing a solution that works out of the box without touching the component itself.

The "least bad" solution I see is to have a HoC between withSelect and withFilters that just renders nothing if the block is not available.

[nerrad]

I still haven't fully dug into the issue yet so I'm just going by comments so far. But I did want to raise these points:

• It's not clear to me yet what exactly is the source of the error. The cause is clear but not what is triggering that cause.
• To fix this specific issue, what @youknowriad proposes sounds like a path forward.
• However, I'm wary that this may be surfacing a larger flaw either in withSelect or withFilter that could surface elsewhere... so even if the quick fix is implemented, we'd should still investigate whether either of these hocs needs a closer look (or "gotchas" documented to prevent future occurrences).

I'll try and see if I can debug this further sometime today but for now just thought I'd give my two cents :)

[nerrad]

Yea if you just add this before the withFilters call in components/block-list/block.js it appears to fix:

ifCondition( ( { block } ) => !! block ),

You'll need to import ifCondition from @wordpress/compose.

[swissspidy]

I'd love to fix the actual root cause, because these kind of errors pop up every now and then. See for example #15543 and #16673.

[youknowriad]

@swissspidy I would love too but the truth is that we might not be able to fix the root issue. hooks in react-redux suffer from the same issue too. I think if a fix should come, there needs to be a new React API allowing "parent hooks" to trigger before "children hooks" which is not possible AFAIK today.

We could refactor the component to rely on useSelect instead of withSelect and I think it would at least be less problematic but in that case, there's a lot of plugins (including yours @swissspidy ) that might still use props coming from the withSelect in the BlockListBlock filter. IMO, this should be deprecated anyway and the only prop this filter should access to is clientId but it feels too late.

[epiqueras]

From docs:

Don't rely on props in your selector function for extracting data.
In cases where you do rely on props in your selector function and those props may change over time, or the data you're extracting may be based on items that can be deleted, try writing the selector functions defensively. Don't just reach straight into state.todos[props.id].name - read state.todos[props.id] first, and verify that it exists before trying to read todo.name.

[senadir]

@epiqueras how do you force a re-render from inside the select function? or is @nerrad the only possible solution?

[epiqueras]

By changing state or throwing an error.

[youknowriad]

@epiqueras That's from the react-redux docs.

Also, the useSelect API was added after the fact which means all of our components were not written with that aspect in my mind. Do you think it's fair to make that assemption without rewriting everything?

[epiqueras]

Yes, but it also applies to our hook. I'm quoting it because that's what we should strive for going forward.

The components were already broken though, the original withSelect had the same issue because it didn't use Redux's Subscription component that establishes a hierarchy, and some side effects rely on being called in the constructor to be called in the correct order.

[youknowriad]

The original withSelect impelementation might have some edge cases where this triggered but it was largely solved (granted it's not async mode compatible). The fact that this works the commit before useSelect is introduced is telling.

[epiqueras]

The fact that this works the commit before useSelect is introduced is telling.

Yes, but what do you propose for now? I think we should just refactor this component to follow the react-redux hook guidelines.

[senadir]

since this problem was only introduced in innerBlocks, we can start with it for now

[youknowriad]

You're right that we don't have too much choice here. We could:

• add those guidelines somewhere
• Apply the fix @nerrad suggested for this particular component. (Let's comment why it was added)

In an ideal world, React would provide an API for us to fix this. Not sure if they're considering it but I'm pretty sure we're not the only ones having that need.

[epiqueras]

Let's do both.

In an ideal world, React would provide an API for us to fix this. Not sure if they're considering it but I'm pretty sure we're not the only ones having that need.

The Redux connect HOC solves it, because every wrapped component replaces its subtree's store with a clone that runs its subscriptions after its parent. They also suggest that if you can't get around the error by modifying your selecting functions, you could just wrap the component in connect so that the subscription hierarchy can kick in. We could do something similar in our registry implementation by leveraging their low level Subscription component, which implements this hierarchical callback firing.

[youknowriad]

Anyone willing to push the fix above?

[senadir]

Darren's Fix?

[youknowriad]

yes

[ellatrix]

Any further ideas for a proper fix?

[epiqueras]

I left a suggestion above.

[senadir]

@epiqueras Will there be any side effects to that?

[epiqueras]

There shouldn’t be.

On Sat, Jan 11, 2020 at 11:08 AM Seghir Nadir [e-mail]
wrote:

@epiqueras https://github.com/epiqueras Will there be any side effects
to that?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/WordPress/gutenberg/issues/17013?email_source=notifications&email_token=AESFA2ESTON6FURZUES6PDTQ5HVJVA5CNFSM4ILER26KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIWFDWY#issuecomment-573329883,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AESFA2A6L4HOSDAD2RQQYK3Q5HVJVANCNFSM4ILER26A
.

[leeshadle]

Hi @senadir, @youknowriad, @epiqueras
I created a hot module replacement (hmr) package for block development. Versions 7.4.0+ of Gutenberg break my hmr package.

The way my hmr package works is if a block's code changes on save, then I unregister ( unregisterBlockType ) the changed block and then immediately register ( registerBlockType ) the block with the updated code.

I'm getting the error in the screenshot below. This error lead me to the BlockListBlock, who's comments below lead me to here. I'm not sure if this is highlighting the same issue, but I was curious as to your thoughts?

// block is sometimes not mounted at the right time, causing it be undefined
// see issue for more info https://github.com/WordPress/gutenberg/issues/17013

[epiqueras]

This is something else. A render is taking place between the time you unregister and register the new block. We need to guard against that or overwrite the block without unregistering it first.