Open ciampo opened 1 month ago
At first glance, picking a high-level strategy that works for all components can be tricky or limiting. We can decide on a component-by-component basis because the strategy for each compound component group may be different. For each of them, we may want to expose a different level of the underlying abstraction.
On the contrary, I'd be happy if we could demonstrate that a unified strategy can work for all the components we want to build as compound components in the future.
I agree that every component is different, but I think it's still valuable to discuss our options at a high-level, and understand which tools we want to add to our belts, and which ones we may want to avoid to start with.
For example, in the case of DropDownMenu
or CustomSelectControl
, which option do we prefer?
// Option A: render prop
<DropdownMenu.Root trigger={ <TriggerButton /> }>
<DropdownMenu.Item />
<DropdownMenu.Item />
<DropdownMenu.Item />
</DropdownMenu.Root>
// Option B: more low-level apis
<DropdownMenu.Root>
<DropdownMenu.Trigger />
<DropdownMenu.List>
<DropdownMenu.Item />
<DropdownMenu.Item />
<DropdownMenu.Item />
</DropdownMenu.List>
</DropdownMenu.Root>
// Or any other alternatives
If we think that there's space for both options depending on the component, what factors could help us to make the best decision on a component-by-component base?
I personally prefer option B in this instance. However, there might be instances where a render prop could be more helpful, especially if you want to share it between multiple components in the tree. I find it difficult to generalize our decision-making process, but again, I'm open to suggestions.
Also, isn't this issue a duplicate of #63242? I guess one of them could be closed in favor of the other one.
In my opinion, this PR and #63242 are discussing two separate topics. This PR is about how we compose our compound components (regardless of how we name them), and how we make sure we expose a good API surface to the consumers of the components (see this example conversation)
I wrote about pretty much this exact issue and I think it fully applies here, so consider this my take on it: The "everything bagel" of components (article on my blog)
TL;DR:
hasCloseButton
prop that defaults to true
, so the user can set it to false
).I made a great effort to think deeply about this topic and write about it the most clear and detailed way possible, so please do read it :)
As we worked on stabilising Composite
, a few more aspects to our API approach were brought up
One aspect that was discussed was how to type those sub-components' props.
Ariakit types are quite complex and internally build on many layers of modular utilities and dependencies. Adding first-party types for such props would require us to add a lot of internal ariakit types, causing duplication and adding maintenance costs.
On the other hand, relying on Ariakit's types gives us less control and can potentially cause us to "forward" API (breaking) changes to our consumers without any type check from TypeScript.
Another negative aspect of using Ariakit types directly, is that currently our Storybook setup is not able to extract those prop types correctly, and we have to do that manually instead (which is a lot of work).
store
sUntil recently, our ariakit-based components have kept ariakit store
s as an internal implementation detail of the component — in other words, those components don't expose neither the store
prop, nor the use*Store
hooks:
use*Store
hook to add some internal logic (example)As we're about to stabilize Composite
, though, we noticed that the private version of the component allows for its consumers to use the useCompositeStore
hook to create a store
and pass it to composite components via the store
prop.
Before we stabilize any of these components (Composite
, Tabs
, DrodownMenu
, etc), I think we should discuss which approach we should take, and aim at applying the agree approach across all ariakit-based components.
@DaniGuardiola suggested adopting and exposing store
, use*Store
hooks and "provider" components.
I'll try to list some initial considerations:
store
gives access to the component's internal state functions, which allow very fine-grain controlTabs
) if we let consumers define a store externally?store
, instead of exposing store creator hooks? That would allow us to add custom logic (see above point). We could still allow access to the store
object via a custom hook (ie. useComponentContext
or something)In general, I value adopting a coherent approach as much as possible, and therefore I'd like to agree on an approach that can be applied across all ariakit-based components.
@WordPress/gutenberg-components , let's discuss.
My $0.02: since we already have existing use cases outside the @wordpress/components
package that access the store, we'll still need to expose store
props and therefore the use*Store
hooks and the some generic hooks like useStoreState
.
I agree that a store
should be optional, in order to keep default usage as simple as possible, while still allowing for advanced customization.
I'm not particularly concerned about us being tied to Ariakit, since we're already headed that way in many different regards.
Would love to hear opinions from @mirka and @DaniGuardiola.
we already have existing use cases outside the @wordpress/components package that access the store
From a quick search, the only usages of Ariakit stores outside of the components package are about useCompositeStore
, which is still a private API. So that shouldn't be a concern / limitation.
I'm not particularly concerned about us being tied to Ariakit, since we're already headed that way in many different regards.
On this aspect, what's important is that we make a conscious decision and understand pros and cons.
Also, I would like to highlight a few questions from my previous message that I'd like folks to answer to:
- How would we implement custom internal logic (like in
Tabs
) if we let consumers define a store externally?- When talking about component store providers, would they be a 1:1 with ariakit providers? Could that be the place where we build a
store
, instead of exposing store creator hooks? That would allow us to add custom logic (see above point). We could still allow access to thestore
object via a custom hook (ie.useComponentContext
or something)
A strategy I am leaning toward is to keep our API surface as minimal as possible until someone comes with a specific use case and requests the feature.
So if someone has a legitimate use case that requires exposing the store, we expose the store but only for the relevant prop. And maybe the store is merged with our internal store.
This will require some administrative overhead to process each feature request, but it will allow the API surface to grow organically rather than having to offer the entire smorgasbord on day one. We only pay the cost of documentation, typing, and maintenance for the features that are actually needed enough that somebody asks for it. These are infrequently used “advanced” features in the first place — it shouldn’t be taking up a disproportionate amount of our time.
Another downside is that if the request comes from an extender, they will have to wait until the next release until the feature is publicly available. But I don’t anticipate getting these requests very often. Any obvious gaps will be addressed early on, and the rest will be rare edge cases.
OK, thanks for the feedback, folks.
These are all legitimate reasons to keep the API minimal and simple. I'm happy to go with that by default and expose additional props only when necessary. This also removes the concerns you highlighted in your last message, @ciampo, right?
Can you think of any obvious cons of going with this minimal approach? If not, let's just do go for it.
Re. the "Exposing components' stores" topic, together with @mirka and @tyxla we agreed on testing the following approach:
use*Store
hooks;use*Store
hook would accept on the top-level component (ie. Composite
)store
for any advanced usages, they can do so via a Context
exposed by the componentI'm testing the approach on the Composite
component in #64723
Context
As we're re-writing some of the components in the
@wordpress/components
package to useariakit
, we can take advantage of the low-level and granular nature of the library components, and build our components with the desired level of abstraction:ariakit
);@wordpress/components
package).Each approach has pros and cons: monolithic components offer a pre-packaged, easy-to-use solution to consumers, but are very inflexible, and often end up causing severe YAPping and style overrides; low-level components are flexible, but are more difficult to use and offer little control and "art-direction" to the library.
We need to pick the compromises that we think suits best the role of the package, and decide how to mitigate the negative aspects of that choice.
Our shared approach so far has been to find a middle group:
What
The tricky aspect when "abstracting" lower-level components into higher level, is that there can be a confluence of originally "separated" APIs in one place. This aspect started emerging when working on components that have both a trigger and a related popover, like dropdown menu or select controls (see example conversation).
We need to find ways to make sure that our APIs are flexible and future-proof and are clear to our consumers. For example:
Dialog
component)A few potential solutions:
DropdownMenu
we are using atrigger
render prop for the trigger button, and thechildren
prop for menu items. Any aspect related to thetrigger
is passed directly by the consumer to the component rendered in thetrigger
prop;Dialog
component, we could have use render props as "slots" in order to restrict the design and/or behavior of the component if needed.Any thoughts?
cc @WordPress/gutenberg-components