Overriding templates does not work anymore w/ colocated addon templates

[simonihmig]

In #1221 we refactored the ember-bootstrap components internally to use template colocation.

While this first seemed like a non-breaking change (all tests passing), it seems that the usual way of overriding template of an addon in an app does not work anymore. Idk exactly how colocation is implemented, but looking at the RFC, specifically the build-time section, I read this as a way to tightly bundle class and template together. So putting a template with the same name as the addon's template into the app does not work anymore to override it. Seems the only way to do it is by explicitly calling setComponentTemplate as in this example:

import BaseAlert from 'ember-bootstrap/components/bs-alert';
import layout from './template';

export default setComponentTemplate(layout, class Alert extends BaseAlert {});

In ember-bootstrap we explicitly told users that extending from classes is not supported anymore (to better allow internal refactorings, e.g. to Glimmer components), but we did not mention anything specific with regards to templates. So not sure if this can/must be considered an (accidental) breaking change? 🤔

/cc @jelhan

[jelhan]

To be honest I would consider overwriting a template of a component as a bad practice. It's even worse than extending it's class. I wouldn't consider either as supported. I have used that pattern myself in the past but it was a maintenance nightmare. I would love to support all needed customization through public API so that a consumer can customize as they need by a wrapping component instead.

Regarding the public API I would only consider invoking the component in a template as our public API. So neither extending a class nor overwriting a template.

And I would also push for that pattern in our internal composition of components. This is what I had in mind here: #1022 (comment)

[simonihmig]

Regarding the public API I would only consider invoking the component in a template as our public API. So neither extending a class nor overwriting a template.

Yeah, I agree. Everything else makes maintenance extremely difficult. I just wanted to bring this up, as people (including me and you) have done so in the past, and we haven't explicitly stated anything about the template part. Maybe we should mention that somewhere, maybe alongside the don't-extend-classes warning!?

[fran-worley]

Whilst I totally agree with your conclusions on this, We've just run into this issue as we had overridden the element and label templates to provide the required attribute to the label component so we can include a visual indicator for required fields.

Since the move to html attributes on the control this 'hack' has been necessary and no longer works! Can you suggest an alternative solution to getting 'required' into the label?

You suggest that overriding the class is bad... what's the official way of supporting validation addons as ember-bootstrap-changeset-validations seems to do just this...

[simonihmig]

Can you suggest an alternative solution to getting 'required' into the label?

That's a valid concern, but I am afraid I have no good answer atm. 🤔

Like you could override the whole label component for example, by basically implementing your own component class and template, without extending or replacing just the template. But for a use case that is so common as yours, this doesn't seem like an acceptable solution really...

We could introduce new arguments to form.element like @labelClass, but this seems on the other hand like a step backwards again, as we wanted to get rid of arguments that just apply HTML attributes. Unfortunately there is no way apply attributes to anything but the "default" element (which hash ...attributes) in Ember.

The form API is really not as modular and customizable as it probably should, just as it had been around for so many years without any substantial changes. Like it hides the way components are combined and rendered, i.e. you cannot do something along the lines of:

<form.element ... as |el|>
  <el.label class="required">Email</el.label>
  <el.control />
</form.element>

There have been ideas floating around to rethink all of this, and probably create something not coupled to Bootstrap as the foundation. @jelhan was interested in this as well I believe. But as always there is just so much time...

what's the official way of supporting validation addons as ember-bootstrap-changeset-validations seems to do just this...

Yes, you are right, they do just that, mostly because we haven't yet come up with an API to integrate validation libraries by something other than extending classes (also related to that general form library idea). For now these two addons (´ember-bootstrap-changeset-validationsandember-bootstrap-cp-validations`) are considered privileged in that they continue to extend from base classes (which is something like using a private API), but we will make sure that once there are changes in ember-bootstrap that would cause things to break (e.g. moving to Glimmer components) we will immediately update those addons to continue working.

[jelhan]

The form API is really not as modular and customizable as it probably should, just as it had been around for so many years without any substantial changes. Like it hides the way components are combined and rendered, i.e. you cannot do something along the lines of:

<form.element ... as |el|>
  <el.label class="required">Email</el.label>
  <el.control />
</form.element>

There have been ideas floating around to rethink all of this, and probably create something not coupled to Bootstrap as the foundation. @jelhan was interested in this as well I believe. But as always there is just so much time...

Yieldable named blocks may be a good fit for that scenario. Not having much experience with them yet. But if I got it right, they should allow us to provide a public API like the following:

<form.element>
  <:label as |label|>
    <label class="required">Email</label>
  </:label>
  <:control as |control|>
    <control disabled />
  </:control>
</form.element>

It should also allow you to skip named blocks and use the default implementation instead. So if you don't need to customize the control it should also work like this if I got it right:

<form.element>
  <:label as |label|>
    <label class="required">Email</label>
  </:label>
</form.element>

We should be able to still support the existing API for consumers that do not need that level of customization.

<form.element />

<form.element as |el|>
  <el.control />
</form.element>

The polyfill covers all Ember versions that we need. So we should be able to add that feature without requiring a new major.

[fran-worley]

I like this idea, but it is still a little 'clunky' for something that IMHO should be easy to do. Having to set required on the control and then write my own label from scratch to put a class on it starts to make my form very long with a greater risk of error (therefore even more tests being required!)...

In theory, if the 'is-required' class remained on the FormGroup (see #1261) then I could handle everything via CSS without needing to override the label component at all.

I worry though that without formal documentation,I am essentially relying on another feature which is pending deprecation/ removal...

[fran-worley]

The other solution would be to allow 'custom' label components like you do for controls. As we use FontAwesome 5, rather than relying on CSS styling, we inject an icon tag into the label:

{{#if @requiredLabel}}
  <span class="sr-only">Required</span>
  <FaIcon @icon="star" @size="xs" data-test-form-element-required-icon />
{{/if}}

Declaring this for every required label in every form is a little unpleasant but if we could instead do <form.element labelType="requiredLabel"> that would be quite a bit cleaner.

[jelhan]

I like this idea, but it is still a little 'clunky' for something that IMHO should be easy to do. Having to set required on the control and then write my own label from scratch to put a class on it starts to make my form very long with a greater risk of error (therefore even more tests being required!)...

I can't follow to be honest. The proposed API would look like this:

<BsForm as |form|>
  <form.element @label="username">
    <:label as |label|>
     <label>
       {{label.text}}
       <span class="sr-only">Required</span>
       <FaIcon @icon="star" @size="xs" data-test-form-element-required-icon />
     </label>
    </:label>
    <:control as |control|>
      <control required />
    </control>
  </form.element>
</BsForm>

If you want to share the required feature on label you can put it on a component, which you use as labelComponent. On such custom components you can support additional arguments. It would look like these:

<BsForm as |form|>
  <form.element @label="username" @labelComponent={{component "my-label-component"}}>
    <:label as |label|>
      <label @required={{true}} />
    </:label>
    <:control as |control|>
      <control required />
    </:control>
  </form.element>
</BsForm>

I see that this would be easier with a @required argument on <BsForm::element>, which is than passed to the yielded control and label component. We had a @required argument in v3. But we dropped it in v4 as it was only used to set the required HTML attribute on the control.

Wondering if there is any way to allow consumers to add such additional arguments on <BsForm::element>. That would be very powerful for customization.

[jelhan]

I'm closing this now. We migrated to colocated templates long time back. Seems as if it is working fine for consumers.