webrecorder / browsertrix

Browsertrix is the hosted, high-fidelity, browser-based crawling service from Webrecorder designed to make web archiving easier and more accessible for all!
https://webrecorder.net/browsertrix
GNU Affero General Public License v3.0
201 stars 35 forks source link

Investigate lit html linting #1378

Open SuaYoo opened 1 year ago

SuaYoo commented 1 year ago

lit-plugin/lit-analyzer doesn't show errors for missing attributes or non-existent attributes. We should investigate whether this can be enabled with lit-analyzer or another tool.

emma-sg commented 1 year ago

Proof of concept, with lit-analyzer@2.0.1:

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

@customElement("prop-demo")
export class PropDemo extends LitElement {
  @property({ type: String })
  name!: string; // this should error if it's missing

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

@customElement("example-element")
export class ExampleElement extends LitElement {
  render() {
    return html`
      <prop-demo></prop-demo> this should error, it's missing 'name', but it
      doesn't

      <prop-demo name="this is correct, and doesn't error"></prop-demo>
      <prop-demo .name=${"this is correct, and doesn't error"}></prop-demo>
      <prop-demo .name=${true}></prop-demo> this is incorrect, and does error

      <prop-demo
        incorrect-prop="this is incorrect, and only sometimes errors"
      ></prop-demo>
      <prop-demo .incorrect-prop=${"...but now it does"}></prop-demo>
    `;
  }
}

Image

SuaYoo commented 1 year ago

Unknown attributes addressed in https://github.com/webrecorder/browsertrix-cloud/pull/1381

emma-sg commented 1 year ago

disagree — there's still nothing that highlights missing but required attributes/props. mind if we keep this open?

emma-sg commented 1 year ago

Not too sure what potential solutions exist here, aside from looking at frameworks that allow for JSX templating (Stencil?). Unless we want to try to contribute back to lit-analyzer?? Let's keep this open for now so we can keep track of issues that arise as a result of this not being possible.

SuaYoo commented 1 year ago

Perhaps there's a solution that also addresses https://github.com/webrecorder/browsertrix-cloud/issues/55 ?

emma-sg commented 11 months ago

Kicking this back to todo for now — I'm not sure there's a good solution right now as long as we continue to use lit. Svelte does support custom elements (and could be adopted incrementally) though, that could potentially be something we look at in the future — maybe for our component library, even if the main app continues to be lit? cc @Shrinks99 @SuaYoo

SuaYoo commented 11 months ago

disagree — there's still nothing that highlights missing but required attributes/props. mind if we keep this open?

Might be worth starting a discussion in the Lit Discord channel to see how others are approaching this? (edit: started one here)

Svelte does support custom elements (and could be adopted incrementally) though, that could potentially be something we look at in the future — maybe for our component library, even if the main app continues to be lit?

I don't think it'd be worth the time spent to incorporate another component library unless we need to address a large feature gap between the libraries.

SuaYoo commented 11 months ago

Not ideal, but maybe the initial approach should be:

SuaYoo commented 10 months ago

@emma-sg what else needed to be done here to close this issue?

emma-sg commented 10 months ago

@SuaYoo I'm not too sure, I'm keeping an eye on @lit-labs/analyzer and it's possible there are other solutions too. I asked about this in the Lit Discord server too, and the answer I got was mostly "possible with JSX but Lit doesn't really use JSX":

augustine — 01/08/2024 7:54 PM i don't think that lit-plugin will treat any properties as required and throw a type error on templates with missing properties https://github.com/runem/lit-analyzer/issues/74. nor does custom element manifest include a way of marking a field to my knowledge.

i think the philosophy behind that was that native elements don't have required properties/attributes so custom elements generally shouldn't either. though i empathize with the use case for building app components where you do want required props. definitely possible with JSX but i don't know if a way to do that within html tagged template.

So this issue is not resolved, but also doesn't have a straightforward resolution right now.