lit / lit-element

LEGACY REPO. This repository is for maintenance of the legacy LitElement library. The LitElement base class is now part of the Lit library, which is developed in the lit monorepo.
https://lit-element.polymer-project.org
BSD 3-Clause "New" or "Revised" License
4.49k stars 319 forks source link

[feature request] create an listen decorator for host listeners #1165

Closed e111077 closed 1 year ago

e111077 commented 3 years ago

Description

Acceptance criteria

What the card must do in order to accept it as complete. Acceptance Criteria must be concrete or measurable. A TS decorator to add event listeners to host.

jpzwarte commented 3 years ago
@listen('click')
@listen('click', { target: 'document' })
@listen('resize', { target: 'window' })

Not sure whether you'd want to allow addEventListener options here. Since there is already @eventOptions

jpzwarte commented 3 years ago

My attempt at this:

type EventHandler = (event: Event) => boolean | void;

interface CustomElement extends Element {
  connectedCallback: () => void;
  disconnectedCallback: () => void;
}

export function listen(eventName: string) {
  return (target: CustomElement, key: string): void => {
    const { connectedCallback, disconnectedCallback } = target;

    let eventHandler: EventHandler;

    target.connectedCallback = function () {
      connectedCallback.call(this);

      // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access
      eventHandler = (event: Event) => (this as any)[key](event);
      this.addEventListener(eventName, eventHandler);
    };

    target.disconnectedCallback = function () {
      this.removeEventListener(eventName, eventHandler);

      disconnectedCallback.call(this);
    };
  };
}
justinfagnani commented 3 years ago

We've considered this before and haven't done it yet mainly because this.addEventListener() in the constructor works pretty well already for host listeners.

Where we've added decorators so far have been cases where we need to transform an existing JS construct (@property and @internalProperty) or we want a declarative form for analysis (@customElement) or a declarative form to have a spot to hang type information (@query and friends).

I'm not sure if @listen() fits those or not, in that we don't need to transform the method, need type info on the method, or analyze it. It's "just" sugar. Sugar is a fine reason to add something sometimes though, if we can come up with a form that fits enough of the use cases. The one I find most compelling is listeners not on the host that need to be removed in disconnected to prevent memory leaks. This case requires hooking lifecycle callbacks.

Since we already had a controller API in lit-next that can hook lifecycles, it seems like all we need to allow @listen() to be implemented without base class support or patching is a way to add initializer hooks to the class, which can in turn add controllers to instances. I filed https://github.com/Polymer/lit-html/issues/1663 to track this, and @sorvell is working on it.

e111077 commented 3 years ago

They're also good to prevent spots where anti-patterns may arise like memory leaks from element references due to listeners or when I was speaking to a community member about the @watch directive they created, calling it after super.update which may cause unnecessary rerenders.

This not to mention that internally we aren't even allowed to use .bind and must rewrap all of our method references in () =>{} and pass ags and return values

e111077 commented 3 years ago

Coming back to this because of the frustration the following pattern gives me:

@customElement('my-element')
class MyElement extends LitElement {
  private boundOnClick = (_e: MouseEvent) => {}; // hopefully some type that satisfies addEventListener in TS

  connectedCallback() {
    super.connectedCallback();
    this.boundOnClick = this.onClick.bind(this);
    // internally at google the line above has to be
    // this.boundOnClick = (e: mouseEvent) => (this.onClick(e));
    // because we are not allowed the use of `bind`
    this.addEventListener('click', this.boundOnClick, {capture: true});
  }

  disconnectedCallback() {
    super.disconnectedCallback();
    this.removeEventListener('click', this.boundOnClick);
  }

  onClick(e: MouseEvent) {...}
}

as opposed to something like:

@customElement('my-element')
class MyElement extends LitElement {
  @eventOptions({capture: true}) // tbh not sure if the implementation of eventOptions would allow this
  @hostListener('click', {removeOnDisconnect: true, bind: true}) // these likely could be default
  onClick(e: MouseEvent) {...}
}
sorvell commented 1 year ago

Closing in favor of https://github.com/lit/rfcs/issues/11 since this should be included and/or covered by that proposal.