WordPress / gutenberg

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

infinite rerenders when using useInstanceId in template-part preview #63713

Closed silaskoehler closed 2 months ago

silaskoehler commented 3 months ago

Description

I get infinite rerenders when i use useInstanceId() in my custom block and save it to an attribute if it changes. for testing purpose i created a block using pnpx @wordpress/create-block@latest --variant dynamic and change the edit.js to this:

import { useBlockProps } from '@wordpress/block-editor';
import { useInstanceId } from '@wordpress/compose';

export default function Edit( { setAttributes, attributes } ) {
    const { uniqueId } = attributes;

    const instanceId = useInstanceId( Edit );

    useEffect( () => {
        console.log( uniqueId, instanceId );

        setAttributes( {
            uniqueId: instanceId,
        } );
    }, [ uniqueId ] );

    return <div { ...useBlockProps() }>uniqueId: { uniqueId }</div>;
}

In the template preview in the site editor (/wp-admin/site-editor.php?postType=wp_template) my block flickers and in the console I see infinite rerenders. The error only occurs when my block is in a Temaplte part.

Am I doing something wrong or is there a bug because it does a render every time I save the attribute. He doesn't do that in the block editor.

Step-by-step reproduction instructions

  1. Use default Theme and WP 6.6
  2. create a block using @wordpress/create-block@latest
  3. edit the edit.js as described above
  4. go to the template preview
  5. go to the console

Screenshots, screen recording, code snippet

https://github.com/user-attachments/assets/f666be2d-dc19-423f-b788-1e0c261c26c9

Environment info

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

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

talldan commented 3 months ago

It's not something I can reproduce. I think your effect dependency should be instanceId, btw.

silaskoehler commented 3 months ago

sorry for missing the useEffect dependency. Also i forget to import useEffect. I updated it to:

import { useBlockProps } from '@wordpress/block-editor';
import { useInstanceId } from '@wordpress/compose';
import { useEffect } from '@wordpress/element';

export default function Edit( { setAttributes, attributes } ) {
    const { uniqueId } = attributes;

    const instanceId = useInstanceId( Edit );

    useEffect( () => {
        console.log( uniqueId, instanceId );

        setAttributes( {
            uniqueId: instanceId,
        } );
    }, [ uniqueId,instanceId ] );

    return <div { ...useBlockProps() }>uniqueId: { uniqueId }</div>;
}

you can find the plugin here: https://github.com/Taigistal/wp-bug-preview-template-preview-rerenders Also i created a playground here:

please tell me, if you can reproduce the error.

talldan commented 3 months ago

Thanks for the playground link, that's super helpful.

I can reproduce it now just on that screen, so it must be related to the previews. 🤔

It also only happens with the Template previews in the listing, not the Template Parts 🤔 🤔

talldan commented 3 months ago

It's definitely a weird one.

I think preferably try to avoid setting attributes like this in blocks, it can cause some unexpected issues.

If you do need to, you can try something like this:

/**
 * WordPress dependencies
 */
import {
    useBlockProps,
    store as blockEditorStore,
} from '@wordpress/block-editor';
import { useInstanceId } from '@wordpress/compose';
import { useEffect } from '@wordpress/element';
import { useDispatch } from '@wordpress/data';

export default function Edit( { setAttributes, attributes } ) {
    const { uniqueId } = attributes;
    const { __unstableMarkNextChangeAsNotPersistent } =
        useDispatch( blockEditorStore );

    const instanceId = useInstanceId( Edit );

    useEffect( () => {
        if ( ! uniqueId ) {
            __unstableMarkNextChangeAsNotPersistent();
            setAttributes( {
                uniqueId: instanceId,
            } );
        }
    }, [ instanceId, uniqueId ] );

    return <div { ...useBlockProps() }>uniqueId: { uniqueId }</div>;
}

I'm guessing you only need to set the uniqueId when it doesn't exist already, because otherwise you're changing the id quite a lot. If you need to setAttributes in an effect, I'd also suggest using __unstableMarkNextChangeAsNotPersistent. While it's not a stable Gutenberg API, it helps to avoid unwanted undo levels in the editor.

Also fair warning that useInstanceId is not really the right tool for the job here. It won't create a uniqueId the way you think it will. Each time the editor loads it starts counting from zero again, so I think you can have duplicate ids if insert this block in two different posts and then copy it from one into the other. It'd be better to use something that generates a uuid.

silaskoehler commented 2 months ago

In my case i needed a unique Id for each Instance of my block so i generated one with the package uuid. My problem then was that this Id is not unique if i duplicate the block.

I found a similar case here in the query block. I'm not sure if i understand why the Id from useInstanceId isn't unique anyway? Has it just to be unique for its innerBlocks?

Is it bad practice to work with a unique id for each block instance anyway? Should you avoid it when ever you can?

Is there an easier way to make sure that i have a unique Id for each block instance? Is a custom store the solution? So that I assign IDs and check for duplicates in the store? That seems a bit too complicated to me, doesn't it?

So many questions 😅. I hope not too many stupid questions. Thank you for your thoughts.

silaskoehler commented 2 months ago

An addition: Now i realize that the solution above (with __unstableMarkNextChangeAsNotPersistent) only works when i set the attribute if it is false

if ( ! uniqueId ) {

but not if i set it if it not the same as the instanceId.

if ( !uniqueId || uniqueId !== instanceId ) {

But that's what i need to get a unique Id when duplicating the block.

talldan commented 2 months ago

I found a similar case here in the query block.

That's interesting. I wouldn't be surprised if that causes some bugs. It looks like it's been there since the inception of the query block (#22199)

I'm not sure if i understand why the Id from useInstanceId isn't unique anyway?

useInstanceId is suitable for what I'd call 'in memory' ids, but not ids that are saved.

In the block editor, useInstanceId is mostly used in the UI that makes up the editor chrome, often for the html id of form elements (e.g. in the block inspector). This HTML isn't saved anywhere, each time you load the editor the UI is created from scratch, and the ids are generated anew. There will never be any clashes because the counter is correct within a session (the html is fully controlled by react and useInstanceId when re-rendered).

The problem with using it for a block attribute is that ids end up being saved, and that means the id is now being used somewhere outside the control of useInstanceId. You can more easily get clashes because the counter resets to 0 (or maybe it starts at 1, not sure) each time you load the editor, so the generated ids are often in the same range.

UUIDs are a better solution since there's higher entropy, but still not perfect, there's the issue with duplicating blocks, or a pattern with an id within it being used twice in the same post.

Using dynamic rendering (A PHP callback for your block) is probably the best current solution. The HTML Tag Processor is one way to enhance a block's HTML.

The general problem of unique ids has been discussed a bit in https://github.com/WordPress/gutenberg/issues/17246, and unfortunately there haven't been any solutions (though I do have some new ideas).

silaskoehler commented 2 months ago

Thanks for your helpful explanation!

In my specific case i use filters to add responsive breakpoint settings to the core/columns block. For that i need the unique Id. My new solution is to use the Instance ID in the editor and a uuid generated by wp_generate_uuid4 inside the render_block filter. I dont save the Id to the attributes anymore. It works.

So one last question i think:

I use useInstanceId in both filters here and it is the same Id per Block. But why? Are the objects BlockListBlock and BlockEdit identical? is useInstanceId linked to the block in other ways then the object i put into the function?

// editor.BlockListBlock
const withClientIdClassName = createHigherOrderComponent(
    ( BlockListBlock ) => {
        return ( props ) => {
            const instanceId = useInstanceId( BlockListBlock );

            // ... 
        };
    },
    'withClientIdClassName'
);

addFilter(
    'editor.BlockListBlock',
    'luther-responsive-columns/with-client-id-class-name',
    withClientIdClassName
);
// editor.BlockEdit
const LutherResponsiveColumnsBlockEdit = createHigherOrderComponent(
    ( BlockEdit ) => {
        return ( props ) => {
            const instanceId = useInstanceId( BlockEdit );

            // ...
        }
    },
    'withInspectorControl'
);

addFilter(
    'editor.BlockEdit',
    'luther-responsive-columns/with-inspector-controls',
    LutherResponsiveColumnsBlockEdit
);
talldan commented 2 months ago

Are the objects BlockListBlock and BlockEdit identical?

I think that's usually the case now, yes. Older versions of blocks (apiVersion of 1) used to have an automatically added wrapper, which the BlockListBlock filter could be used to modify. For newer blocks that don't have that the BlockListBlock filter seems to be quite duplicative of BlockEdit.