Block Editor: use shallow memo for prioritized inserter blocks #65737
Conversation
tyxla
added
[Type] Enhancement
tyxla
requested review from
Mamaduka,
scruffian,
jorgefilipecosta and
a team
|
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. |
|
Size Change: +344 B (+0.02%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
| const _prioritizedInserterBlocks = useMemo( | ||
| () => prioritizedInserterBlocks, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const _prioritizedInserterBlocks = useShallowMemo( |
There was a problem hiding this comment.
It's a good idea to use useShallowMemo here but I think the useShallowMemo utility is not implemented very well. It will do a cascade of setState calls and rerenders. It should be storing the memoized value in a ref instead. Something like:
function useMemoEx( callback, compare ) {
const memoRef = useRef( undefined );
const newValue = callback();
if ( memoRef.current === undefined || ! compare( memoRef.current, newValue ) ) {
memoRef.current = newValue;
}
return memoRef.current;
}I'm pretty sure we already have such a hook (memo with custom compare) defined somewhere.
There was a problem hiding this comment.
That's a good point 👍
I've seen you working on it in another repository.
Feel free to open a PR, I think it can be improved separately.
There was a problem hiding this comment.
@jsnajdr the implementation you're proposing is unsafe because you're reading/writing refs in render, which is forbidden by the React Gods (can mess things up in concurrent rendering trees). When doing render-time computations, you simply can't use refs.
However, many use cases for render-time refs can be replaced by derived state, which is a technique consisting of calling a setState at render time, which causes the render function to instantly re-run synchronously, skipping all effects and other work of the kind. This is fine, as long as the state is guaranteed to settle, otherwise it can result in an endless loop. Can't find it right now, but the technique is officially listed somewhere in the official, updated docs.
Running a component's render function is typically pretty much free, so it'd be rare if this technique is a problem for performance. Re-rendering is a tool, not a danger to avoid.
It'd be another story imo if you found a specific perf problem somewhere that is caused by excessive re-rendering, that can happen but anything earlier than that feels like premature optimization (or even worsening, as sometimes re-rendering is the better option).
That's my 2cents anyway :)
There was a problem hiding this comment.
Some context for the original useShallowMemo - #53943 (comment). We can adopt the usePrevious hook to achieve the same behavior by adding the optional compare as the second argument. But duplication is okay, as usual.
Aside: The usehooks.com has also updated its implementation of usePrevious to use the new pattern.
There was a problem hiding this comment.
I made this about 3 years ago: https://codesandbox.io/p/sandbox/previous-vs-previous-value-t7t3y?file=%2Fsrc%2FApp.tsx
There was a problem hiding this comment.
I don't agree that breaking the rules to prevent renders without a clear performance issue is the pragmatic and less ideological way, I'd argue it's the opposite. That said, we can agree to disagree on that too :P
Just hope I don't have a "told you so" moment a few months down the line. No hard feelings either way!
There was a problem hiding this comment.
In the general case, I do agree that we shouldn't be disobeying the React Gods without a substantial upside, and without a mechanism to catch problems when things start to break. Especially because this seems to be the type of thing where the kinds of bugs that could possibly occur in the future are very subtle.
I'm not privy enough of React internals to assess how much of a special exception this is to the general case. In the general case, just because an official hook adopts an implementation internally right now doesn't necessarily make it safe to imitate in userland. Could they rewrite the internal useMemo implementation without us noticing? Could they add a special safeguard for useMemo that doesn't apply to userland imitations? If there really is no magic and we can confidently answer No to those questions, then sure, maybe this is a special exception. (Unfortunately I cannot confidently answer those questions 😅)
There was a problem hiding this comment.
I think it's helpful to try to really understand why exactly reading/writing refs during render can be dangerous. The metaphors about rules written by gods are counterproductive, and also not completely innocent, because they suggest that we're dealing with some revealed truths that are inaccessible to reason. That's not true.
One example of a bad component is this:
function Foo( { value } ) {
const valueRef = useRef();
valueRef.current = value;
return <button onClick={ () => { console.log( valueRef.current ); } } />
}Here, if you render <Foo value="hello"/>, clicking on the button will print hello. That's good. Now React can start rerendering the tree that contains <Foo />, with another value of the prop, but that render is aborted. It suspends or is interrupted, and the results are thrown away. DOM is not changed and effects are not run. But the valueRef.current = value modification was done and it stays. It's an assignment very similar to calling this.value = ... on a component instance, there is nothing sophisticated about it.
This means that an aborted render of <Foo value="world" /> will actually modify valueRef and clicking on the button will log world. The modification that was not supposed to happen has a material effect on the rendered UI. We can construct many other examples like this where aborted, tentative renders can leak into the active UI. That's why the ref access should be moved into effects, because effects run only when the result of the render was actually commited. Or we should use state, because the state of a committed component is isolated from the state of the tentative render, and the updates are not done directly, but go though a managed queue. Refs are not isolated or managed like this, they are "naked" values.
But not all ref reads/writes are dangerous like this. The React docs contain this example that gives us a first piece of evidence that the rules are not absolute.
function Video() {
const playerRef = useRef(null);
if (playerRef.current === null) {
playerRef.current = new VideoPlayer();
}
// ...Here we read and write a ref during render, but the docs say this is fine!
Normally, writing or reading
ref.currentduring render is not allowed. However, it’s fine in this case because the result is always the same, and the condition only executes during initialization so it’s fully predictable.
Indeed, if a tentative render that's going to be aborted initializes the VideoPlayer, we don't mind, because we would create it the same way anyway. And most of the time the tentative render will find out the VideoPlayer already exists and will do nothing. Nothing bad can happen.
Now let's check if useMemo could also possibly be safe like this. Let's implement useMemo in user space:
function useMemo( create, deps ) {
const prevState = useRef();
if ( prevState.current === undefined || ! areDepsEqual( prevState.current[ 1 ], deps ) ) {
prevState.current = [ create(), deps ];
}
return prevState.current[ 0 ];
}Two things to note here:
- The hook's
createparameter is always capable to produce a valid return value. And it's constructed from "pure" React values: the props, the state, the context. Also thedepsare derived from these "pure" values. - The hook is very skeptical about the value in the
prevStateref. Before using it, it carefully verifies that it's valid, i.e., whether it was constructed with the samedeps. If some bad value leaked into theprevStateref from an aborted render, this will be detected, the value will be ignored, and will be recalculated from the "pure"createfunction.
That means that the useMemo hook is resilient against bad values that leak into it from aborted renders. The only thing that the aborted renders can ever achieve is that they invalidate your memoization and force you to recreate it, but the hook will never return a bad value.
That's why the native useMemo hook is safe, and also why a user-space useMemo or useShallowMemo implementation is also safe if done well.
There was a problem hiding this comment.
@jsnajdr Thanks for the explanation!
There was a problem hiding this comment.
Thanks y'all for the feedback!
I agree with @jsnajdr and will move to ship this, and will keep an eye in the future.
| } | ||
| return prevValue; | ||
| const [ memo ] = useState( createShallowMemo ); | ||
| return memo( value ); |
There was a problem hiding this comment.
There is a side effect here at render time, comparable to reading/writing a ref, which makes this unsafe for concurrency and a future source of tearing.
The only reason this doesn't trigger a React compiler error is that their linter is not accounting for this more obscure way of setting and reading non-stateful values, but it is no different than a ref-based solution and the error applies all the same.
There was a problem hiding this comment.
Reading the discussion on this PR was very insightful thank you @DaniGuardiola, @tyxla, @jsnajdr.
From the functionality point of view this PR was also working as expected 👍
priethor
added
[Type] Code Quality
…ress#65737) * Block Editor: use shallow memo for prioritized inserter blocks * Add single-pass implementation of useShallowMemo --------- Co-authored-by: Jarda Snajdr <[email protected]> Co-authored-by: tyxla <[email protected]> Co-authored-by: jsnajdr <[email protected]> Co-authored-by: DaniGuardiola <[email protected]> Co-authored-by: Mamaduka <[email protected]> Co-authored-by: mirka <[email protected]>
What?
This PR updates the prioritized inserter blocks prop to utilize the
useShallowMemoutility, which considers array length changes.Why?
This was initially motivated by resolving an error for React Compiler (see #61788):
But then, while researching why it was there in the first place, I noticed @ellatrix introduced the memoization in #30311 to reduce extra updates without a stable reference. @jorgefilipecosta took it a step further in #53943 to consider array length changes, but the fix wasn't considered for #50510, which introduced a similar practice for prioritized inserter blocks.
So, in this PR, we're making the same improvement for
prioritizedInserterBlockswhile getting rid of the extraeslint-ignoredirective.How?
We're utilizing
useShallowMemoforprioritizedInserterBlocks.Testing Instructions
Testing Instructions for Keyboard
Same
Screenshots or screencast