Skip to content
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

useBlockElement: return null until ref callback has time to clean up the old element #63565

Merged
merged 1 commit into from
Jul 16, 2024
Merged

useBlockElement: return null until ref callback has time to clean up the old element #63565

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 19 additions & 3 deletions packages/block-editor/src/components/block-list/use-block-props/use-block-refs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
/**
* WordPress dependencies
*/
import { useContext, useMemo, useRef } from '@wordpress/element';
import { useRefEffect, useObservableValue } from '@wordpress/compose';
import {
useContext,
useMemo,
useRef,
useState,
useLayoutEffect,
} from '@wordpress/element';
import { useRefEffect } from '@wordpress/compose';

/**
* Internal dependencies
Expand Down Expand Up @@ -67,7 +73,17 @@ function useBlockRef( clientId ) {
*/
function useBlockElement( clientId ) {
const { refsMap } = useContext( BlockRefs );
return useObservableValue( refsMap, clientId ) ?? null;
const [ blockElement, setBlockElement ] = useState( null );
// Delay setting the resulting `blockElement` until an effect. If the block element
// changes (i.e., the block is unmounted and re-mounted), this allows enough time
// for the ref callbacks to clean up the old element and set the new one.
useLayoutEffect( () => {
setBlockElement( refsMap.get( clientId ) );
return refsMap.subscribe( clientId, () =>
setBlockElement( refsMap.get( clientId ) )
Copy link
Member

Choose a reason for hiding this comment

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

On unmount, shouldn't it be set to null?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was considering this, but I think it's not needed, it wouldn't have any effect. On unmount, the component is going away, and updating its local state is not needed, because nobody is going to read it.

But maybe we can add it for consistency sake. So that the behavior is indistinguishable from the classic "store ref as state" pattern:

const [ el, setEl ] = useState( null );
return <div ref={ setEl } />;

In this case the setEl will be called with null on unmount.

useBlockRefs is basically the same pattern, store an element ref into state, but the ref and the state are in different components, and the store in the provider serves as a teleportation machine between them.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be possible to for a block to disappear, while another component is using useBlockElement, which would then still point to an unmounted DOM node?

Copy link
Member Author

Choose a reason for hiding this comment

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

Before the block disappears, the ref callback is called and sets the ref to null. Then the observable map calls its listeners and inside the useBlockElement hook, the setBlockElement inside the listener is called, and sets the local state to null. Therefore, all the references to the unmounted DOM node are cleared.

Copy link
Member Author

Choose a reason for hiding this comment

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

This issue makes me thinking that our current API for reading the elements is not very good. Ideally it should work exactly like element refs. Like this:

const blockElRef = useRef(); // can also be a ref callback
useBlockElementRef( clientId, blockElRef );

The blockElRef would be set and unset in real time as the target element changes. The point is that it is identical to how refs to local elements work:

const blockElRef = useRef(); // can also be a ref callback
return <div ref={ blockElRef } />;

Exactly the same thing, only in one case the element is local and in the other it's teleported from another location.

This is much better than the current useBlockRef hook that returns a ref with a synthetic ref.current getter.

The useBlockElementRef primitive can be used to very easily implement useBlockElement that returns the element and triggers a rerender on change:

function useBlockElement( clientId ) {
  const [ el, setEl ] = useState( null );
  useBlockElementRef( clientId, setEl );
  return el;
}

This implementation also never has the bug with returning stale unmounted elements.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, sorry, I misread this as a change to the hook inside the block rather than a change to the hook in the consumer.

Yes, that alternative seems good to me. We should try it :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented in #63799 🙂

);
}, [ refsMap, clientId ] );
return blockElement;
}

export { useBlockRef as __unstableUseBlockRef };
Expand Down
Loading