open-wc / form-participation

Helper, mixins, tools, and more for supporting your custom elements in participating in an ancestor form.
MIT License
53 stars 7 forks source link

FormControlMixin causes build failures on server/Lit SSR #48

Closed hrmcdonald closed 1 year ago

hrmcdonald commented 1 year ago

The calls to addEventListeners in the element's constructor break Lit SSR compatibility.

Could these be moved to the connectedCallback?

connectedCallback() {
  super.connectedCallback();

  this.addEventListener?.('focus', this.#onFocus);
  this.addEventListener?.('blur', this.#onBlur);
  this.addEventListener?.('invalid', this.#onInvalid);
}

Alternatively, perhaps provide a method we can override to inform the mixin if it is being rendered on a server at the moment but in a library agnostic way:

protected isServer() {
  return false;
}

constructor(...args: any[]) {
  super(...args);

  if (!this.isServer()) {
    this.addEventListener?.('focus', this.#onFocus);
    this.addEventListener?.('blur', this.#onBlur);
    this.addEventListener?.('invalid', this.#onInvalid);
  }
  this.setValue(null);
}

So then in a Lit element we could just override:

import {isServer} from 'lit';

// ...

  protected isServer() {
    return isServer;
  }
hrmcdonald commented 1 year ago

Any thoughts on this @calebdwilliams ? We'd be happy to put in a PR for either direction.

calebdwilliams commented 1 year ago

We probably don’t need the is server. Just the optional chaining. Happy to accept that PR; more so if there’s a basic test using @lit-labs/testing.

hrmcdonald commented 1 year ago

Ah, yup, that makes way more sense. We'll look into getting something added soon.

calebdwilliams commented 1 year ago

@hrmcdonald I believe this should be resolved, can you confirm?

hrmcdonald commented 1 year ago

Yes, this is working for us now! You can close.