Renaming the generated `hlm-` selector to anything else breaks the `select` component

alexciesielski commented 3 months ago

Please provide the environment you discovered this bug in.

Any environment with select

Which area/package is the issue in?

select

Description

As already mentioned in Discord I like to make use of the fact that I have full control over the generated components in my repository and like to change the hlm prefix to my customer's prefixes.

Unfortunately this breaks the select component because the brn part specifically selects for both a label[hlmLabel] and hlm-select-trigger here.

I argue that this approach is architecturally and conceptually incorrect, because it's not only a (conceptually) circular dependency, but also tightly couples the hlm implementation to the brn part.

While, if I understand correctly, brn is supposed to be a headless standalone collection of directives and components and should be unaware of where/how it's used.

Proposed solution

Again, as already mentioned in Discord, my proposed solution would be to decouple hlm from brn and instead of using ng-content with a select I would instead opt for the (while more verbose) more explicit way of passing a template reference either through an @Input or (as proposed in Discord) through a @ContentChild.

Since we're still in alpha I would even suggest taking the dive and making this a breaking change and completely removing any mention of hlm inside brn, although I think it should be possible to keep the current solution while adding support for selecting the label and trigger through ContentChild.

I would even go as far as suggesting adding a eslint rule for every brn library that would consist of a regex looking for hlm throughout its code and erroring on that.

Please provide the exception or error you saw

No response

Other information

Happy to open a PR with the suggested solution of tagging the (hlm) label and trigger components with a directive and querying it in the BrnSelect component using @ContentChild

I would be willing to submit a PR to fix this issue

[X] Yes
[ ] No

goetzrobin commented 3 months ago

Let's do this. You're absolutely correct that brain shouldn't functionally depend on hlm. I like your proposed solution and that should also allow us to use the new APIs in 17.3 if you want.

alexciesielski commented 3 months ago

As discussed in Discord my initial proposal is not as elegant as using ng-content, so instead we decided to move the template into the hlm part. Which means refactoring the brn-select, brn-select-content, brn-select-value components into directives, which means they can't use the ViewChild and ContentChild queries anymore, which means more refactoring etc. etc... So basically it wouldn't be possible anymore to use <brn-select> in the HTML as it wouldn't be a component anymore.

Which opens another can of worms: How much template/UI-logic should even exist inside brn?

The more I think about it, the more I come to the conclusion that ideally brn would only consist of helpers and directives, basically just logic and leave the whole UI part to hlm (or any other implementation building on top of brn)... which would mean a lot of refactoring and a lot of "raising" components up from brn to hlm. But before I embark on this journey I want to get some feedback of you guys first.

Also tagging @thatsamsonkid and @elite-benni as you guys are also deeply involved in the library.

alexciesielski commented 3 months ago

Although if I continue that thought brn would be nothing more than a cdk with some code scaffolding. I guess my confusion stems from the lack of a clear idea in my mind of what brn is supposed to be...

I will continue converting the brn select components to directives for now, moving the templates to hlm, and once that is materialized then I guess the picture should become clearer.

thatsamsonkid commented 3 months ago

I will be honest this is a lot more changes than I was expecting. So overall this kind of transition is a very slippery slope for us from a library maintenance perspective. To be clear I'm not against your objective here which is, from the helm perspective be able to make all of the customizations you could ever want to it and it still continue to work properly, especially in regards to selector renaming for example. However I don't believe this is the way for the reasons below. (Apologize for the long read and also this is only my view, I don't wish to speak for @goetzrobin and @elite-benni )

The UI Library Paradigm (as I understand it at least)

Brn: is meant to still be out of the box a functioning component, unstyled but functioning and not just a set of pieces left to the developer to figure out how to use to make a working component, even if documented.

Hlm: is meant to be low logic and primarily focused on applying the shadcn skin and not much else.

Why Hlm has low logic and should continue this way:

Hlm is in the full control of the end user, moving all the logic there while sure gives great customizability to the end user also has a lot of downsides in terms of maintainability and upgradability of this library.

People will feel empowered to make customizations and if something breaks we will see them in our issues log asking us to tell them why something is not working which will make it very difficult to maintaining this library. Blackboxing the majority of the logic in Brn helps with maintenance and longevity of this project
Getting users to upgrade versions will be very difficult if we were to do this and in some extremes they simply will never upgrade versions for a component and maybe just continuously apply upgrades manually. It will also inhibit us from ever making automatic upgrades/migrations most likely an impossibility at that point, if lets say anything related to the shadcn style or behavior of a component were to change.
We should be consistent in terms of where the divide is in terms of where the logic lives. This is hard because its not clear cut in some cases but I think we are striving to limit the amount of logic we do keep in hlm with the goal of having none if possible.

Why helm selectors were placed in select brn:

The decision for having the helm selectors in the brn component was not a design decision that was taken lightly. I ran into very much the same issues you are running into yourself. Most alternative solutions ultimately resulted in a much more verbose API. Ultimately we are limited in what Angular allows us to do but our ultimate goal was to provide the best API possible and having the helm selectors in brn, while not ideal, is ultimately not a hard dependency and seemed reasonable compared to the other options. There is currently no hard requirement for hlm to be present at all for brn component to function.

Closing words

Now I'm not saying there is no solution for customizing the helm selector, I feel like there should be a way for us to provide a slightly different alternative if there really is a desire to customize the helm selector, but ultimately I believe we should support both because that alternative will definitely be a much more verbose API and IMHO I feel greater amount of people would prefer a simpler API over the ability to customize the selector, which is the only downside with current implementation I see.

Again I really want to support the helm selector customization, I really hate that the select component right now is the only component in the library that seems to have this shortcoming at the moment.

alexciesielski commented 3 months ago

Thanks for the write-up.

I think it is important to clarify these assumptions and expectations and define them in a central place, probably best in the Intro/Getting-started section of the documentation, so these questions don't keep popping up in the future, as I'm sure I'm not the only one looking for a definition of what brn, and what hlm really is supposed to be.

For a bit of context of what my motivations are: in a customer's project we are using Angular Material components for their superb API, there is no other component library which has such a good and well though out API. Still, the fact that it is so opinionated, not only from a styling perspective, but also from an extensibility perspective caused us to write a lot of additional code to "make it ours". This has become a huge pain to maintain, especially when upgrading versions because each Angular Material update means checking the whole app for regressions. Which is why, after having read a lot about headless component libraries in React, I embarked on a quest to find a way to build a component library that we have full control over both in terms of templating and styling.

I really like the idea of shadcn, which is why I'll copy the part I most liked from it's Introduction docs:

This is NOT a component library. It's a collection of re-usable components that you can copy and paste into your apps. ... Pick the components you need. Copy and paste the code into your project and customize to your needs. The code is yours. Use this as a reference to build your own component libraries.

This clicked with me most importantly because of this part: The code is yours.

I want for our project to have full control over the library that we will be building, and my vision for such a library is to have as great an API as Material has without compromises.

Meaning that while looking at the refactoring mentioned above I backtracked from my initial idea of defining additional directives just for brn-select to be able to content-project the hlm-trigger and value components.

We both agree that it's unnecessarily more verbose when ng-content does the trick just fine. But that obviously only works if we can control the selectors.

I get the idea of upgradibility and maintainability and that it will be a pain to apply upgrades to the generated code files - this was a big question mark for me when reading more about shadcn. How would this be solved in a somewhat automatic manner?

But if the decision has to be made between either full control of code, or easier upgrading in the future, I vote full control. Once the component library stands and is in use and is proven to work there is little reason to make additional changes to it, other than applying bugfixes and/or adding accessibility features.

Which is why I claim that upgradibility is less important (to me).

So I guess the final question becomes: what is spartan supposed to be?

An Angular reimplementation of the ideas that shadcn has brought forward?
Or to be a (closed) headless component library and the only inspiration it takes from shadcn is its theming for hlm?

I don't see how it's possible to marry the ideas of shadcn of giving full control, while building a complete and functioning headless library that is npm-installable.

Edit: another thought that came to mind in regards to your statement

Brn is meant to be ... not just a set of pieces left to the developer to figure out how to use to make a working component, even if documented.

But that's exactly the beauty of the code scaffolding commands - there isn't really anything to figure out for anybody if they want to use the components differently than anticipated by spartan. They get a full set of working components, which may be styled already, but there is nothing in the way of just removing the Tailwind classes if they prefer an unstyled version. In fact, the generators could be adjusted to generate fully working components that do not contain any styling initially.

And as a lighthearted end-note - if we're to take the naming convention seriously, then I wouldn't want my brain to be exposed uncovered to the outside world - I would prefer having a helm(et) in between.

Which is to say the brains are gonna be wrapped in a helmet library anyway, so why not separate the looks from the brain completely?

goetzrobin commented 3 months ago

So this is a tough one. I don't consider it a hard dependency on helm. It's more of a quality of life improvement for those using the hlm scaffolded components.

I just read this article: https://angular.io/guide/content-projection

We can use the ngProjectAs attribute to allow for custom components.

We are absolutely limited by Angular here. I tried using the host property inside the Angular @Component to apply the ngProjectAs, but it did not work. I do believe that it should though: https://github.com/angular/angular/issues/24058

We wouldn't use a dynamic attribute, but a static one.

Here is more info on this issue too. https://github.com/angular/angular/issues/44755

Here is an example of it working with ngProjectAs

export const CustomTrigger: Story = {
    render: (args) => ({
        props: { ...args },
        moduleMetadata: {
            imports: [CustomSelectTriggerComponent],
        },
        template: /* HTML */ `
            <hlm-select class="inline-block" ${argsToTemplate(args, { exclude: ['initialValue'] })}>
                <custom-select-trigger ngProjectAs="[brnSelectTrigger]" class="w-56">
                    <hlm-select-value />
                </custom-select-trigger>
                <hlm-select-content>
                    <hlm-select-label>Fruits</hlm-select-label>
                    <hlm-option value="apple">Apple</hlm-option>
                    <hlm-option value="banana">Banana</hlm-option>
                    <hlm-option value="blueberry">Blueberry</hlm-option>
                    <hlm-option value="grapes">Grapes</hlm-option>
                    <hlm-option value="pineapple">Pineapple</hlm-option>
                </hlm-select-content>
            </hlm-select>
        `,
    }),
};

@Component({
    selector: 'custom-select-trigger',
    standalone: true,
    imports: [BrnSelectTriggerDirective, HlmIconComponent],
    providers: [provideIcons({ lucideChevronDown })],
    template: `
        <button [class]="_computedClass()" #button brnSelectTrigger type="button">
            <ng-content />
            @if (icon) {
                <ng-content select="hlm-icon" />
            } @else {
                <hlm-icon class="ml-2 h-4 w-4 flex-none" name="lucideChevronDown" />
            }
        </button>
    `,
})
export class CustomSelectTriggerComponent {
    @ViewChild('button', { static: true })
    public buttonEl!: ElementRef;

    @ContentChild(HlmIconComponent, { static: false })
    protected icon!: HlmIconComponent;

    public readonly userClass = input<ClassValue>('', { alias: 'class' });
    protected readonly _computedClass = computed(() =>
        hlm(
            '!bg-sky-500 flex h-10 items-center justify-between rounded-md border border-input bg-background px-3 py-2 text-sm ring-offset-background placeholder:text-muted-foreground focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50 w-[180px]',
            this.userClass(),
        ),
    );
}

What are your thoughts? I think if using ngProjectAs would work if applied as a static host field this would resolve our issue. Do you think it's worth opening an issue with the Angular team?

goetzrobin commented 2 months ago

I think it's worth opening an issue. The reproduction is very simple and I have played around with it and to a developer there is no reason it should not work with the static host attribute. Maybe the Angular team has other ideas of working around the issue. What do you think?

thatsamsonkid commented 2 months ago

I think it's worth opening, I think maybe they could give us an idea of maybe an alternative or workaround and in general I think this could be useful for us in some other places as well in the library if resolved

alexciesielski commented 2 months ago

When you open it, can you link it with this issue for reference please?

goetzrobin commented 2 months ago

I opened one: https://github.com/angular/angular/issues/55438 I forgot to link it. But will add it now

goetzrobin / spartan