WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.33k stars 4.12k forks source link

React app are crashing if I use `<Spinner/>` inside `<Notice/>` #61199

Open cawa-93 opened 4 months ago

cawa-93 commented 4 months ago

Description

When I dynamically remove a <Spinner/> component from a <Notice/> component, the entire React-app crashes with an error:

Warning: React has detected a change in the order of Hooks called by Notice. 
This will lead to bugs and errors if not fixed. For more information, read the Rules of Hooks: https://reactjs.org/link/rules-of-hooks

   Previous render            Next render
   ------------------------------------------------------
1. useContext                 useEffect
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Step-by-step reproduction instructions

Full javascript code:

// app.jsx
import {createRoot, useEffect, useState} from '@wordpress/element';
import {Notice, Spinner} from '@wordpress/components';

export function CreatorForm() {
    const [status, setStatus] = useState('PENDING');

    useEffect(() => {
        setTimeout(() => setStatus('DONE'), 3000)
    }, []);

    return <Notice >
        {status}
        {
            status !== 'DONE' && <Spinner />
        }
    </Notice>;
}

const root = createRoot( document.querySelector('#ai-creator-app') );
root.render( <CreatorForm /> );

When component mounted it shows notice with "PENDING" text and spinner. After 3s spinner should hide and text should change to "DONE". But instead all react app crash.

Screenshots, screen recording, code snippet

No response

Environment info

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

stokesman commented 4 months ago

I've tested and can verify the issue.

Tinkering with it I found that various conditionally rendered components inside of Notice can cause this but some do not. The ones I tried that do not cause the issue include Tip and Animate. These components don’t use contextConnect or ContextSystemProvider and perhaps that makes the difference. Other components like View do cause the issue.

Then I tried replacing Notice with other components like View and Card and the issue was also avoided. I can’t yet pinpoint what goes wrong with Notice but I found that removing the use of the useSpokenMessages prevents the issue. However, I don’t see what’s wrong with that hook. I did find that using it from an anonymous component rendered as one of the children resolved the issue, but it’s still mysterious to me and an unidiomatic sort of fix.

@cawa-93, I did find that adding a key prop on the Notice allowed working around the issue.

<Notice key={ status }>
stokesman commented 4 months ago

I took another look at this and have concluded the issue is caused by use of renderToString on the client. The react docs do mention that it’s not recommended for this and how to do without so the useSpokenMessage hook could be refactored to avoid using it.

EDIT: One other tidbit, I've only been able to reproduce this in a standalone app. I did try it in a BlockEdit filter and PluginDocumentSettingPanel plugin within the block editor and there are no errors or crashes. I don’t know why that is.

@cawa-93, another (probably better) way to avoid this issue besides using key is to use the spokenMessage prop of Notice. It can be better in that it would avoid any excess spoken messages in case children of notice render multiple times with some non-textual markup changes. It’s not the case for your sample code but it is a potential pitfall.