runem / lit-analyzer

Monorepository for tools that analyze lit-html templates
MIT License
318 stars 36 forks source link

Confusing ifDefined extension error when author wants default property fallback behavior #223

Closed AndrewJakubowicz closed 2 years ago

AndrewJakubowicz commented 2 years ago

Context

The extension doesn't allow correct usage of ifDefined (I think). See minimal example and playground link.

Explanation of the repro:

We have a custom element simple-greeting with optional name property with fallback "DefaultValue". The example tests 4 cases:

  1. Passing undefined directly to .name. This is valid but prevents the default value from rendering. I think this is expected.
  2. Passing nothing to .name. Plugin raises error: Type 'Symbol(undefined)' is not assignable to 'string | undefined'. But we get correct behavior of the default value.
  3. Passing a string directly. This works.
  4. Using ifDefined raises plugin error: The 'ifDefined' directive has no effect here.lit-plugin(no-invalid-directive-binding)(2308). However correct rendering behavior is observed.

Here is a code sample that can be copy pasted into VS code to reproduce the plugin error:

 import {html, css, LitElement, nothing} from 'lit';
 import {customElement, property} from 'lit/decorators.js';
 import {ifDefined} from 'lit/directives/if-defined.js';

 @customElement('simple-greeting')
 export class SimpleGreeting extends LitElement {

   @property()
   name?: string = 'DefaultValue';

   render() {
     return html`<p>Hello, ${this.name}!</p>`;
   }
 }

 @customElement('if-defined-test')
 class ParentTest extends LitElement {
   render() {
     const definedName = "Foo";
     const undefinedName = undefined;
     return html`
         <p>'undefined' test:</p>
         <simple-greeting .name=${undefinedName}></simple-greeting>

         <!-- Type 'Symbol(undefined)' is not assignable to 'string | undefined'lit-plugin(no-incompatible-type-binding)(2304) -->
         <p>'nothing' test:</p>
         <simple-greeting .name=${nothing}></simple-greeting>

         <p>Defined test:</p>
         <simple-greeting .name=${definedName}></simple-greeting>

         <!-- The 'ifDefined' directive has no effect here. lit-plugin(no-invalid-directive-binding)(2308) -->
         <p>ifDefined(undefined) test:</p>
         <simple-greeting .name=${ifDefined(undefinedName)}></simple-greeting>
     `;
   }
 }

Impact

This was raised in a chat as a point of confusion. Maybe there's a workaround that could be added to the error message.

43081j commented 2 years ago

because ifDefined is intended to be used with attribute bindings IIRC.

meaning:

<my-element

  .name=${ifDefined(val)} <- wrong

  name=${ifDefined(val)} <- right
>
arthurevans commented 2 years ago

In Lit 2, ifDefined is a simple function that returns the value or nothing. @justinfagnani merged some code to support Lit 2 directives, but I don't think the plugin has been updated yet--see https://github.com/runem/lit-analyzer/issues/219#issuecomment-1018714267

AndrewJakubowicz commented 2 years ago

Capturing some offline conversation here. The behavior here is more nuanced because the case I have described is only on initial render. Once a reactive update has overwritten the default, we don't get the default back on future updates that pass in nothing. So except for a property binding on initial render, this would indeed be a no-op.

Example: https://lit.dev/playground/?mods=gists#gist=11f037fa531cb794b37b6c2a7fba917c

Closing this issue since I think the analyzer is correct.