WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.27k stars 4.09k forks source link

`@wordpress/components`: agree on a naming convention for compound (sub) components #63242

Closed ciampo closed 3 weeks ago

ciampo commented 1 month ago

As we plan on introducing more modularity (ie. compound components) to some of the components offered by the package, we need to make sure that we pick the right naming strategy. Bringing some consistency will also retroactively benefit existing components.

Options

Here are some options that @DaniGuardiola and myself came up with while discussing this topic:

Option 1: Function + properties


// Implementation
function Component() {}
Component.Subcomponent = function() {}

// Usage
<Component>
  <Component.Subcomponent />
</Component>

Option 2: Dot notation (either by exporting an object or by using single named exports)

// Using objects:
//================

// Implementation
Component = {
  Root: function() {},
  Subcomponent: function() {}
}

// Usage
<Component.Root>
  <Component.Subcomponent />
</Component>

// Using named exports
//================

// Implementation
export function Root() {}
export function Subcomponent = function() {}

// Usage
import * as Component

<Component.Root>
  <Component.Subcomponent />
</Component>

Option 3: "flat" namespacing (ie. without dot notation)


// Implementation
export function Component() {}
export function ComponentSubcomponent = function() {}

// Usage
import * as Component

<Component>
  <ComponentSubcomponent />
</Component>

Mixing compound components and monolithic components

Finally, we may also have to keep, for the same component, both a "monolithic" version and a more "a la carte", compound components-based version.

One hypothesis that made was to use the default export for the monolith, and to use option 2 (dot notation) for the compound components (ie.):


// Implementation

export function Root() {}
export function Subcomponent() {}

export default function Comp() {
  return (
    <Root>
      <Subcomponent />
    </Root>
  );
}

// Usage

<Component />
// or
<Component.Root>
  <ComponentSubcomponent />
</Component.Root>
ciampo commented 1 month ago

I'm not a fan of option 1 for a few reasons:

Personally, I lean towards option 2 and 3, but I'm sure that I may be missing interesting and important aspects that we should consider.

ciampo commented 1 month ago

I'd love to hear folks' opinions on this one, especially @diegohaz (who has experience with Ariakit) and the rest of @WordPress/gutenberg-components .

Also cc'ing @WordPress/gutenberg-core in case.

youknowriad commented 1 month ago

I don't mind option 1 and 3 but I don't like option 2. The Root seems artificial.

ntsekouras commented 1 month ago

I'd echo Riad with a preference in option 3 for the reasons you mentioned above..

aaronrobertshaw commented 1 month ago

I'd lean towards option 3 as I agree with the arguments against both 1 and 2 so far.

ciampo commented 1 month ago

Some extra perspective:

Using option 3 as illustrated above would pose the question of how to achieve monolithic and compound components under the same name — the Ariakit-style approach would probably be the solution.

mirka commented 1 month ago

it's not TypeScript-friendly

@ciampo What are the issues here? I don't think I've noticed any.

mirka commented 1 month ago

The reason we started looking for alternatives to option 3 is that it's very unergonomic when dealing with imports and private API unlocking. This will become even more of an annoyance when an increasing number of our components become modular.

https://github.com/WordPress/gutenberg/blob/bcf2b357d5a56452d8ce6e3d42af96da8ff78362/packages/editor/src/components/post-actions/index.js#L23-L26

Ariakit doesn't have this ergonomics problem because you import the entire package as * as Ariakit. This approach is somewhat off the table for us because we still need to do private API unlocking, as well as renaming (e.g. __experimental prefixes or V2 version suffixes).

Practically speaking, since we already have an entire library where Component is always the monolithic version, it would be very confusing if we did any kind of partial/gradual transition to option 2. Plus, this would rely on the consumer having to do a path-based import (import * as Component from '@wordpress/components/component), which is a departure from project-wide rules and conventions.

So if we do want to tackle the downsides to option 3, the only feasible option for me is 1. Or more specifically, the "Mixing compound components and monolithic components" approach. I've thought about the tree-shaking downsides, and I think the cost is negligible as long as we design things appropriately.

noisysocks commented 1 month ago

I used shadcn/ui which uses option 3 in a side project. I didn't find the verbosity to be a problem.

diegohaz commented 1 month ago

The main reason we use separate named exports in Ariakit is tree-shaking. We have many optional components that extend the functionality of their modules. If they were exported as a single object, you would bundle all the features, even when using the simplest version of the component. I also feel this aligns better with modern JavaScript and ESM.

That said, this may not apply to higher-level component libraries like @wordpress/components.

DaniGuardiola commented 1 month ago

Thanks for sharing this here! FYI, I am writing a detailed comparison between APIs and approaches, with all their pros and cons. Will share soon for our consideration.

ciampo commented 1 month ago

While waiting for @DaniGuardiola to share his thoughts, I basically came to the same conclusion as @mirka 's:

the only feasible option for me is 1. Or more specifically, the "Mixing compound components and monolithic components" approach. I've thought about the tree-shaking downsides, and I think the cost is negligible as long as we https://github.com/WordPress/gutenberg/pull/60926#issuecomment-2191646729.

I don't really see valid alternatives to keeping <Component /> for the monolithic version of a component, and using subcomponents as properties on the main export.

We will have to find a naming convention for the top-level subcomponent, though — common choices are Root and Provider, although I'm not sure about Provider because it implies that no markup will be rendered, which is not the case for our compound components. Happy to consider alternatives, if folks have any.

youknowriad commented 1 month ago

We will have to find a naming convention for the top-level subcomponent, though — common choices are Root and Provider, although I'm not sure about Provider because it implies that no markup will be rendered, which is not the case for our compound components. Happy to consider alternatives, if folks have any.

Why do we need a naming convention? Card is the the level component, Dropdown or Menu could be top level. I don't know that we need a convention, it seems specific to every component.

ciampo commented 1 month ago

As we're moving towards having more compound components, we feel like we should agree on how to name such components, in order to clearly differentiate between monolithic and compound components, indicate their hierarchy and establish a coherent approach throughout the package.

There will be occasions in which we will rewrite an existing component, exposing its subcomponents, but we still need to offer the monolithic version too for backwards compat reasons. So we need a way to distinguish the monolithic component, from the top-level compound component.

youknowriad commented 1 month ago

There will be occasions in which we will rewrite an existing component, exposing its subcomponents, but we still need to offer the monolithic version too for backwards compat reasons. So we need a way to distinguish the monolithic component, from the top-level compound component.

It sounds like two separate things for me. The ideal name is always the "non prefixed" one IMO and I'm not sure that we should fine another name just because we want to rework how the component works. In the past, we've used __experimentalVersion to introduce a new version of a component. We were not good enough to follow-up properly there (and switch the default version) but it seems like a valid option too to use a prop. I know that for other cases (Toolbar) we're used the.label prop as a discriminator. I'd prefer this discussion to happen for each component personally rather than finding a generic approach that would prevent us from having the right naming and right props.

ciampo commented 1 month ago

I may have explained myself poorly — the naming convention that we'd like to agree on is not about versioning strategies (and I agree with you that, when introducing a new version of a component, we should discuss the best course of action on a per-component basis).

In other words, we'd like to standardise how we name (and namespace) components, especially given that we will sometimes need both Component (monolithic) and Component.Root (top level subcomponent) for components that can be consumed both as a monolith or as compound components.

youknowriad commented 1 month ago

@ciampo Can you give examples of cases where we need both Component and Component.Root?

DaniGuardiola commented 1 month ago

@youknowriad we are in the process of creating better versions of the components, but are limited by the namespace, as we can't just replace or remove a component (it would break everything).

The new components are designed to be composable and address many of the shortcomings of the previous versions. One way we're doing this is by creating them with a compound API (in other words, broken down into multiple sub-components). This issue is about which naming strategy and API variant in particular is the one we feel is best.

Marco is suggesting that the approach of adding subcomponents to the existing monoliths is a good way to solve the name spacing issue, and prevent breaking the old API while providing a new, better one.

I hope this helps clarify things for you :)

youknowriad commented 1 month ago

@DaniGuardiola That makes sense to me but I said above that the "non breaking" part can also be done by keeping the same name for the top level component and adding a version prop.

Hypothetically, we could need both a root level component and an unsuffixed component but I'm not entirely sure when, so a concrete example might help.

DaniGuardiola commented 1 month ago

I can appreciate the suggestion, but that strategy feels like a potential nightmare in terms of prop types and maintaining a conditional implementation. I think having a separate root component is better, and having it as a property of the original one might be a good way to represent that strong correlation if done consistently.

I still need to think more about this, and I am working on my comprehensive comparison, but at face value it seems like something that could work:

Want the old version? Use Component.

Want the new, composable version? Use Component.Root and other subcomponents (Component.Whatever).

youknowriad commented 1 month ago

I just don't like Component.Root or Component.Provider, it doesn't make sense when you want a Dropdown. A version is clear indicator that you have to migrate and an approach that we used in the past for a couple of components already.

youknowriad commented 1 month ago

One other downside is that for new components that don't exist, we'll be artificially forced to use Something.Root even without any backward compatibility concern rather than just Something because we need to follow a convention.

mirka commented 1 month ago

The need for Component and Component.Root coexisting is not about versioning, but for providing an easy/consistent monolithic component for most use cases, in addition to providing a modular escape hatch for more customized use cases. So depending on the component, sometimes a Component.Root will not be necessary.

(Tangentially, we have in fact considered a version prop or discriminator prop strategy for versioning components, but dropped the idea because it would add a lot of TypeScript/documentation complexity, as well as not being tree-shakable.)

youknowriad commented 1 month ago

Personally I don't like being the blocker here. I think forcing a convention for no reason is not useful and I'd rather discuss this when implementing components instead. So let's just move forward and avoid losing time.

DaniGuardiola commented 1 month ago

@youknowriad

I just don't like Component.Root or Component.Provider, it doesn't make sense when you want a Dropdown

Why?

One other downside is that for new components that don't exist, we'll be artificially forced to use Something.Root even without any backward compatibility concern rather than just Something because we need to follow a convention.

Just to clarify, you're suggesting that we use option 1 then? If so, the namespacing issue is relevant - if we want a version of "XComponent" that is compound, but there's an existing "XComponent", then we have a conflict. One way to solve it would be through the versioning prop you're suggesting, to which we've already replied, twice:

Me: "that strategy feels like a potential nightmare in terms of prop types and maintaining a conditional implementation"

@mirka: "[we] dropped the idea because it would add a lot of TypeScript/documentation complexity, as well as not being tree-shakable"

If you intend to insist on this point (which is fine), I would appreciate a response.

Personally I don't like being the blocker here. I think forcing a convention for no reason is not useful and I'd rather discuss this when implementing components instead. So let's just move forward and avoid losing time.

It is not for no reason. Each naming/export approach has non-trivial implications, as has been discussed (and as I will expand on in detail in my upcoming write-up), so the goal here is to go through the process of selecting an approach once and be done with it.

This is not something that depends on the context of a specific component. Something being a menu or a select doesn't change anything about whether Menu.Item is a better option than MenuItem, or whether doing named exports is better than exporting a single object or a function with properties.

There are tree-shakeability concerns, developer experience concerns (both for maintainers and users), readability concerns, backwards compatibility concerns, discoverability concerns... And it all has to be balanced because there are trade-offs everywhere. I personally don't want to have this discussion for each component we create or update. We are also not "forcing" a convention, we are making a technical decision as maintainers of this package, with the sole intent of providing the best experience to consumers, users, and ourselves.

This started as a sync discussion between @ciampo and me, which we brought here to get more feedback from other folks in the community. I would appreciate your thoughts on whether an approach is better than others and why. I don't think saying "let's not make a decision" -without giving examples or reasoned arguments of why you think that's the better route- is helpful.

youknowriad commented 1 month ago

I think you misunderstood my latest comment. I meant: I still think it's wrong to force a .Root but if you all insist on it, I'm fine to disagree and commit, let's not wait time on this.

Now for the why, I explained above that if I'm starting from scratch, the ideal name for a Dropdown as a user is Dropdown, not Dropdown.Root (Root is meaningless IMO) and also ideally, if it conflicts with existing components, we should find a way to deprecate the existing one and transition to the ideal outcome which is not possible if we go with Dropdown.Root. What happens tomorrow, if we figure that Dropdown.Root is not ideal either (say a new React version brings something that changes things fundamentally or we want to deprecate some behavior...), are we going to try to find a new naming convention just because of that? This is why IMO, versioning of deprecated components and naming are two separate things.

ciampo commented 1 month ago

if I'm starting from scratch, the ideal name for a Dropdown as a user is Dropdown, not Dropdown.Root.

I understand your point of view (and actually it was my preferred option too at the beginning), but IMO that's more of a style/preference, and not necessarily an aspect where there is a "right" and a "wrong" way of doing things. For example, to cite a few of the highest-rated headless react component libraries, RadixUI uses the Component.Root naming, ariakit the Component.Provider naming, and React Aria the naming that you suggested (option 3).

versioning of deprecated components and naming are two separate things.

I agree that they are separated, although IMO naming APIs, in general, can have a lot of implications and overlaps, including when we deprecate APIs.

Let me try to unroll the train of thoughts that brought @mirka @DaniGuardiola and myself to have this conversation, and to lean towards the conventions proposed above.

  1. As we create/rewrite components to ariakit, we will introduce more compound components to the package, and therefore we asked ourselves how we should name their exports;
  2. As discussed above, there are many implications (like tree-shakeability, potential conflicts, dev ex...).
    • One aspect that we wanted to consider is that most of the components in the library, especially the earlier ones, are monolithic. Before even considering any backward compatibility and versioning aspects, we took into account the fact that, contrarily to most modern headless component libraries, our library would offer both monolithic and compound components — and therefore we think that finding a naming convention that makes a distinction between those two kinds of components could be valuable;
      • We also don't exclude that some of our components could expose, at the same time, a monolithic version and a compound version. The compound component would offer more flexibility where needed, while the monolithic version (which would internally use the compound components) could have its utility in offering an easy-to-use API for the most typical component use case.
    • Existing components could also benefit from having a new compound version introduced alongside existing monolithic versions

I hope that this can share more context, and make our thoughts more clear and easier to follow.

diegohaz commented 1 month ago

if I'm starting from scratch, the ideal name for a Dropdown as a user is Dropdown, not Dropdown.Root.

I understand your point of view (and actually it was my preferred option too at the beginning), but IMO that's more of a style/preference, and not necessarily an aspect where there is a "right" and a "wrong" way of doing things. For example, to cite a few of the highest-rated headless react component libraries, RadixUI uses the Component.Root naming, ariakit the Component.Provider naming, and React Aria the naming that you suggested (option 3).

In case it helps, here are a few reasons why Ariakit chose ComponentProvider instead of something like Component.Root:

ciampo commented 1 month ago

@diegohaz Thank you for adding more context!

Personally I think that Provider works well for ariakit also because it "provides" the state to all underlying components as an alternative to passing the store directly (docs), similarly to React context providers.

"Root" sounds like something that should be required, as it's the root of the tree.

That's a good observation, and in our case it would be true — Root would always be mandatory.

"Root" sounds like something that renders an element in the DOM tree:

That's another interesting observation — the term "root" in Web Standards usually refers to a DOM element or to the shadow root.


More in general, I think that @DaniGuardiola @mirka and myself have given as much context as possible about the implications that we took into account while reasoning on what naming convention could best fit the needs of the @wordpress/components package.

Especially for folks who expressed an opinion against the proposed convention (dot notation, Root name for the top-level component, un-dotted name reserved for monolithic component), it would be great to know if the additional context that we shared recently helps to put the proposal into perspective.

Alternatively, we're open to hearing about alternative solutions that take into account the constraints of the package, weighing in on the details and implications.

ciampo commented 1 month ago

Update: in the past few days we continued discussing nuances and consequences of the possible naming conventions, as we'd like to take the feedback received into account as much as possible.

Upon reflecting on it, we thought that we could drop the constraint (that we initially set for ourselves) that the @wordpress/components package needs to offer, for certain components, both a low-level and a high-level version. By not committing to that, we won't have to deal with reserving the "un-suffixed" export name for monolithic components.

We will basically be able to pick between Option 1 (dot notation) and Option 3 (flat names), without the need for the Root name — which should meet the general preference of the folks who contributed to the conversation, and should introduce fewer changes to what it's currently implemented in the package.

Here are a few more considerations:

We believe that this represents a good compromise.

Unless folks disagree with the proposed approach, the last decision to take would be to pick between dot notation and flat naming — I know that @DaniGuardiola has been looking into this and should shortly share more about it.

DaniGuardiola commented 1 month ago

Thank you @ciampo!

As promised (too long ago, apologies), here's a draft where I list all different options and compare their pros and cons: https://drafts.dio.la/article/compound-components

In the end, I've realized that there are really only 4 options that depend on the namespacing strategy and the export style:

I've compared them in 4 areas:

Please do read it to see my findings and to have a common, well-defined language we can use in this discussion moving forward. Feedback very welcome (in fact, please do give me feedback, I want this to be a complete-enough resource for future reference).

As per our latest thinking, we seem to be leaning into either "Flat + named export" or "Overloaded" - a.k.a. options 3 and 1.

Some observations and thoughts:

  1. As you can see in my article, "overloaded" doesn't tree-shake while "flat" does.
    • Other than stylistic preferences, I think the above is the only point in favor of "flat" over "overloaded".
    • However, in these kinds of components and large projects there's not much to gain from tree-shakability. We certainly didn't seem to care before, as we were using monolithic components.
    • When you go down to the details, this becomes even more apparent. If you can name a part of a component that would make sense to tree-shake for less-than-insignificant gains, I'll gift you a copy of Outer Wilds.
    • Let's get real. WordPress is not tree-shaking externalized libraries any time soon. The point is DOA.
    • Also, in case it's not clear: this only refers to the tree-shaking of parts of components. The tree-shaking of full components (whole sets of parts) is still possible in all variants.
  2. "Overloaded" has a significantly nicer DX, since it supports component and library* level autocomplete, which "flat" does not.
    • * See article for details. "Flat" means that every part of every component is available at the top level, which clutters it and makes it almost unusable if your goal is to quickly see and pick a component from the library.
  3. I know there have been opinions on the "Root" naming. I get it, I don't fully like it myself. That said, there are two disadvantages to choosing "overloaded" (no "Root") over "dot + object export" (requires "Root"):

    • In "overloaded" style, the component autocomplete not only contains the parts, but also the function prototype methods and properties.
    • This is a temporary annoyance, but since components need to use forwardRef, the type story becomes complicated.

      The cleanest solution I came up with (after a lot of iteration) is what you can see here, for example. Basically, you can't nicely co-locate JSDoc, types, and implementations of components and you need to resort to ugly methods like these.

      Fortunately though, in React 19 forwardRef is not necessary anymore and components can be plain function declarations, which (through a not very well know but awesome TypeScript feature) do support a straightforward type/JSDoc implementation. Simply create the function and then append properties to it, and TypeScript will automatically detect this and 1. overload (haha) the function type as a module and 2. add those properties to the module.

      In conclusion, I don't think this is something we should care much about, as it's temporary.

With all this said, my stylistic preference between the two is "overloaded", plus I think there are objective advantages over "flat" and the main trade-off (tree-shakability) does not really apply imo, as I argued.

mirka commented 1 month ago

Thank you @DaniGuardiola for the analysis and providing us with shared terminology to work with. I'm happy to see that the "overloaded" style withstands some more comprehensive evaluation.

Although I'm already onboard with "overloaded", for completeness, another consideration we should add in the evaluation is displayName. A small downside to some approaches is that we need to explicitly set a displayName for it to make sense in devtools and Storybook auto-generated code snippets.

Another aspect (which may be too Gutenberg-specific for a general audience) is how we deal with the component development lifecycle (including versioning). Components can be run through lock/unlock functions, and renamed at export/import. "Overloaded" or "object export" make this very easy.

DaniGuardiola commented 1 month ago

@mirka good point about displayName. I think it might not be an issue as long as we use named functions, e.g.

export function Menu() {};
Menu.Button = function MenuButton() {};

Or in object export form:

export const Menu = {
  Root: function Menu() {},
  Button: function MenuButton() {},
}

Sure, the display name will be MenuButton instead of Menu.Button, but honestly that doesn't seem like a big deal to me since the goal is to provide enough clarity for devs to tell which component is being inspected instead of some anonymous name.

DaniGuardiola commented 1 month ago

Btw, since you raised the point about lock/unlocks, I've been thinking about how we could simplify the system and this could be a good opportunity to do so. Right now the API is more or less something like unlock( componentsPrivateApis ), which means that all components are lumped together in the componentsPrivateApis object. It's also a bit awkward that it requires passing through the unlock function to obtain the "unlocked" value, which gets in the way of automatic imports.

I'm not sure how a solution looks like, but ideally this would be the flow for unlocking, IMO:

import { SomePrivateComponent } from `@wordpress/components`;

export function Whatever() {
  return <SomePrivateComponent />
}

Dev runs it, error happens: "this component is private etc etc "


import { SomePrivateComponent } from `@wordpress/components`;

+unlock(SomePrivateComponent);

export function Whatever() {
  return <SomePrivateComponent />
}

And now the component is unlocked for the module. Again, I don't know how this could be implemented, I'm trying to think of some JavaScript magic that could allow this (maybe some lexical scope magic or something), there must be something close enough. But you get my point. Autoimports, discoverability, simplicity.

If there are concerns about private components being too discoverable, we could always not export them at the root of @wordpress/components, and instead re-export them from a different @wordpress/private-components package, so that there's more clarity about them being private (and users that don't install the package in the first place don't have access to them) while still providing all of the nice DX features.

tyxla commented 1 month ago

@DaniGuardiola I think rediscovering the locking/unlocking mechanism should be our last concern, especially because it's a system that's used across the entire repository and not just in @wordpress/components.

DaniGuardiola commented 1 month ago

Sure, I'll file this as a separate issue for discussion in the future. I don't consider it blocking for this discussion.

ciampo commented 4 weeks ago

My preference is also for what Dani called "Overloaded". The pros and const have been already listed above, and I think that the pros outweigh the cons.

If there aren't any objections, I'll reflect this choice in a new section of the guidelines in https://github.com/WordPress/gutenberg/pull/63714, and we'll be able to mark this issue as closed.