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

Small cleanup of lit-plugin errors #303

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

Misiu
Copy link
Contributor

@Misiu Misiu commented Sep 21, 2022

I've added recommended extensions (same as in HA Frontend).
I've also removed some (not all) lit-plugin errors.
I'll try to clean the rest in the next PR (plus replace confirm update all with a proper dialog)

@Misiu
Copy link
Contributor Author

Misiu commented Sep 21, 2022

any ideas why npm exec -- prettier --check src in github actions only shows 3 files, but locally (windows 10) I get 81 files?
I'm also formatting files via Prettier locally before every commit, but I get errors every time CI runs on github.
Probably because of different settings.

@@ -24,7 +24,7 @@ export class ESPHomeProcessDialog extends LitElement {
<mwc-dialog
open
heading=${this.heading}
scrimClickAction
scrimClickAction=""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this. This is not an improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this doesn't look pretty, but without this I get this error in VS Code:
image

same with lit-analyzer:

Type 'true' is not assignable to 'string'
27:  scrimClickAction
no-incompatible-type-binding

both properties (scrimClickAction, escapeKeyAction) are of type string:
image

not sure how to solve that better way instead of extending mwc-dialog and adding custom properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attributes are always strings, a string without a value is interpreted as an empty string in HTML. This sounds like a bug in lit-analyzer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at https://github.com/esphome/dashboard/blob/main/src/components/process-dialog.ts#L25-L27.
In VS Code I see an error for scrimClickAction:
image

Looking at the definition of mwc-dialog (mwc-dialog-base.d.ts) I see this:

/**
 * @license
 * Copyright 2019 Google LLC
 * SPDX-License-Identifier: Apache-2.0
 */
import 'blocking-elements';
import 'wicg-inert';
import { MDCDialogAdapter } from '@material/dialog/adapter.js';
import MDCDialogFoundation from '@material/dialog/foundation.js';
import { BaseElement } from '@material/mwc-base/base-element.js';
export { MDCDialogCloseEventDetail } from '@material/dialog/types.js';
export declare class DialogBase extends BaseElement {
    protected mdcRoot: HTMLDivElement;
    protected primarySlot: HTMLElement;
    protected secondarySlot: HTMLElement;
    protected contentSlot: HTMLElement;
    protected contentElement: HTMLDivElement;
    protected conatinerElement: HTMLDivElement;
    hideActions: boolean;
    stacked: boolean;
    heading: string;
    scrimClickAction: string;
    escapeKeyAction: string;
    open: boolean;
    defaultAction: string;
    actionAttribute: string;
    initialFocusAttribute: string;
    set suppressDefaultPressSelector(selector: string);
    /**
     * @export
     */
    get suppressDefaultPressSelector(): string;
    protected closingDueToDisconnect?: boolean;
    protected initialSupressDefaultPressSelector: string;
    protected get primaryButton(): HTMLElement | null;
    protected currentAction: string | undefined;
    protected mdcFoundationClass: typeof MDCDialogFoundation;
    protected mdcFoundation: MDCDialogFoundation;
    protected boundHandleClick: ((ev: MouseEvent) => void) | null;
    protected boundHandleKeydown: ((ev: KeyboardEvent) => void) | null;
    protected boundHandleDocumentKeydown: ((ev: KeyboardEvent) => void) | null;
    protected emitNotification(name: string, action?: string): void;
    protected getInitialFocusEl(): HTMLElement | null;
    protected searchNodeTreesForAttribute(nodes: Node[], attribute: string): HTMLElement | null;
    protected createAdapter(): MDCDialogAdapter;
    protected render(): import("lit-html").TemplateResult<1>;
    protected renderHeading(): import("lit-html").TemplateResult<1>;
    protected firstUpdated(): void;
    connectedCallback(): void;
    disconnectedCallback(): void;
    forceLayout(): void;
    focus(): void;
    blur(): void;
    protected setEventListeners(): void;
    protected removeEventListeners(): void;
    close(): void;
    show(): void;
}

open is a type boolean (no error), but scrimClickAction is a type string (we get an error), I think that's the reason for the error.
Saying that I'm not sure if this is an error in the lit-plugin (vs code extensions) or if should this be fixed in the dashboard code base.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/lit/lit/blob/main/packages/reactive-element/src/reactive-element.ts#L333

Lit's fromAttribute convertor has no case for string, so it treats strings just as they get it from the HTML DOM object. An empty attribute is returned as an empty string. That is implicit. It's a bug in lit analyzer that it does not accept an empty attribute for a string value. It's not that it didn't extract the type correctly.

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 this pull request may close these issues.

None yet

2 participants