Anti Patterns

Here are a few common anti-patterns which we've discovered as developers have used Catalyst. We consider these anti-patterns as they're best avoided, because of surprising edge-cases, or simply because there are easier ways to achieve the same goals.

Avoid doing any initialisation in the constructor

With conventional classes, it is expected that initialisation will be done in the constructor() method. Custom Elements are slightly different, because the constructor is called before the element has been put into the Document, which means any initialisation that expects to be connected to a DOM will fail.

import { controller } from "@github/catalyst"

@controller
class HelloWorldElement extends HTMLElement {
  constructor() {
    // This will fire before DOM is connected, so will never bubble!
    this.dispatchEvent(new CustomEvent('loaded'))
  }
}

import { controller } from "@github/catalyst"

@controller
class HelloWorldElement extends HTMLElement {
  connectedCallback() {
    // This will fire _after_ DOM is connected, so will bubble up as expected
    this.dispatchEvent(new CustomEvent('loaded'))
  }
}

Avoid interacting with parents, use Events where possible

Sometimes it's necessary to let ancestors know about the state of a child element, for example when an element loads or needs the parent to change somehow. Sometimes it can be tempting to use methods like this.closest() to get a reference to the parent element and interact with it directly, but this creates a fragile coupling to elements and is best avoided. Events can used here, instead:

import { controller } from "@github/catalyst"

@controller
class UserSettingsElement extends HTMLElement {
  loading() {
    // While this is loading we need to disable
    // the whole User if `user-profile` ever
    // changes, this code will break!
    this
      .closest('user-profile')
      .disable()
  }
}

<user-profile>
  <user-settings></user-settings>
</user-profile>

Instead of interacting with the parent's API directly in JS, you can use Events which can be listened to with data-action, this moves any coupling into the HTML which already has the association, and so subsequent refactors will have far less risk of breaking the code:

import { controller } from "@github/catalyst"

@controller
class UserSettingsElement extends HTMLElement {
  loading() {
    this.dispatchEvent(
      new CustomEvent('loading')
    )
  }
}

<user-profile>
  <user-settings
    data-action="loading:user-profile#disable">
  </user-settings>
</user-profile>

Avoid shadowing method names

When naming a method, you should avoid naming it something that already exists on the HTMLElement prototype; as doing so can lead to surprising behaviors. Test out the form below to see what method names are allowed or not:

Avoid naming methods after events, e.g. onClick

When you have a method which is only called as an event, it is tempting to name that method based off of the event, e.g. onClick, onInputFocus, and so on. This name implies a coupling between the event and method, which later refactorings may break. Also names like onClick are very close to onclick which is already part of the Element's API. Instead we recommend naming the method after what it does, not how it is called, for example resetForm:

import { controller } from "@github/catalyst"

@controller
class UserLoginElement extends HTMLElement {

  // `onClick` is not clear
  onClick() {
    // Log the user in
  }
}

<user-login>
  <!-- ... -->
  <button
    data-action="click:user-login#onClick">
    <!-- `onClick` is not clear -->
    Log In
  </button>
</user-login>

import { controller } from "@github/catalyst"

@controller
class UserLoginElement extends HTMLElement {

  login() {
    // Log the user in
  }
}

<user-login>
  <!-- ... -->
  <button
    data-action="click:user-login#login">
    Log In
  </button>
</user-login>

Avoid querying against your element, use @target or @targets

We find it very common for developers to return to habits and use querySelector[All] when needing to get elements. The @target and @targets decorators were designed to simplify querySelector[All] and avoid certain bugs with them (such as nesting issues, and unnecessary coupling) so it's a good idea to use them as much as possible:

class UserListElement extends HTMLElement {
  showAdmins() {
    // Just need to get admins here...
    for (const user of this.querySelector('[data-is-admin]')) {
      user.hidden = false
    }
  }
}

class UserList {
  @targets admins: HTMLElement[]

  showAdmins() {
    // Just need to get admins here...
    for (const user of this.admins) {
      user.hidden = false
    }
  }
}

Avoid filtering @targets, use another @target or @targets

Sometimes you might need to get a subset of elements from a @targets selector. When doing this, simply use another @target or @targets attribute, it's okay to have many of these! Adding getters which simply return a @targets subset has various drawbacks which make it an anti pattern.

For example let's say we have a list of filter checkboxes and checking the "all" checkbox unchecks all other checkboxes:

@controller
class UserFilter {
  @targets filters: HTMLInputElement[]

  get allFilter() {
    return this.filters.find(el => el.matches('[data-filter="all"]'))
  }

  filter(event: Event) {
    if (event.target === this.allFilter) {
      for(const filter of this.filters) {
        if (filter !== this.allFilter) filter.checked = false
      }
    }
    // ...
  }

}

<user-filter>
  <label><input type="checkbox"
    data-action="change:user-filter#filter"
    data-targets="user-filter.filters"
    data-filter="all">Show all</label>
  <label><input type="checkbox"
    data-action="change:user-filter#filter"
    data-targets="user-filter.filters"
    data-filter="new">New Users</label>
  <label><input type="checkbox"
    data-action="change:user-filter#filter"
    data-targets="user-filter.filters"
    data-filter="admin">Admins</label>
  <!-- ... --->
</user-filter>

While this works well, it could be more easily solved with targets:

@controller
class UserFilter {
  @targets filters: HTMLInputElement[]
  @target allFilter: HTMLInputElement

  filter(event: Event) {
    if (event.target === this.allFilter) {
      for (const filter of this.filters) {
        if (filter !== this.allFilter) filter.checked = false
      }
    }
    // ...
  }

}

<user-filter>
  <label><input type="checkbox"
    data-action="change:user-filter#filter"
    data-target="user-filter.allFilter"
    data-targets="user-filter.filters"
    data-filter="all">Show all</label>
  <label><input type="checkbox"
    data-action="change:user-filter#filter"
    data-targets="user-filter.filters"
    data-filter="new">New Users</label>
  <label><input type="checkbox"
    data-action="change:user-filter#filter"
    data-targets="user-filter.filters"
    data-filter="admin">Admins</label>
  <!-- ... --->
</user-filter>