useBlockRefs: use more efficient lookup map, use uSES #60945
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsnajdr
added
[Type] Code Quality
|
Size Change: -10 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
ellatrix
reviewed
Apr 22, 2024
| map.set( id, setForId ); | ||
| } | ||
| setForId.add( value ); | ||
| } |
There was a problem hiding this comment.
Any reason to not reuse the same logic from #60879?
There was a problem hiding this comment.
The situation is very similar, but not entirely identical. Here, when adding refs to the map, we are merely registering them, without firing listeners. It's only when the ref callback actually sets the ref.current when the listeners are fired.
Maybe it's possible to merge the two implementations, but I'd rather do it in steps.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ellatrix
reviewed
Apr 22, 2024
|
|
||
| function getRefElement( refs, clientId ) { | ||
| // Multiple refs may be created for a single block. Find the | ||
| // first that has an element set. |
There was a problem hiding this comment.
I don't remember why there were multiple refs 😬
There was a problem hiding this comment.
There are multiple refs if you call useBlockRefProvider( clientId ) twice for the same clientId:
const refA = useBlockRefProvider( clientId );
return <div ref={ refA } />;
// somewhere else, with same clientId but different element
const refB = useBlockRefProvider( clientId );
return <div ref={ refB } />;The it registers two refs for the same clientId, and useBlockRef/useBlockElement will return the one that's non-null.
I also don't know if this ever happens 🙂
There was a problem hiding this comment.
I wonder if we should just error if we try to call it twice with the same clientId
There was a problem hiding this comment.
Yes, ignoring the second call and printing a warning would certainly simplify the code. Then we can store just one element instead of a Set.
ellatrix
reviewed
Apr 22, 2024
| return () => deleteFromMap( refs, clientId, ref ); | ||
| }, [ refs, clientId ] ); | ||
|
|
||
| return useCallback( |
There was a problem hiding this comment.
Can't we reuse the same callback for the layout effect as well? The benefit of useRefEffect is that you can pass a cleanup function just like useLayoutEffect, so I think we could add/remove to/from the map as well in addition to callListeners?
There was a problem hiding this comment.
What do you mean by reuse?
I looked more closely at the lifecycle of this hook and some things are suspicious:
- On mount, the DOM element is created, and the ref callback called. This callback sets the
ref.currentand calls listeners. But at this moment, the effect that adds therefto the map hasn't yet run, so the listeners don't see the element yet!useBlockElement( clientId )hooks that started observingclientIdbefore it was mounted will getnullas return value. That looks bad! Intrunkthis works ok because the element is set directly with localsetElementstate setter, the map is not used. Only this branch is broken. - Now the effect that calls
addToMapruns. But there's no calling of listeners. ObservinguseBlockElementhooks won't be notified. - What if, during the lifecycle of the component, the reffed element changes? Consider code like this:
When the element changes like this, the
const ref = useBlockRefProvider( clientId ); return variant ? <b ref={ ref } /> : <i ref={ ref }>;
refcallback will be first called withnull. But our callback ignoresnullvalues, so it will do nothing. Then therefcallback is called with the new element. That updates theref.currentand notifies the listeners, and they will see the new element. That's great. - When the component is unmounting, first the
refcallback is called withnull. Nothing happens, listeners are not notified. - Then the cleanup callback of the effect is called and the
refis removed from the map. - If a component that calls
useBlockElementis rerendered for a random reason, then i) intrunkit will return the element that no longer exists, because it's stored in the hook's local state; ii) in this branch it will returnnullbecause thegetValuefunction foruSESwon't find the map record. In both branches the hook won't be notified about the element change, we need to wait for a random rerender triggered by something else.
To conclude, this PR still needs more testing and more work.
There was a problem hiding this comment.
That's what I meant here: addToMap should move to the ref callback, right before callListeners? The ref callback will also be called when then a ref is attached to a newly mounted component (your snipper), so the ref will be re-added and listeners called again.
There was a problem hiding this comment.
addToMapshould move to the ref callback
The disadvantage of this is that when the element changes, the listeners will get two notifications: one about null element and one about the new element. While ideally they should be notified only about the new element. But it's an edge case anyway, and it's not harmful, just slightly inefficient.
ellatrix
approved these changes
Apr 22, 2024
ellatrix
reviewed
Apr 23, 2024
packages/block-editor/src/components/block-list/use-block-props/use-block-refs.js
Show resolved
Hide resolved
jsnajdr
force-pushed
the
rewrite/use-refs
branch
from
e3bf6ca to
76082e5
Compare
|
@ellatrix I pushed a 76082e5 commit which radically simplifies the implementation 🙂 It uses the
If you mount multiple
const { get } = useSelect( 'store' );
function onClick() {
const el = get();
}in the sense that it's just a non-reactive getter, and the getter is hidden in the TODO: decide whether |
jsnajdr
force-pushed
the
rewrite/use-refs
branch
from
76082e5 to
4c48dcb
Compare
I agree
IMHO, we should err on the side of caution with these new APIs and leave them private for now until we find more use cases. That said, for this one it's easier to deprecate and create a new one if needed, but I still think it's nice to find a few more other uses? |
ellatrix
approved these changes
Apr 23, 2024
jsnajdr
force-pushed
the
rewrite/use-refs
branch
from
4c48dcb to
3e74736
Compare
After trying to make the new The main reason because private APIs have trouble with types. I need to export an import type { ObservableMap } from '@wordpress/compose';
type SlotFillContext = {
slots: ObservableMap<SlotKey, SlotValue>;
};But the type can't be private, only the JS exports can! Additionally, the On the other hand, both I pushed a commit that improves the JSDoc documentation, and I'm ready to merge. |
|
Sounds good! |
huubl
pushed a commit
to huubl/gutenberg
that referenced
this pull request
Apr 26, 2024
* useBlockRefs: use more efficient lookup map, use uSES * Rewrite block refs with observableMap, which moves to compose * Improve docs * Add changelog entry
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updates the
useBlockRefsimplementation in two ways:refsandcallbackswas effectively an array, always searched by linear traversal. The.getmethod was never used! Now it's a real map indexed byclientIdand storingSetof refs or listeners.useSyncExternalStoreinstead of localuseStateto reactively observe ref elements being added and removed.Let's see if this has any measurable performance impact.