Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
e111077 opened this issue Mar 3, 2021 · 6 comments
Closed

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

e111077 opened this issue Mar 3, 2021 · 6 comments

Comments

@e111077
Copy link
Contributor

e111077 commented Mar 3, 2021

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.

@jpzwarte
Copy link

jpzwarte commented Mar 4, 2021

@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
Copy link

jpzwarte commented Mar 4, 2021

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
Copy link
Contributor

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 lit/lit#1663 to track this, and @sorvell is working on it.

@e111077
Copy link
Contributor Author

e111077 commented Mar 12, 2021

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
Copy link
Contributor Author

e111077 commented Mar 27, 2021

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
Copy link
Member

sorvell commented Sep 8, 2022

Closing in favor of lit/rfcs#11 since this should be included and/or covered by that proposal.

@sorvell sorvell closed this as completed Sep 8, 2022
@sorvell sorvell moved this to ✅ Done in Lit Project Board Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

4 participants