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

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

Open
lucaelin opened this issue Apr 27, 2022 · 7 comments · May be fixed by #126
Open

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

lucaelin opened this issue Apr 27, 2022 · 7 comments · May be fixed by #126

Comments

@lucaelin
Copy link

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

43081j commented Apr 29, 2022

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

lucaelin commented Apr 30, 2022

Ah my bad, I see! This is why I was confused when seeing different results when trying to reproduce 😅
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
Copy link
Owner

43081j commented May 1, 2022

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

lucaelin commented May 1, 2022

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

43081j commented May 3, 2022

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

lucaelin commented May 3, 2022

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

43081j commented May 3, 2022

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

@lucaelin lucaelin linked a pull request May 3, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants