43081j / eslint-plugin-lit

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

Added boolean attributes rule #129

Closed alicanerdogan closed 1 year ago

alicanerdogan commented 2 years ago

As also requested in the issue https://github.com/43081j/eslint-plugin-lit/issues/92, this PR adds the rule to accomplish the following:

Boolean attribute expressions (e.g ?hidden=${true}) should only be used for standard boolean attributes that are defined with the HTML standard.

With the boolean attribute expression, it is not possible to assign a falsy value to the attribute. Simply the boolean attribute expression assigns a truthy value or when the value is falsy, do not execute any assignment. For example, <div ?hidden=${false}></div>will be rendered as<div></div>. This might cause bugs that are hard to figure out when a custom boolean property is initialized with the value true.

Milestones

alicanerdogan commented 2 years ago

Here is the case where using a ? can cause a bug with a custom boolean property.

Let's start with a simple component with a boolean property value called flag. Its initialization value is true.

import {LitElement, css, html} from 'lit';
import {customElement, property} from 'lit/decorators.js';

@customElement('comp-1')
class Comp1 extends LitElement {
  @property() flag = true;

  render() {
    return html`
      <p>
        ${this.flag.toString()}
      </p>
    `;
  }
}

And the component is used here and set the flag to false by using ?.

import {LitElement, html} from 'lit';
import {customElement} from 'lit/decorators.js';

import './my-article.js';

@customElement('my-page')
class MyPage extends LitElement {
  render() {
    return html`
      <comp-1 ?flag=${false}></comp-1>
    `;
  }
}

The common sense says the component above should render <p>false</p>, because the value for flag is set to false. However, it actually renders <p>true</p> since the boolean attribute statement doesn't really pass the value to the component. Therefore, the initialization value is used during the render.

If the . statement is used as below, it renders <p>false</p> as expected.

return html`
  <comp-1 .flag=${false}></comp-1>
`;
43081j commented 2 years ago

sorry it took a while to get to this one.

just reading through it now, i'd be tempted to loosen it up / switch it around a little..

currently, it requires that only built in boolean attributes are bindable by boolean bindings.

my suggestion would be that it requires that built-in boolean attributes are always bound using a boolean binding (?) but allows all other attributes to be bound that way.

reason being, it is fairly common to implement custom boolean attributes which should be bound using boolean bindings. this in its current state would disallow that.

lets see what @stramel thinks

stramel commented 2 years ago

Thanks for the PR @alicanerdogan!

Taking a look at the PR, related issue and the discussion, I think I'm in agreement with @43081j. I like the idea of enforcing it for built-in booleans but allowing other attributes to be bound that way. I would imagine most people would disable to rule for being too strict otherwise.