runem / lit-analyzer

Monorepository for tools that analyze lit-html templates
MIT License
321 stars 38 forks source link

[lit-plugin] no-incompatible-type-binding when types should be compatible #97

Open jrobinson01 opened 4 years ago

jrobinson01 commented 4 years ago

I'm using the lit-plugin in VSCode. I'm NOT using typescript, but instead, javascript with jsdoc type annotations.

It seems like when setting properties via the template binding, lit-plugin isn't using the jsdoc annotation to determine the type, but rather the inferred type based on the value (or is it looking at the type in the static properties getter?):

Screen Shot 2020-04-29 at 12 00 43 PM

The type for this.context.states is picked up correct as string[] and the component I'm assigning the property on has this constructor:

constructor() {
    super();
    /** @type {Array<string>} */
    this.states = [];
    /** @type {string?} */
    this.selected = null;
  }
jrobinson01 commented 4 years ago

I'm going to try to look into the code today, but my assumption is that the analyzer is looking at the type as defined in the static properties getter instead of the actual property.

runem commented 4 years ago

I'm sorry for the long waiting time on this issue!

What I have on the 1.2.0 branch uses a much newer version of WCA which has better support for JSDoc. I would start by testing on that branch. You can test the VSCode plugin by installing the .vsix using the artifact here: https://github.com/runem/lit-analyzer/actions/runs/108128514. That should be the newest build of lit-plugin :blush:

jrobinson01 commented 4 years ago

Thanks! It seems like that version gets rid of my errors and such, but it's still not correctly picking up the type from the jsdoc, but using the type from properties instead. Here's an example:

import {LitElement, html} from 'lit-element';

class ChildElement extends LitElement {
  static get properties() {
    return {
      // type picked up from here
      things: {
        type: Array,
      }
    }
  }
  constructor() {
    super();
    // this has no effect
    /** @type {Array<boolean>} */
    this.things = [];
  }
}

customElements.define('child-element', ChildElement);

class MyElement extends LitElement {
  static get properties() {
    return {
      items: {
        type: Array,
      }
    };
  }

  constructor() {
    super();
    /** @type {Array<string>} */
    this.items = [];
  }

  render() {
    return html`
      <child-element .things=${this.items}></child-element>
    `;
  }
}

customElements.define('my-element', MyElement);

In this case, child-element should be expecting a type of Boolean[] for it's things property, but it's expecting any[]. If you make this change:

import {LitElement, html} from 'lit-element';

class ChildElement extends LitElement {
  static get properties() {
    return {
      things: {
        type: String,
      }
    }
  }
  constructor() {
    super();
    /** @type {Array<boolean>} */
    this.things = [];
  }
}

customElements.define('child-element', ChildElement);

class MyElement extends LitElement {
  static get properties() {
    return {
      items: {
        type: Array,
      }
    };
  }

  constructor() {
    super();
    /** @type {Array<string>} */
    this.items = [];
  }

  render() {
    return html`
      <child-element .things=${this.items}></child-element>
    `;
  }
}

customElements.define('my-element', MyElement);

changing the things.type property to String then the plugin is able to detect a problem here: <child-element .things=${this.items}></child-element> and complains that child-element.things should be a String instead of an String[].

Meanwhile, the jsdoc typecast in the constructor of child-element seeming has no effect.

jrunning commented 3 years ago

Is there any update on this? I'm still seeing this issue in VSCode using lit-plugin 1.2.1.

hsablonniere commented 2 years ago

FYI: I have the same limitation, I just went deep into lit-analyzer's code with some console.log here and there. The types are detected by Web Component Analyzer, so I'm going to dig there... I may be back with more details :wink:

cpiggott commented 1 year ago

FYI: I have the same limitation, I just went deep into lit-analyzer's code with some console.log here and there. The types are detected by Web Component Analyzer, so I'm going to dig there... I may be back with more details 😉

@hsablonniere Did you come back with any details from your deep dive into the Web Component Analyzer?

louwers commented 12 months ago

Web Component Analyzer simply doesn't support TypeScript types?

Edit: It does.

Here is my workaround:

export class BLExerciseEntryPercentage extends LitElement {
  static properties = {
    template: { type: Object },
  };

  /**
   * @type {Models.Exercise['templates'][0] & {type: 'percentageOfOneRepMax'}}
   * @prop
   * */
  template;

  constructor() {
    super();
    ...
  }

You need to modify jsconfig.json and set

{
  "compilerOptions": {
    "strictPropertyInitialization": false,

Unfortunately one thing I was not able to fix is completion inside a template property with a type:

html`
  <bl-exercise-percentage>
    .template="${<cursor_here>}"

Edit2: The above change is somehow actually breaking my component. Looking for another workaround...

desean1625 commented 6 months ago

This is just a great plugin. But this still seems to be an issue (1.4.3). Has anyone found a workaround?

desean1625 commented 6 months ago

@runem Took a look at this and the webcomponent analyzer returns a member declaration with a type function that always results in {kind:"ANY"} unless it is a basic type like number|string|bool The member declaration does return the correct type in the typeHint as a string.

In the example test

/**
* @typedef {Object} Chip
* @property {string} test
* /
/**
* @element
*/
class MyElement extends HTMLElement { 
      static get properties () {
          return {
              /**
               * This is a comment
               * @type {Chip[]}
               */
              myProp: {attribute:false}
          }
      }
    constructor(){
    /**
    * @type {Chip[]}
    * /
    this.myProp = []
    }
}

I was thinking that we could loop through all the property assignments in the constructor block and get the correct type and return a ts.Type instead of a SimpleType, but I don't know enough about typescript compiler to figure out how to get the type from a statement at a position.

I think maybe it was on the right track?

function parseStaticProperties(returnStatement: ReturnStatement, context: AnalyzerDeclarationVisitContext,node:Node): ComponentMember[] {
    const { ts } = context;
      var component;
      if(ts.isClassDeclaration(node.parent)){
          component = node.parent;
      }
    const memberResults: ComponentMember[] = [];
        if (returnStatement.expression != null && ts.isObjectLiteralExpression(returnStatement.expression)) {
        // Each property in the object literal expression corresponds to a class field.
        for (const propNode of returnStatement.expression.properties) {
            // Get propName
            const propName = propNode.name != null && ts.isIdentifier(propNode.name) ? propNode.name.text : undefined;
            if (propName == null) {
                continue;
            }
                    if(component){
                        const constructor = (component.members.filter((n) => ts.isConstructorDeclaration(n))[0]||undefined) as ConstructorDeclaration|undefined
                        if(constructor && constructor.body?.statements){
                            console.log(constructor.body?.statements)
                            //Todo Extract the type for  statements that are property assignment and match the propName 
                        }
                    }
desean1625 commented 6 months ago

Using the declaration.typeHint we can at least fix the quickInfo popup.

image

But without breaking down the jsdoc type into a SimpleType the completions don't work correctly.