Skip to content
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

@wordpress/components: high-level API strategy for compound components #63704

Open
ciampo opened this issue Jul 18, 2024 · 12 comments
Open

@wordpress/components: high-level API strategy for compound components #63704

ciampo opened this issue Jul 18, 2024 · 12 comments
Labels
[Package] Components /packages/components

Comments

@ciampo
Copy link
Contributor

ciampo commented Jul 18, 2024

Context

As we're re-writing some of the components in the @wordpress/components package to use ariakit, 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:

  • at one end of the spectrum, we could offer a very low-level and modular set of compound components (basically 1:1 to ariakit);
  • at the other end we would expose high-level monolithic components (similar to most components in the @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:

  • Expose compound components, but try to keep a minimum level of abstraction
  • Potentially expose, alongside the "compound" version, a monolithic component representing the simpler and most frequent usage of the component (this aspect should be decided case-by-case and may be also influenced by backward-compatibility reasons).
  • Use Storybook as a place to provide ready-made "recipes", ie. example usages of the compound components that fulfill a certain UI that can be copied and dropped in a codebase with little-to-no tweaking.

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:

  • how would we make sure that consumers can pass props and grab refs or specific subcomponents, like the root wrapper vs the trigger vs the popover wrapper?
  • do we want (and can we) achieve a certain level of control (like discussed here about a potential Dialog component)

A few potential solutions:

  • lower the level of abstraction of our compound components, which would avoid the problem altogether, but would also introduce a different set of compromises;
  • use render props
    • for example, in DropdownMenu we are using a trigger render prop for the trigger button, and the children prop for menu items. Any aspect related to the trigger is passed directly by the consumer to the component rendered in the trigger prop;
    • in a hypothetical 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

@ciampo ciampo added the [Package] Components /packages/components label Jul 18, 2024
@tyxla
Copy link
Member

tyxla commented Jul 18, 2024

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.

@ciampo
Copy link
Contributor Author

ciampo commented Jul 19, 2024

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?

@tyxla
Copy link
Member

tyxla commented Jul 19, 2024

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.

@tyxla
Copy link
Member

tyxla commented Jul 21, 2024

Also, isn't this issue a duplicate of #63242? I guess one of them could be closed in favor of the other one.

@ciampo
Copy link
Contributor Author

ciampo commented Jul 22, 2024

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)

@DaniGuardiola
Copy link
Contributor

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:

  • Break down absolutely everything as a start.
  • Then, bundling parts together (e.g. close button with dialog header) can be considered, in a 1-by-1 basis, being aware of the (very high) costs of it and of the unidirectionality of composability.
  • In these situations, provide clean-ish escape hatches (e.g. a 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 :)

@ciampo
Copy link
Contributor Author

ciampo commented Aug 14, 2024

As we worked on stabilising Composite, a few more aspects to our API approach were brought up

Typing component props

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).

Exposing component stores

Until recently, our ariakit-based components have kept ariakit stores 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:

  • Any store customization, if needed, is exposed via dedicated props on the top-level component (example);
  • Internally, components can use the 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:

  • Allowing consumers of our components to deal with stores directly definitely enables more flexibility:
    • The top-level component prop surface would stay smaller and cleaner
    • Accessing the store gives access to the component's internal state functions, which allow very fine-grain control
  • Exposing these props would make our components even more low-level. We'd need to make sure that such APIs are completely optional and only used for advanced use cases;
  • Exposing stores and store creators would tie us even more to ariakit, making it more difficult to patch any breaking changes from ariakit's side and/or migrate to another third-party consumer;
  • 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 the store object via a custom hook (ie. useComponentContext or something)
  • We'd need to offer a way to forward internal context (including the store) across slot-fill boundaries. So far we've done that by exposing (and manually forwarding) custom component contexts

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.

@tyxla
Copy link
Member

tyxla commented Aug 20, 2024

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.

@ciampo
Copy link
Contributor Author

ciampo commented Aug 20, 2024

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 the store object via a custom hook (ie. useComponentContext or something)

@mirka
Copy link
Member

mirka commented Aug 21, 2024

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.

@tyxla
Copy link
Member

tyxla commented Aug 21, 2024

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.

@ciampo
Copy link
Contributor Author

ciampo commented Aug 22, 2024

Re. the "Exposing components' stores" topic, together with @mirka and @tyxla we agreed on testing the following approach:

  • do not expose use*Store hooks;
  • spread the options that the use*Store hook would accept on the top-level component (ie. Composite)
  • if consumers of the component need access to the store for any advanced usages, they can do so via a Context exposed by the component

I'm testing the approach on the Composite component in #64723

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components
Projects
None yet
Development

No branches or pull requests

4 participants