SlotFill: remove explicit rerender from portal version #67471
Conversation
jsnajdr
added
[Type] Code Quality
|
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. |
| @@ -20,18 +19,15 @@ import type { FillComponentProps } from '../types'; | |||
| export default function Fill( { name, children }: FillComponentProps ) { | |||
| const registry = useContext( SlotFillContext ); | |||
| const slot = useObservableValue( registry.slots, name ); | |||
| const [ , rerender ] = useReducer( () => [], [] ); | |||
| const ref = useRef( { rerender } ); | |||
| const instanceRef = useRef( {} ); | |||
There was a problem hiding this comment.
The rerender API goes away and the fill now registers only an unique instance object. I'm renaming it from ref to instance when registering, something that @tyxla asked for in one of the previous PRs.
| }; | ||
| const instance = instanceRef.current; | ||
| registry.registerFill( name, instance ); | ||
| return () => registry.unregisterFill( name, instance ); |
There was a problem hiding this comment.
This effect stays the same except renaming the ref variable to instanceRef etc.
| ref: ref || slot?.ref, | ||
| fillProps: fillProps || slot?.fillProps || {}, | ||
| } ); | ||
| slots.set( name, { ref, fillProps } ); |
There was a problem hiding this comment.
A drive-by change removing redundant code. All the registerSlot calls always include both the ref and fillProps values. Inheriting them from the previous slot value has no effect.
| // Force update fills. | ||
| slotFills.forEach( ( fill ) => fill.rerender() ); | ||
| } | ||
| slots.set( name, { ref, fillProps } ); |
There was a problem hiding this comment.
This is the key change in this PR! Instead of silently updating a field with slots.get( name ).fillProps = fillProps, we call slots.set. That sets a new value and triggers calling all observableMap listeners, leading to automatic rerendering of all fills.
There was a problem hiding this comment.
Much clearer and more concise, looks great 🚀
| }, [ registerSlot, unregisterSlot, name ] ); | ||
| registry.registerSlot( name, ref, fillPropsRef.current ); | ||
| return () => registry.unregisterSlot( name, ref ); | ||
| }, [ registry, name ] ); |
There was a problem hiding this comment.
A mere style change: don't destructure fields of registry. Simplifies the hook dependencies.
| @@ -20,18 +19,15 @@ import type { FillComponentProps } from '../types'; | |||
| export default function Fill( { name, children }: FillComponentProps ) { | |||
| const registry = useContext( SlotFillContext ); | |||
| const slot = useObservableValue( registry.slots, name ); | |||
| const [ , rerender ] = useReducer( () => [], [] ); | |||
| const ref = useRef( { rerender } ); | |||
| const instanceRef = useRef( {} ); | |||
| ref: ref || slot?.ref, | ||
| fillProps: fillProps || slot?.fillProps || {}, | ||
| } ); | ||
| slots.set( name, { ref, fillProps } ); |
| // Force update fills. | ||
| slotFills.forEach( ( fill ) => fill.rerender() ); | ||
| } | ||
| slots.set( name, { ref, fillProps } ); |
There was a problem hiding this comment.
Much clearer and more concise, looks great 🚀
|
Flaky tests detected in efbd839. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12121082461
|
Simplifies the
SlotFillimplementation (the portal "bubbles virtually" version by removing explicitrerenderAPI and its calls fromFill. After this patch the rerender happens automatically when the slot in theslotsobservableMapis changed with theslots.set( name, ... )call.How to test:
All unit tests and e2e tests should pass.