43081j / eslint-plugin-lit

lit-html support for ESLint
116 stars 21 forks source link

lit/no-template-bind is not correct about auto-binding? #125

Open lucaelin opened 2 years ago

lucaelin commented 2 years ago

First of all: Thank you so much for all your work and effort! I've noticed that this plugin considers .bind-calls inside template-expressions an error, claiming that lit would auto-bind those. Trying to fix my projects unnecessary binding, I noticed that auto-binding seems to no longer be performed by lit (at least in version 2 it isn't).

I've created a small project to replicate the error and a working use of the directive in a property-binding: https://github.com/lucaelin/lit-lint-issues/blob/main/issue-template-bind.js (please ignore the async-directive stuff, as it is related to a different issue in a different project)

This change seems to also affect the no-template-arrow rule, as it is build around the same assumption.

Kind regards!

43081j commented 2 years ago

In your example, you don't use lit element, but lit directly.

Lit 2.x does automatically bind if using LitElement, otherwise you have to set the host when rendering.

With LitElement:

class MyElement extends LitElement {
  // ...
  render() {
    return html`
      <div @click=${this._onClick}></div> <!-- THIS IS AUTOMATICALLY BOUND ->
    `;
  }
}

With lit directly:

class MyElement extends HTMLElement {
  connectedCallback() {
    render(html`
      <div @click=${this._onClick}></div>
    `, this, {host: this}); // <- `host` means all event handlers are bound to `this`
  }
}
lucaelin commented 2 years ago

Ah my bad, I see! This is why I was confused when seeing different results when trying to reproduce :sweat_smile: But I think that this not working for property-binding as well - I could only confirm that auto-binding happens when using event-handlers in LitElement.

class MyElement extends LitElement {
  logThis() {
    console.log(this);
    if (!(this instanceof MyElement)) {
      alert("this not instanceof MyElement");
    }
  }
  indirectLogThis() {
    this.shadowRoot.querySelector("some-element").logThis();
  }
  render() {
    return html`
      <some-element .logThis=${this.logThis}></some-element>
      <button @click=${this.indirectLogThis}>.logThis=logThis</button>
    `;
  }
}

Am I still missing something? I noticed this being an issue when using 3rd-party components that require passing a function via properties...

43081j commented 2 years ago

Lit doesn't know or care what you're passing in that case as its just a data binding. If you pass a function, it'll get a function, etc.

In those cases its upto you to create a bound version of it, which you should do once in the constructor:

class MyElement extends LitElement {
  // ...
  constructor() {
    super();
    this._logThis = this._logThis.bind(this);

    // or
    this._logThisBound = this._logThis.bind(this);
  }

then if you pass .logThis=${this._logThis}, it'll be bound (or _logThisBound if you named it that)

lucaelin commented 2 years ago

Okay, that's what I thought. This does mean that the lit/no-template-bind is showing false positives on both property bindings and on template tags used outside of a LitElement context? I'd say it is causing more confusion for me and my team than it does good then... Thank you so much for your clarification! Maybe the documentation for this rule should include this or the rule should be improved?

43081j commented 2 years ago

it isn't showing false positives as you should bind those functions once in the constructor, not in your render method. otherwise, you'll be creating a function every time you render

when i get time i will add more examples to the docs

lucaelin commented 2 years ago

Oh, I see now! This rule is not really about the auto-binding, but about repeatedly binding a function in the LitElement render functions! The message that appears in the IDE is `.bind` must not be used in templates, a method should be passed directly like `${this.myMethod}` as it will be bound automatically, which led me to believe that this rule is to prevent binding from happening twice. I'll see if I can create a PR that improves the message.

43081j commented 2 years ago

ah thats a fair point. we should probably point out that its still valid to use bind but outside the render method, i.e. beforehand in the constructor or somewhere