-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
: agree on a naming convention for compound (sub) components
#63242
Comments
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. |
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. |
I don't mind option 1 and 3 but I don't like option |
I'd echo Riad with a preference in option |
I'd lean towards option 3 as I agree with the arguments against both 1 and 2 so far. |
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. |
@ciampo What are the issues here? I don't think I've noticed any. |
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. gutenberg/packages/editor/src/components/post-actions/index.js Lines 23 to 26 in bcf2b35
Ariakit doesn't have this ergonomics problem because you import the entire package as Practically speaking, since we already have an entire library where 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. |
I used shadcn/ui which uses option 3 in a side project. I didn't find the verbosity to be a problem. |
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 |
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. |
While waiting for @DaniGuardiola to share his thoughts, I basically came to the same conclusion as @mirka 's:
I don't really see valid alternatives to keeping We will have to find a naming convention for the top-level subcomponent, though — common choices are |
Why do we need a naming convention? |
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. |
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 |
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 |
@ciampo Can you give examples of cases where we need both |
@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 :) |
@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. |
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 Want the new, composable version? Use |
I just don't like |
One other downside is that for new components that don't exist, we'll be artificially forced to use |
The need for (Tangentially, we have in fact considered a |
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. |
Why?
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:
If you intend to insist on this point (which is fine), I would appreciate a response.
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 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. |
I think you misunderstood my latest comment. I meant: I still think it's wrong to force a Now for the why, I explained above that if I'm starting from scratch, the ideal name for a |
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
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.
I hope that this can share more context, and make our thoughts more clear and easier to follow. |
In case it helps, here are a few reasons why Ariakit chose
|
@diegohaz Thank you for adding more context! Personally I think that
That's a good observation, and in our case it would be true —
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 Especially for folks who expressed an opinion against the proposed convention (dot notation, 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. |
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 We will basically be able to pick between Option 1 (dot notation) and Option 3 (flat names), without the need for the 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. |
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:
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. |
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 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. |
@mirka good point about 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 |
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 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 |
@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 |
Sure, I'll file this as a separate issue for discussion in the future. I don't consider it blocking for this discussion. |
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 #63714, and we'll be able to mark this issue as closed. |
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
Option 2: Dot notation (either by exporting an object or by using single named exports)
Option 3: "flat" namespacing (ie. without dot notation)
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.):
The text was updated successfully, but these errors were encountered: