Migrate Composite component from reakit to ariakit

[andrewhayward]

What?

As per #53278, we need to move away from reakit to use ariakit instead. This PR tackles part of that, by starting the migration of the Composite component.

Why?

Reakit does not explicitly support React 18, so it is causing peer dependency warnings.
Reakit has already been succeeded by Ariakit, so it will not receive any more updates.

Composite is currently a simple wrapper around reakit, re-exporting its various component parts. It needs to move away from this and export ariakit instead.

How?

This is the first step in migrating Composite away from reakit:

• Refactors usages of reakit to ariakit in @wordpress/components, but without modifying the version exported by the @wordpress/components package
• Temporarily exports the ariakit version of Composite via the private APIs

It does not tackle the following, which will be follow-up tickets:

• Iteratively refactor all usages of the reakit composite components across Gutenberg
• Remove the reakit implementation, and use the ariakit-based version instead, removing the ariakit private API export

Note

Even though Composite is currently exported from the components package as __unstable, it is being used publicly, so we will have to proceed with caution!

Testing Instructions

Despite the API changes, both AlignmentMatrixControl and CircularOptionPicker should continue to work as now.

[andrewhayward]

Hi @jsnajdr, @diegohaz – hoping I can get some ideas. I'm in the process of migrating a Gutenberg component from reakit to ariakit, and I'm having some issues with focus and flaky tests in the latter.

The two test runs below (skipped irrelevant bits) are consecutive, with no changes, and fail in different places. This seems to be because sometimes initial focus is in different places, and not where the component expects it to be. As it's not consistent, I'm figuring it's something in my tests, or me missing something in how ariakit works, rather than the logic itself.

Essentially, there's a three-by-three grid, and by default the focus should be in the center. This always seems to be fine in practice, but in the unit tests sometimes that's true and other times it's in the top left. I wrote the tests against reakit, and it seemed to be fine, so any pointers would be really useful.

 FAIL  packages/components/src/alignment-matrix-control/test/index.tsx
  AlignmentMatrixControl
    Basic rendering
      ✓ should render (56 ms)
      ✕ should be centered by default (66 ms)
    Should change value
      with Mouse
        on cell click
          ...
      with Keyboard
        on arrow press
          ✓ ArrowUp (44 ms)
          ✓ ArrowLeft (36 ms)
          ✓ ArrowDown (40 ms)
          ✕ ArrowRight (36 ms)
        but not at at edge
          ...

 FAIL  packages/components/src/alignment-matrix-control/test/index.tsx
  AlignmentMatrixControl
    Basic rendering
      ✓ should render (57 ms)
      ✕ should be centered by default (64 ms)
    Should change value
      with Mouse
        on cell click
          ...
      with Keyboard
        on arrow press
          ✕ ArrowUp (37 ms)
          ✕ ArrowLeft (39 ms)
          ✕ ArrowDown (40 ms)
          ✓ ArrowRight (37 ms)
        but not at at edge
          ...

[diegohaz]

@andrewhayward If everything works in the browser, but fails in the tests, I'd try to start replacing the following getByRole with findByRole:

gutenberg/packages/components/src/alignment-matrix-control/test/index.tsx

Lines 16 to 18 in 9b5e714

	const getCell = ( name: string ) => {
	return within( getControl() ).getByRole( 'gridcell', { name } );
	};

If that doesn't work, it may be necessary to waitFor something on the screen.

This happens because those unit tests are mostly synchronous, and the actual behavior may happen at a different timing (for example, using queueMicrotask or requestAnimationFrame).

More context: #52133 (comment)

[andrewhayward]

This happens because those unit tests are mostly synchronous, and the actual behavior may happen at a different timing (for example, using queueMicrotask or requestAnimationFrame).

Thanks for the suggestions, @diegohaz. The only way I was able to get the tests to all pass consistently was with three requestAnimationFrames, which feels like a bit of a hack...

const asyncRender = async ( jsx ) => {
	const view = render( jsx );
	await act( async () => {
		await new Promise( requestAnimationFrame );
		await new Promise( requestAnimationFrame );
		await new Promise( requestAnimationFrame );
	} );
	return view;
};

Never a big fan of "magic" like this, but it seems like it's what the underlying implementation needs.

[ciampo]

I think that we can levegra ariakit's internal store logic more than we're currently doing — had a quick attempt at that on my machine and came up with these changes (they may not be prefect, but they may be a step in the right direction?)

Click to expand

diff --git a/packages/components/src/alignment-matrix-control/index.tsx b/packages/components/src/alignment-matrix-control/index.tsx
index 1d8a1a08c4..20f2c4770c 100644
--- a/packages/components/src/alignment-matrix-control/index.tsx
+++ b/packages/components/src/alignment-matrix-control/index.tsx
@@ -8,7 +8,6 @@ import classnames from 'classnames';
  */
 import { __, isRTL } from '@wordpress/i18n';
 import { useInstanceId } from '@wordpress/compose';
-import { useState } from '@wordpress/element';
 
 /**
  * Internal dependencies
@@ -17,7 +16,7 @@ import Cell from './cell';
 import { Composite, CompositeRow, useCompositeStore } from '../composite/v2';
 import { Root, Row } from './styles/alignment-matrix-control-styles';
 import AlignmentMatrixControlIcon from './icon';
-import { GRID, getItemId, normalizeValue } from './utils';
+import { GRID, getItemId, getItemValue } from './utils';
 import type { WordPressComponentProps } from '../ui/context';
 import type {
 	AlignmentMatrixControlProps,
@@ -56,27 +55,30 @@ export function AlignmentMatrixControl( {
 	width = 92,
 	...props
 }: WordPressComponentProps< AlignmentMatrixControlProps, 'div', false > ) {
-	const [ immutableDefaultValue ] = useState(
-		normalizeValue( value ?? defaultValue )
-	);
-	const currentCell = normalizeValue( value ?? immutableDefaultValue );
-
 	const baseId = useInstanceId(
 		AlignmentMatrixControl,
 		'alignment-matrix-control',
 		id
 	);
-	const defaultActiveId = getItemId( baseId, immutableDefaultValue );
-
-	const handleOnChange = ( nextValue: AlignmentMatrixControlValue ) => {
-		onChange( nextValue );
-	};
-
 	const compositeStore = useCompositeStore( {
-		defaultActiveId,
+		defaultActiveId: getItemId( baseId, defaultValue ),
+		activeId: getItemId( baseId, value ),
+		setActiveId: ( nextActiveId ) => {
+			onChange(
+				getItemValue(
+					baseId,
+					nextActiveId as
+						| AlignmentMatrixControlValue
+						| undefined
+						| null
+				)
+			);
+		},
 		rtl: isRTL(),
 	} );
 
+	const activeId = compositeStore.useState( 'activeId' );
+
 	const classes = classnames(
 		'component-alignment-matrix-control',
 		className
@@ -88,7 +90,6 @@ export function AlignmentMatrixControl( {
 			store={ compositeStore }
 			aria-label={ label }
 			as={ Root }
-			id={ baseId }
 			className={ classes }
 			role="grid"
 			size={ width }
@@ -97,7 +98,7 @@ export function AlignmentMatrixControl( {
 				<CompositeRow as={ Row } role="row" key={ index }>
 					{ cells.map( ( cell ) => {
 						const cellId = getItemId( baseId, cell );
-						const isActive = cell === currentCell;
+						const isActive = cellId === activeId;
 
 						return (
 							<Cell
@@ -105,7 +106,6 @@ export function AlignmentMatrixControl( {
 								isActive={ isActive }
 								key={ cell }
 								value={ cell }
-								onFocus={ () => handleOnChange( cell ) }
 							/>
 						);
 					} ) }
diff --git a/packages/components/src/alignment-matrix-control/stories/index.story.tsx b/packages/components/src/alignment-matrix-control/stories/index.story.tsx
index 24b496d1f2..03bec9d92a 100644
--- a/packages/components/src/alignment-matrix-control/stories/index.story.tsx
+++ b/packages/components/src/alignment-matrix-control/stories/index.story.tsx
@@ -6,7 +6,7 @@ import type { Meta, StoryFn } from '@storybook/react';
 /**
  * WordPress dependencies
  */
-import { useEffect, useState } from '@wordpress/element';
+import { useState } from '@wordpress/element';
 import { Icon } from '@wordpress/icons';
 
 /**
@@ -24,10 +24,11 @@ const meta: Meta< typeof AlignmentMatrixControl > = {
 		'AlignmentMatrixControl.Icon': AlignmentMatrixControl.Icon,
 	},
 	argTypes: {
-		onChange: { action: 'onChange', control: { type: null } },
+		onChange: { control: { type: null } },
 		value: { control: { type: null } },
 	},
 	parameters: {
+		actions: { argTypesRegex: '^on.*' },
 		controls: { expanded: true },
 		docs: { canvas: { sourceState: 'shown' } },
 	},
@@ -42,11 +43,6 @@ const Template: StoryFn< typeof AlignmentMatrixControl > = ( {
 	const [ value, setValue ] =
 		useState< AlignmentMatrixControlProps[ 'value' ] >();
 
-	// Convenience handler for Canvas view so changes are reflected
-	useEffect( () => {
-		setValue( defaultValue );
-	}, [ defaultValue ] );
-
 	return (
 		<AlignmentMatrixControl
 			{ ...props }
diff --git a/packages/components/src/alignment-matrix-control/types.ts b/packages/components/src/alignment-matrix-control/types.ts
index 892781234e..3b0378401b 100644
--- a/packages/components/src/alignment-matrix-control/types.ts
+++ b/packages/components/src/alignment-matrix-control/types.ts
@@ -27,11 +27,13 @@ export type AlignmentMatrixControlProps = {
 	/**
 	 * The current alignment value.
 	 */
-	value?: AlignmentMatrixControlValue;
+	value?: AlignmentMatrixControlValue | null;
 	/**
 	 * A function that receives the updated alignment value.
 	 */
-	onChange?: ( newValue: AlignmentMatrixControlValue ) => void;
+	onChange?: (
+		newValue: AlignmentMatrixControlValue | undefined | null
+	) => void;
 	/**
 	 * If provided, sets the width of the control.
 	 *
diff --git a/packages/components/src/alignment-matrix-control/utils.tsx b/packages/components/src/alignment-matrix-control/utils.tsx
index 7f0e2b08e3..8159bf2b89 100644
--- a/packages/components/src/alignment-matrix-control/utils.tsx
+++ b/packages/components/src/alignment-matrix-control/utils.tsx
@@ -37,7 +37,9 @@ export const ALIGNMENTS = GRID.flat();
  *
  * @return The normalized value
  */
-export function normalizeValue( value: AlignmentMatrixControlValue ) {
+export function normalizeValue(
+	value: AlignmentMatrixControlValue | undefined | null
+) {
 	return value === 'center' ? 'center center' : value;
 }
 
@@ -48,11 +50,12 @@ export function normalizeValue( value: AlignmentMatrixControlValue ) {
  *
  * @return The parsed value.
  */
-export function transformValue( value: AlignmentMatrixControlValue ) {
-	return normalizeValue( value ).replace(
-		'-',
-		' '
-	) as AlignmentMatrixControlValue;
+export function transformValue(
+	value: AlignmentMatrixControlValue | undefined | null
+) {
+	return normalizeValue( value )?.replace( '-', ' ' ) as
+		| AlignmentMatrixControlValue
+		| undefined;
 }
 
 /**
@@ -65,9 +68,14 @@ export function transformValue( value: AlignmentMatrixControlValue ) {
  */
 export function getItemId(
 	prefixId: string,
-	value: AlignmentMatrixControlValue
+	value: AlignmentMatrixControlValue | undefined | null
 ) {
-	const valueId = transformValue( value ).replace( ' ', '-' );
+	const transformed = transformValue( value );
+	if ( ! transformed ) {
+		return undefined;
+	}
+
+	const valueId = transformed.replace( ' ', '-' );
 
 	return `${ prefixId }-${ valueId }`;
 }
@@ -79,9 +87,12 @@ export function getItemId(
  * @param id       An item ID
  * @return         The item value
  */
-export function getItemValue( prefixId: string, id: string ) {
+export function getItemValue(
+	prefixId: string,
+	id: AlignmentMatrixControlValue | undefined | null
+) {
 	return transformValue(
-		id.replace( prefixId + '-', '' ) as AlignmentMatrixControlValue
+		id?.replace( prefixId + '-', '' ) as AlignmentMatrixControlValue
 	);
 }
 
@@ -93,9 +104,13 @@ export function getItemValue( prefixId: string, id: string ) {
  * @return The index of a matching alignment.
  */
 export function getAlignmentIndex(
-	alignment: AlignmentMatrixControlValue = 'center'
+	alignment: AlignmentMatrixControlValue | undefined | null = 'center'
 ) {
 	const item = transformValue( alignment );
+	if ( ! item ) {
+		return undefined;
+	}
+
 	const index = ALIGNMENTS.indexOf( item );
 
 	return index > -1 ? index : undefined;

Noting that my code changes introduce a slight change of behavior for the onChange prop, which will not fire when the user picks the same value as the currently selected one (meaning that this usecase is expected to fail, since center center is already the default value)

With regards to the tests, I'd love to be able to avoid hacky workarounds — at the same time, I'd be happy to punt that to a follow-up PR and go ahead with the rest of the migration.

---

Most importantly, though, I noticed that this PR so far introduces the new Composite components from ariakit alongside the old reakit version (under a v2.ts file):

• there are still many components relying on reakit's Composite components — those usages should be migrated too
• we need to assess the backwards compatibility of the new APIs with the old ones — for example, how can we ensure backwards compat between useCompositeState (reakit) and useCompositeStore (ariakit)? @diegohaz I guess this question is more for you — ideally we'd like to remove reakit entirely from the project, in favour of ariakit.

[diegohaz]

• we need to assess the backwards compatibility of the new APIs with the old ones — for example, how can we ensure backwards compat between useCompositeState (reakit) and useCompositeStore (ariakit)? @diegohaz I guess this question is more for you — ideally we'd like to remove reakit entirely from the project, in favour of ariakit.

Does it still need to keep backward compatibility considering it's exported as __unstable?

gutenberg/packages/components/src/index.ts

Lines 59 to 64 in 21b6a37

	export {
	Composite as __unstableComposite,
	CompositeGroup as __unstableCompositeGroup,
	CompositeItem as __unstableCompositeItem,
	useCompositeState as __unstableUseCompositeState,
	} from './composite';

If so, the main difference between state and store hooks is that state hooks include all state as direct properties of the state object and re-render the component on every state change. This can be achieved with the store with this code:

function useCompositeState() {
  const store = useCompositeStore();
  const state = store.useState();
  return { ...store, ...state };
}

Besides that, state hooks accept only the initial state as an argument and some properties have been renamed (check Reakit's useCompositeState API reference and Ariakit's useCompositeStore API reference). These must be converted on both the input and the output:

function useCompositeState(props) {
  const store = useCompositeStore(convertInput(props));
  const state = store.useState();
  return convertOutput({ ...store, ...state });
}

[tyxla]

Thanks for working on this migration 👍

I'm leaving a drive-by suggestion on how to make the tests more reliable.

Let me know what you think!

[ciampo]

Does it still need to keep backward compatibility considering it's exported as __unstable?

@diegohaz unfortunately is it my understanding that any APIs that got merged into WordPress could be assumed to be stable regardless of its prefix (see an example of past conversations), which then led to the creation of the lock/unlock APIs as a solution to export private APIs across Gutenberg packages without exposing them to all WordPress developers.

In the case of Composite, maybe it will be a niche enough usage that we'll be able to simply swap its implementation without introducing much disruption — although for sure the safest way would be to implement a layer of back-compat if possible

[diegohaz]

Does it still need to keep backward compatibility considering it's exported as __unstable?

@diegohaz unfortunately is it my understanding that any APIs that got merged into WordPress could be assumed to be stable regardless of its prefix (see an example of past conversations), which then led to the creation of the lock/unlock APIs as a solution to export private APIs across Gutenberg packages without exposing them to all WordPress developers.

In the case of Composite, maybe it will be a niche enough usage that we'll be able to simply swap its implementation without introducing much disruption — although for sure the safest way would be to implement a layer of back-compat if possible

I understand. If I remember correctly, those exports were incorporated into the @wordpress/components package to circumvent the need for directly importing Reakit in other Gutenberg packages. I'm uncertain if this is still the situation, but there was even a lint rule for this.

It would surprise me if this is being used anywhere outside of Gutenberg, particularly since individuals could simply use the stable Reakit API directly, either via npm or CDN. But who knows?

[ciampo]

I understand. If I remember correctly, those exports were incorporated into the @wordpress/components package to circumvent the need for directly importing Reakit in other Gutenberg packages. I'm uncertain if this is still the situation, but there was even a lint rule for this.

That is still the case, and an unfortunate consequence of that approach is that the @wordpress/components package becomes responsible for maintaining those exports, instead of delegating to the third-party libraries directly (ie. ariakit)

It would surprise me if this is being used anywhere outside of Gutenberg, particularly since individuals could simply use the stable Reakit API directly, either via npm or CDN. But who knows?

Unfortunately, there are usages (example WPdirectory search).

Since the whole point of this refactor is to remove reakit as a dependency, I don't think that keeping the old implementation of the Composite components around is an option.

In my opinion, we have a couple of alternatives:

1. write a compat layer that can convert reakit APIs to ariakit APIs, ie.:
   • expose a useCompositeState function that internally uses useCompositeStore
   • rewrite all components in such a way that is able to detect whether they are being used with reakit-style APIs, and in that case convert those props to the new ariakit format
2. decide to stop supporting the old reakit version of the component, thus introducing a breaking change
   • we could export no-op version of the old hooks/components, so that at least don't introduce an import error for current consumers. Those "no-op" hooks/components could also log a warning and push consumers to update to the new version of the component
   • on the side, we could export the new ariakit version of these components. In this case, we wouldn't be able to reuse the same export names as the reakit version, and therefore we could:
     • either remove the __unstable prefix (eg. useCompositeStore, Composite, CompositeItem...)
   • or use the lock/unlock APIs to keep the new version private (and encourage external consumers to use ariakit directly in their projects instead(

What do folks think?

[tyxla]

Given that ariakit is considered an improved version of reakit and its successor, to me it makes sense to go with 2, even if it's considered a breaking change. I think it can easily be sold as an update to a newer version and not a direction shift.

[ciampo]

After a quick chat, here's the plan of action:

• refactor usages of reakit to ariakit in @wordpress/components, but without modifying the version exported by the @wordpress/components package
• temporarily export the ariakit version of Composite via the private APIs
• iteratively refactor all usages of the reakit composite components across gutenberg
• once all instances have been migrated, we stop exporting the ariakit Composite component as private API, and instead change the @wordpress/components package to use the ariakit version (and therefore drop the reakit version)

[ciampo]

Also wanted to flag that, with #54818 merged, trunk now uses @ariakit/react@0.3.3. We should rebase/merge trunk to incorporate those changes in this PR too.

[tyxla]

This is great work @andrewhayward 🙌

I've left some feedback, but don't consider any of it blocking. I'm happy to defer to @ciampo and company for a ✅ .

[ciampo]

Code changes are looking good. I just left a couple more comments, but we're close!

[ciampo]

LGTM 🚀

Let's merge this PR and continue with the next steps!

[mirka]

Dev Note to be added in #58620.