fix(ui): camelCase to kebab-case (#970)

[angsherpa456]

Objective

Ensure that all property names of our custom elements are reflected as kebab-case HTML attributes in the DOM.

Solution: Simple inheritance & LitElement overrides

We created a base class BoilerLitElement that extends LitElement and overrides the static createProperty() method. This custom createProperty() method automatically converts the name of any class members marked with the @property decorator to a kebab-cased attribute in HTML. All our custom elements now extend LitElementCustom instead of LitElement.

This solution is simple, light-weight & works well with TypeScript's class inference features.

Dropped approaches

We investigated other solutions to approach this issue but decided to drop them due to complexity or TypeScript incompatibility.

Class mixin approach (dropped)

Class mixins as proposed in Lit's documentation don't play well with TypeScript currently. They work partially and with workarounds but would introduce a lot of complexity to make them adaptable and future proof for our project. It might still be worth it to investigate the mixin pattern in the future but it is out of scope for this issue.

References:

• https://lit.dev/docs/composition/mixins/
• https://www.typescriptlang.org/docs/handbook/mixins.html
• https://www.typescriptlang.org/docs/handbook/mixins.html#alternative-pattern (Not investigated)
• False error TS4094: ... exported class expression may not be private or protected. microsoft/TypeScript#30355

This is as far as we got during research

import { LitElement, PropertyDeclaration } from 'lit';

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type Constructor<T> = new (...args: any[]) => T;

export type Constructable<TConstructorResult, TConstructorHost> = Omit<TConstructorHost, never> &
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  Constructor<TConstructorResult>;

// Have to declare an exported class interface to add additional properties to our mixin class.
// Results in non-ignorable error TS4094 otherwise.
export declare class AdditionalMixinProperties {
  static staticMember(): void;
  protected protectedMember(): void;
}

export const camelCaseToKebabCase = (str: string) => str.replace(/([a-z0-9])([A-Z])/g, '$1-$2').toLowerCase();

// The mixin factory function
export function kebabCaseAttributes<T extends Constructable<LitElement, typeof LitElement>>(constructor: T) {
  class KebabCased extends constructor {
    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    constructor(...args: any[]) {
      super(...args);
    }

    static createProperty(name: PropertyKey, options?: PropertyDeclaration<unknown, unknown> | undefined): void {
      let customOptions = options;

      // generate the attribute name if it hasn't been defined or if it's disabled.
      if (typeof options?.attribute === 'undefined' || options?.attribute === true) {
        const attributeName = camelCaseToKebabCase(name.toString());
        customOptions = { ...options, attribute: attributeName };
      }

      return super.createProperty(name, customOptions);
    }
  }

  return KebabCased as Constructable<InstanceType<T> & AdditionalMixinProperties, T & typeof AdditionalMixinProperties>;
}

// Need to create an explicitly named intermediate class to extend from.
// Otherwise: 
// - static member inference won't work on the subclass.
// - Error TS4094 | Property ... of exported class expression may not be private or protected.
// This needs to be done for every class definition that wants to extend any number of mixins.
export class MixinBase extends kebabCaseAttributes(LitElement) {}

// Extend ^ instead of `kebabCaseAttributes(LitElement)` directly
export class CustomElement extends MixinBase {
  // access to iherited protected, private and static members works
}

This solution works on almost all fronts: proper type inference and autocompletion for statics and non-statics in mixin class implementation as well as classes extending the mixin.
It needs more polish to make it easier to adapt for other future mixins. Eg. offer a utility function that creates mixin classes and does the proper type conversion through inference (if possible). It also has the caveat of having to create explicitly named intermediate classes as commented in the code above.

Setting the attribute option in the @property() decorator

We discussed going the route of explicitly naming our attributes through the @property({ attribute: 'some-attribute-name' }) syntax but decided against it since the current solution is succinct and easy to undo should we decide against it in the future.

closes #970

[ChristianHoffmannS2]

cool stuff :)

[faselbaum]

In addition to the other comments we could think about adding an import restriction rule to our ui-library/.eslintrc.js to prevent ourselves from accidentally using the standard LitElement.

// .eslintrc.js
module.exports = {
  ...
  rules: {
    'no-restricted-imports': [
      'error',
      {
        patterns: [
          {
            group: ['lit'],
            importNamePattern: 'LitElement',
            message: `Don't use the default LitElement class. Import from /utils/boiler-lit-element instead`,
          },
        ],
      },
    ],
  },
  ...
}

[Jumace]

This is a neat solution for the problem of the attributes.

Found three things over all.

• leftover has-Error instead of hasError
• the used regEx for creating kebab-case is a bit flawed, posted an alternative solution
• Somewhere in the documentation it should be noted that only the LitElementCustom should be used

[angsherpa456]

"Somewhere in the documentation it should be noted that only the LitElementCustom should be used"
Now the information is added in the architecture section.

[faselbaum]

What do we do about properties like these?

boiler/packages/ui-library/src/components/input-field-text/index.ts

Line 47 in 5679303

@property() arialabel!: string;

They aren't camelCased even though they probably should be. But this might also produce unwanted behaviour with screen readers when kebab-cased. I presume we usually only use them to forward the values to elements further down the tree? We should probably investigate how screen readers behave when aria attributes are duplicated on the custom element and within the shadow dom of said element.

This might lead to screen readers reading the label or whatever twice.

[Jumace]

For the comment of @faselbaum I'd suggest to cover this in a new ticket. From my understanding should the current implementation not break anything, only if attributes like the arialabel would be written in camelCase it could break.
If you agree with my statement please get in touch with @thrbnhrtmnn to create a new ticket.

[ChristianHoffmannS2]

What do we do about properties like these?

boiler/packages/ui-library/src/components/input-field-text/index.ts

Line 47 in 5679303

@property() arialabel!: string;

They aren't camelCased even though they probably should be. But this might also produce unwanted behaviour with screen readers when kebab-cased. I presume we usually only use them to forward the values to elements further down the tree? We should probably investigate how screen readers behave when aria attributes are duplicated on the custom element and within the shadow dom of said element.

This might lead to screen readers reading the label or whatever twice.

a few things here.

• property names should not match default html attribute names. (thats why it never was camelcased in the first place, it warns us against that)

• the input field is connected to a real label by id. i dont see a need for an additional arial label. and i am also not sure how accessibility tools would choose between those two label options.

i would ignore it for now, we will definitely stumble on this one during the accessibility epic.

[faselbaum]

The js-example-app needs to be updated to reflect the changes as well since we're not using the genericBlrComponentRenderer function there.

[angsherpa456]

The js-example-app needs to be updated to reflect the changes as well since we're not using the genericBlrComponentRenderer function there.

True, some of them are broken currently.