SlotFill: use observableMap everywhere, remove manual rerendering #67400

jsnajdr · 2024-11-28T15:47:26Z

Rewrites the base (bubblesVirtually=false, non-portal) version of SlotFill to use observableMap for storing slots and fills. This removes the need for all manual rerenders. The observable maps will notify their consumers automatically whenever something interesting and rerender-worthy happens.

With this PR, both variants of SlotFill become implemented in a very similar way.

There are observable maps for slots and fills. Each is registering the same base info (instance id) and some additional implementation-specific data:

base fill stores children that the slot should render
portal slot stores fillProps that the fill uses during rendering, and the DOM element into which the fill is rendered

The context has methods for registering/unregistering slots and fills, and additional methods:

base context has updateFill method that updates the children in registry on Fill rerender
portal context has updateSlot method that updates the element/fillProps in registry on Slot rerender

github-actions · 2024-11-30T09:11:59Z

Flaky tests detected in 2f69488.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12123240256
📝 Reported issues:

[Flaky Test] submenu click on the arrow interactions #61884 in /test/e2e/specs/editor/blocks/navigation-frontend-interactivity.spec.js

github-actions · 2024-12-05T12:22:54Z

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

jsnajdr · 2024-12-05T12:24:25Z

packages/components/src/slot-fill/context.ts

 	registerSlot: () => {},
 	unregisterSlot: () => {},
 	registerFill: () => {},
 	unregisterFill: () => {},
-	getSlot: () => undefined,
-	getFills: () => [],
-	subscribe: () => () => {},


getSlot and getFills are replaced by simple map lookups: slots.get( name ). The subscribe method is also a method of the observable map now.

jsnajdr · 2024-12-05T12:26:13Z

packages/components/src/slot-fill/fill.ts

 import type { FillComponentProps } from './types';

 export default function Fill( { name, children }: FillComponentProps ) {
 	const registry = useContext( SlotFillContext );
-	const slot = useSlot( name );


The Fill no longer needs the slot value because it no longer calls slot.rerender(). Instead, it updates the value in the fills map (updateFill call) and the relevant slot rerenders automatically because it listens for fills changes.

The fact that there were two separate useSlot hooks was definitely confusing. Glad to see that the "non-bubblesVirtually" version is going away

The removed useSlot hook was essentially a useSyncExternalStore call. Now when the slot registry uses observableMap, it can be fully replaced by useObservableMap, which is also a thin wrapper around useSyncExternalStore.

jsnajdr · 2024-12-05T12:27:23Z

packages/components/src/slot-fill/fill.ts

-	const ref = useRef( {
-		name,
-		children,
-	} );


Here we used to store name in the fills map but that was redundant. name is a key in the map. It doesn't need to be stored also in the value.

jsnajdr · 2024-12-05T12:30:03Z

packages/components/src/slot-fill/fill.ts

-		if ( slot ) {
-			slot.rerender();
-		}
-	}, [ slot, children ] );


This code used to store the new value of children into a ref.current. But that's a "silent" mutation that doesn't trigger any listeners. That's why the code had to do a manual slot.rerender() call. The new code updates the children using the observableMap.set method (hidden inside the updateFill method). Because the slot listens for fills updates via useObservableValue( fills, name ), it gets notified and rerenders automatically.

jsnajdr · 2024-12-05T12:34:20Z

packages/components/src/slot-fill/provider.tsx

-		if ( previousSlot ) {
-			previousSlot.rerender();
-		}
-	}


All this code collapses into a simple slots.set call:

triggerListeners is built-in in slots.set.

the forceUpdateSlot logic is built-in into useSyncExternalStore. It handles the situation when the store is updated after the consumer component is rendered and before it subscribes to updates in an effect.

previousSlot.rerender() happens automatically because the old slot calls the currentSlot = useObservableValue( slots, name ) hook and gets notified when a new slot is registered under the same name.

I love how elegant this is ! I wonder if we should document it somewhere, though, since otherwise there would be a lot of implicit knowledge required to fully understand the component.

Now when we use an observable store for slots and observe it with useSyncExternalStore, there isn't really much to document. You just use a native hook and it does all the work for you.

The previous implementation was convoluted because:

the Slot component was not subscribed to changes in the slots registry. Only Fill was. That's why functions in the provider had to manually call slot.rerender() on every relevant event. That's not replaced with useSyncExternalStore/useObservableMap with automatically rerenders the Slot component whenever the entry in slots changes.

it manually handled the situation when store was updated between a render (reading a value from store) and the effect that subscribed to updates. Then the update was missed. Before useSyncExternalStore, you needed to be aware of such gotchas. For example, the Redux connect implementation had to handle this, too. But today we have a native hook that does it all for us. It handles it in this code:

https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberHooks.js#L1900-L1907

jsnajdr · 2024-12-05T12:36:40Z

packages/components/src/slot-fill/provider.tsx

-		// Fills should only be returned for the current instance of the slot
-		// in which they occupy.
-		if ( slots[ name ] !== slotInstance ) {
-			return [];


This logic where the fills are [] for a slot instance that has been overwritten in the store by a new instance, it has moved into the Slot component.

jsnajdr · 2024-12-05T12:37:52Z

packages/components/src/slot-fill/slot.tsx

+			key: childKey,
+		} );
+	} );
+}


This is a simple refactor where the addKeysToChildren function has been extracted from the big Slot component. Without any change.

jsnajdr · 2024-12-05T12:39:07Z

packages/components/src/slot-fill/types.ts

@@ -84,6 +84,10 @@ export type SlotComponentProps =
 			style?: never;
 	  } );

+export type FillChildren =
+	| ReactNode
+	| ( ( fillProps: FillProps ) => ReactNode );


Extracted this as a separate type because it's used in the fills map.

ciampo

Thank you for working on this! I find it exciting to see how, little by little, SlotFill code is getting simpler, and the two implementations are getting closer to each other — I'm hoping we will be able to merge them into one single implementation at some point.

Apart from smoke testing Storybook and the editor, I found it hard to test the changes. I wonder if, as a follow-up, we should also spend some time to bolster the suite of unit tests (which is weirdly still written in JS and not in TS).

ciampo · 2024-12-11T12:48:07Z

packages/components/src/slot-fill/fill.ts

-			slot.rerender();
-		}
-	}, [ slot, children ] );
+		registry.updateFill( name, instanceRef.current, childrenRef.current );


Is it still necessary to wrap the updateFill call inside a useLayoutEffect, especially given that the hook doesn't have a dependency list anymore?

Yes, the purpose of the updateFill is to store the latest children into the fills map in the registry on every rerender of the Fill. It might have been rerendered with new children.

It's an effect because it's a side effect. We want to "commit" the updateFill change only when the output render is really committed to the DOM. That's why it can't be called directly during render.

ciampo · 2024-12-11T12:52:16Z

packages/components/src/slot-fill/provider.tsx

-		if ( previousSlot ) {
-			previousSlot.rerender();
-		}
-	}


I love how elegant this is ! I wonder if we should document it somewhere, though, since otherwise there would be a lot of implicit knowledge required to fully understand the component.

ciampo · 2024-12-11T12:54:47Z

packages/components/src/slot-fill/provider.tsx

+	const slots = observableMap< SlotKey, BaseSlotInstance >();
+	const fills = observableMap<
+		SlotKey,
+		{ instance: FillInstance; children: FillChildren }[]


Any reason why here (and in types.ts) we stopped using FillComponentProps ?

FillComponentProps also includes the name. But we don't need name in the value in the map, because name is already the key. Also FillComponentsProps don't include the instance field.

I should give some nice name to the { instance, children } type. It's similar to FillComponentProps, but not identical.

Mamaduka · 2024-12-11T13:27:23Z

I wonder if, as a follow-up, we should also spend some time to bolster the suite of unit tests (which is weirdly still written in JS and not in TS).

I think that would be wonderful. A good unit test coverage for SlotFills is crucial.

jsnajdr · 2024-12-13T11:13:43Z

I think that would be wonderful. A good unit test coverage for SlotFills is crucial.

The existing tests for SlotFill are not bad. They caught one mistake I made in this PR.

And as all the complications are being removed and replaced with useSyncExternalStore and useObservableMap, there is less motivation to do unit testing. The more subtle unit tests would be in fact testing the native useSyncExternalStore hook, which obviously works 🙂

ciampo

I personally don't have more feedback at this point, and I think we could go ahead and merge this PR (this potential change is non-blocking IMO).

The existing tests for SlotFill are not bad. They caught one mistake I made in this PR.

And as all the complications are being removed and replaced with useSyncExternalStore and useObservableMap, there is less motivation to do unit testing. The more subtle unit tests would be in fact testing the native useSyncExternalStore hook, which obviously works 🙂

I'd still like to have the unit tests refactored to TS as a nice follow-up

jsnajdr added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Nov 28, 2024

jsnajdr self-assigned this Nov 28, 2024

jsnajdr force-pushed the update/slotfill-use-observable-map branch from 56eb04b to 3956f58 Compare November 30, 2024 08:51

jsnajdr force-pushed the update/slotfill-use-observable-map branch 3 times, most recently from c0cae70 to f2321f4 Compare December 5, 2024 12:12

jsnajdr marked this pull request as ready for review December 5, 2024 12:22

jsnajdr requested a review from ajitbohra as a code owner December 5, 2024 12:22

jsnajdr requested review from Mamaduka, youknowriad, tyxla, mirka and ciampo December 5, 2024 12:22

jsnajdr commented Dec 5, 2024

View reviewed changes

ciampo reviewed Dec 11, 2024

View reviewed changes

ciampo approved these changes Dec 13, 2024

View reviewed changes

jsnajdr added 2 commits December 13, 2024 20:35

SlotFill: use observableMap in base version

6bc74bc

Add changelog entry

441ca28

jsnajdr force-pushed the update/slotfill-use-observable-map branch from f2321f4 to 441ca28 Compare December 13, 2024 19:41

jsnajdr merged commit 8066995 into trunk Dec 13, 2024
63 checks passed

jsnajdr deleted the update/slotfill-use-observable-map branch December 13, 2024 21:23

github-actions bot added this to the Gutenberg 20.0 milestone Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SlotFill: use observableMap everywhere, remove manual rerendering #67400

SlotFill: use observableMap everywhere, remove manual rerendering #67400

jsnajdr commented Nov 28, 2024 •

edited

Loading

github-actions bot commented Nov 30, 2024 •

edited

Loading

github-actions bot commented Dec 5, 2024 •

edited

Loading

jsnajdr Dec 5, 2024

jsnajdr Dec 5, 2024

ciampo Dec 11, 2024

jsnajdr Dec 13, 2024

jsnajdr Dec 5, 2024

jsnajdr Dec 5, 2024

jsnajdr Dec 5, 2024

ciampo Dec 11, 2024

jsnajdr Dec 13, 2024

jsnajdr Dec 5, 2024

jsnajdr Dec 5, 2024

jsnajdr Dec 5, 2024

ciampo left a comment

ciampo Dec 11, 2024

jsnajdr Dec 13, 2024

ciampo Dec 11, 2024

ciampo Dec 11, 2024

jsnajdr Dec 13, 2024

Mamaduka commented Dec 11, 2024

jsnajdr commented Dec 13, 2024

ciampo left a comment •

edited

Loading

SlotFill: use observableMap everywhere, remove manual rerendering #67400

SlotFill: use observableMap everywhere, remove manual rerendering #67400

Conversation

jsnajdr commented Nov 28, 2024 • edited Loading

github-actions bot commented Nov 30, 2024 • edited Loading

github-actions bot commented Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mamaduka commented Dec 11, 2024

jsnajdr commented Dec 13, 2024

ciampo left a comment • edited Loading

Choose a reason for hiding this comment

jsnajdr commented Nov 28, 2024 •

edited

Loading

github-actions bot commented Nov 30, 2024 •

edited

Loading

github-actions bot commented Dec 5, 2024 •

edited

Loading

ciampo left a comment •

edited

Loading