lit-element icon indicating copy to clipboard operation
lit-element copied to clipboard

[feature request] create an listen decorator for host listeners

Open e111077 opened this issue 3 years ago • 5 comments

Description

  • Who wants the functionality
    • people who want to declaratively add listeners to host
  • What it is they want
    • A @listen decorator to declaratively add event listeners to host
    • I don't necessarily think a "listeners" block should need exist for JS. A helper for TS is sufficient I think.
  • Why they want it
    • it's declarative
    • helpful for serialization
    • having to deal with unregistering listeners on disconnect
    • having to deal with possibly .bind and listener references is annoying
  • Functional description of task/subtask
    • Create a new decorator called @listen

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.

e111077 avatar Mar 03 '21 22:03 e111077

@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 avatar Mar 04 '21 07:03 jpzwarte

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);
    };
  };
}

jpzwarte avatar Mar 04 '21 15:03 jpzwarte

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.

justinfagnani avatar Mar 12 '21 16:03 justinfagnani

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 avatar Mar 12 '21 19:03 e111077

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) {...}
}

e111077 avatar Mar 27 '21 02:03 e111077

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

sorvell avatar Sep 08 '22 21:09 sorvell