Implementing useCompositeState with Ariakit
#57304
Conversation
This patch adds support for the Reakit API, using underlying Ariakit components.
andrewhayward
added
[Type] Enhancement
|
Originally the unit tests were set up to allow for both the original and legacy implementations to run on the same suite... describe.each( Object.entries( COMPOSITE_SUITES ) )( (...) => {
...
});However, the two APIs ended up being so similar that TypeScript didn't know what to do with itself. Given that we're going to be removing the Reakit implementation once we're happy with this, it didn't seem worth fighting the system to make that happen. So we've got two almost identical test suites instead. |
|
Flaky tests detected in d43e02d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7715615393
|
There was a problem hiding this comment.
Thank for working on this!
We had previously discussed adopting a much simpler approach to adding this layer of backward compatibility (#56645 (comment)), is there a reason why you opted for re-implementing (most of) the useCompositeState hook return value?
I will also let @mirka and @tyxla (and @diegohaz if has any feedback to share) take over this PR review, as I won't be around for the next couple of weeks.
My understanding of the noted conversation was that we'd add 'best-effort backward compatibility'. So while I appreciate that you'd said we shouldn't "add extra code to re-implement reakit functionality", I honestly don't see this as doing that; custom state props are intentionally not supported, and Ariakit is still doing the work in the generated state; it's just being exposed in a way that Reakit consumers would expect. It's easy enough to not include, if collectively we're against the idea, but for pretty minimal effort, it seemed to be within the spirit of 'best-effort backward compatibility'. |
|
I'm not sure about the degree of backward compatibility we should enforce in this scenario, but I think the argument for not exposing the entire Reakit API for now—or most of the Having said that, I believe the implementation can exist but remain unexposed, with a comment left in place for potential future exposure (or permanent removal). |
|
Size Change: +92 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
|
I've dropped the support for the Reakit API, simplifying the code a bit and removing some of the noted complexity. This means that as things currently stand, the originally stated scenario should work... // ✅ This works
const state = useCompositeState({ ... });
return (
<Composite { ...state }> // or <Composite state={ state }>
<CompositeItem { ...state }>Item</CompositeItem>
...
</Composite>
);...but providing custom props will not, nor will // ⛔️ This won't work as expected
const state = useCompositeState({ ... });
return (
<Composite { ...state } wrap={ true }>
...
</Composite>
);
// ⛔️ And this won't work at all
const state = useCompositeState({ ... });
const { move } = state;
move('id'); |
|
Pretty sure all of the issues raised have been resolved. The only outstanding point is what to do about |
| return ( | ||
| <Composite role="grid" store={ store } aria-label="Ariakit Composite"> | ||
| <CompositeRow role="row"> | ||
| <CompositeItem role="gridcell">Item A1</CompositeItem> |
There was a problem hiding this comment.
Storybook flags a minor accessibility issue across the Storybook examples:
"ARIA role gridcell is not allowed for given element"
There was a problem hiding this comment.
It's complaining about the buttons having a role. It's not really that big a deal, particularly in this context.
packages/components/src/composite/unstable/stories/index.story.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I'm flexible on all the Storybook stuff (they can be iterated on after merge), and I had one important thing to flag re: the deprecation messages, but even that is not a blocker for this PR to land since the "Legacy" code is not going to be live yet.
So thumbs up from me as well 👍
packages/components/src/composite/legacy/stories/index.story.tsx
Outdated
Show resolved
Hide resolved
| deprecated( `wp.components.__unstable${ previous }`, { | ||
| alternative: `wp.components.${ next || previous }`, | ||
| } ); |
There was a problem hiding this comment.
private status. (Public consumers won't have anything to migrate to yet.)
My impression was that we're going to drop "Unstable" very soon (like right after this PR merges) and replace it with this "Legacy". If "Legacy" already logs deprecation warnings, we're going to have to immediately graduate V2 from private status. Basically, do all the remaining 4 task items in #55224 in one commit. Is that the plan?
Depending on the timeline, we also need to be mindful of how we present this in Storybook — the "Current/Legacy/Unstable" classification was useful for development, but as public documentation it's inscrutable to consumers so we need to make those reflect what we actually export (i.e. Composite and Composite V2.
If we want to drop Reakit ASAP but have the private beta period run a little longer, we're going to have to comment out the deprecation stuff for now.
cc @ciampo
There was a problem hiding this comment.
Good point. Commenting out the deprecation message seems like a good idea — we can always quickly uncomment it whenever we're ready, and in the meantime we won't be rushed to make the transition.
Depending on the timeline, we also need to be mindful of how we present this in Storybook — the "Current/Legacy/Unstable" classification was useful for development, but as public documentation it's inscrutable to consumers so we need to make those reflect what we actually export (i.e.
CompositeandComposite V2.
I guess that, for the time being, we could rename as follows:
- "Current" => "Composite V2"
- "Legacy" => "Composite (compat layer)"
- "Unstable" => "Composite (original)"
As we remove the reakit-based version, maybe we could rename the two stories "Composite (legacy)" and "Composite" ?
There was a problem hiding this comment.
At the moment, the legacy version isn't easily accessible, short of explicitly requiring it. __unstable* imports will use the original unstable version, and any private 'V2' imports will use the current version. So in some ways the more confusing part is around Storybook – we'd be showing docs for something that isn't available.
We could promote the current Ariakit implementation from private without that changing; anyone importing __unstable* would continue to get the Reakit version. We could then point the __unstable* exports at the legacy version, and in theory nobody would notice. At which point, we could drop the unstable Reakit version.
So in my head the decision is really around how we list stories. We probably shouldn't be showing the legacy version at all, but we could maintain "Components / Composite" as it is now, and move "Components / Composite / Current" to "Components / Composite (V2)" or similar.
Once we shuffle things around, "Components / Composite" could then point at the current version, and the legacy implementation would be under "Components / Composite (legacy)" or similar.
There was a problem hiding this comment.
So in my head the decision is really around how we list stories. We probably shouldn't be showing the legacy version at all, but we could maintain "Components / Composite" as it is now, and move "Components / Composite / Current" to "Components / Composite (V2)" or similar.
Once we shuffle things around, "Components / Composite" could then point at the current version, and the legacy implementation would be under "Components / Composite (legacy)" or similar.
Sounds good 👍
|
For clarity, the final steps needed to land this PR:
Steps needed as a follow-up
|
|
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 Core SVNIf you're a Core Committer, use this list when committing to GitHub Merge commitsIf 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. |
andrewhayward
deleted the
56548/useCompositeState-legacy-implementation
branch
What?
This patch adds support for the Reakit
compositeAPI, using underlying Ariakit components.Why?
In order to fully remove Reakit as a dependency, we first need to ensure a degree of backwards compatibility so that builds don't break. This PR ensures that a 'legacy' API will be available for consumers of the current Reakit-based
compositecomponent, while allowing us to proceed using Ariakit instead.How?
An implementation of
useCompositeStatehas been written that creates an Ariakit composite store based on Reakit's configuration options. Mapping functions are provided to bridge the functionality gap between Reakit'scompositeStateand Ariakit'scompositeStore.Additionally, a wrapper function has been created to proxy Ariakit-based
compositecomponents to accept the Reakit-shaped state.As such, consumers should not have to update their code, as we can replace the
__unstableComposite*exports with the legacy wrappers, and while the implementation will be different, it should continue to just work.It's worth noting that while Reakit's
useCompositeStateis locked, this legacy implementation is not. If different options are provided touseCompositeStatein a re-render, they will be picked up. State setters have been provided for backwards compatibility.Testing Instructions
Unit tests have been written for both the original and legacy libraries, and both should pass without issues.
Storybook pages have been created for both the original and legacy libraries (as well as Ariakit, for comparison) – these can be explored and tested, and should work without any significant noticeable difference.