-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
SlotFill: rewrite base Slot to functional, unify rerenderable refs #67153
Conversation
setState( {} ); | ||
} | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is extracted to a separate module because the useForceUpdate
hook is used by two components now. Also with React 18 we no longer need mountedRef
because calling setState
after unmount is no longer a problem.
return () => { | ||
registry.unregisterFill( name, ref ); | ||
registry.unregisterFill( name, refValue ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of registering the ref
object I'm registering directly the Rerenderable
value. Which is also constant.
@@ -70,7 +70,7 @@ function createSlotRegistry(): SlotFillBubblesVirtuallyContext { | |||
const slotFills = fills.get( name ); | |||
if ( slotFills ) { | |||
// Force update fills. | |||
slotFills.forEach( ( fill ) => fill.current.rerender() ); | |||
slotFills.forEach( ( fill ) => fill.rerender() ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a benefit of registering the Rerenderable
value directly: avoids the .current
indirection when calling.
@@ -29,7 +29,7 @@ export default function Fill( { name, children }: FillComponentProps ) { | |||
useLayoutEffect( () => { | |||
ref.current.children = children; | |||
if ( slot ) { | |||
slot.forceUpdate(); | |||
slot.rerender(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forceUpdate
was previously a method of the component class, now slot
is the Rerenderable
instance.
unregisterSlot: ( name: SlotKey, ref: SlotRef ) => void; | ||
updateSlot: ( name: SlotKey, ref: SlotRef, fillProps: FillProps ) => void; | ||
registerFill: ( name: SlotKey, ref: Rerenderable ) => void; | ||
unregisterFill: ( name: SlotKey, ref: Rerenderable ) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the SlotRef
type name, code that uses it is much more tidy.
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. |
const [ , setState ] = useState( {} ); | ||
return useCallback( () => setState( {} ), [] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also seen useReducer
used for a similar pattern, removing the need for useCallback
.
Example from memory: I haven't actually tested it.
const [ , forceUpdate ] = useReducer( () => {}, {} );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I implemented this and removed the useForceUpdate
hook completely because the implementation is now so trivial that it can be easily inlined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @jsnajdr!
- Refactoring/cleanup looks good to me ✅
- Smoke-tested the editors and couldn't spot any errors.
It would be nice to get a second opinion, just in case, but it's not a blocker :)
P.S. Hiding whitespace makes the core view much more accessible.
I'm actually taking a look right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and tests well 👍
Nice refactor, looks so much cleaner 🚀
registerFill: ( name: SlotKey, ref: Rerenderable ) => void; | ||
unregisterFill: ( name: SlotKey, ref: Rerenderable ) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rename the ref
to instance
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have ideas for a few additional cleanups, I'll use your suggestions there.
I'm considering merging the "base" and "bubbles-virtually" implementations into one. Both continue to be useful, as they differ fundamentally in how they pass React context down the tree. And the current SlotFillProvider
renders two context providers nested, and the current Fill
renders both types of fills at once. So, both implementations are kind of merged already, but in a clumsy way.
@@ -90,7 +81,7 @@ function createSlotRegistry(): BaseSlotFillContext { | |||
const slot = getSlot( name ); | |||
|
|||
if ( slot ) { | |||
slot.forceUpdate(); | |||
slot.rerender(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also rename the internal forceUpdateSlot
function to rerenderSlot
or something for consistency?
Thanks for all the quality improvements, @jsnajdr ! |
This is the last
SlotFill
refactor that I originally included in #67000 and later decided to merge one-by-one.The main outcome is refactoring the (base)
Slot
component from a class component to functional. ThecomponentDidMount/DidUpdate
code is converted to auseEffect
. The render code is the same, it just moved from therender()
method to the functional component body.The class version registered its own instance, i.e.,
this
, to the slot registry. But functional components don't have instances likethis
. That's why I'm creating aRenderable
interface with arerender
method, storing a constant per-instance value to a ref, and registering this interface instead.The values in the context that were previously of type
Component< BaseSlotComponentProps >
are nowRerenderable
.It turns out that exactly the same
Renderable
interface is already used by bubbles-virtuallyFill
, so I'm reusing the same code (useForceUpdate
).Last minor update I did is renaming the
SlotFillBubblesVirtuallySlotRef
toSlotRef
, to make the name shorter and prevent the code that uses it from having too many linebreaks.