lit / lit

Lit is a simple library for building fast, lightweight web components.
https://lit.dev
BSD 3-Clause "New" or "Revised" License
18.76k stars 925 forks source link

[lit-html] Choose directive doesn't check types correctly #4220

Closed remziatay closed 1 year ago

remziatay commented 1 year ago

Which package(s) are affected?

Lit Core (lit / lit-html / lit-element / reactive-element)

Description

When I use choose directive like this, I would expect typescript to throw error since 'test' and 'random' can't be assigned to CheckoutStep type

type CheckoutStep = 'register' | 'delivery' | 'payment';

function render(step: CheckoutStep) {
    return choose(step, [
        ['test', () => 1],
        ['random', () => 2],
        ['register', () => 3],
    ]);
}

Reproduction

There should be a type error but there isn't: Typescript gist

Workaround

It's seems like it's because T is inferred as string since it's used for both value and cases params

export declare const choose: <T, V>(value: T, cases: [T, () => V][]) => V;

It works as expected when I update the signature like this

export declare const choose: <T, V, K extends T>(value: T, cases: [K, () => V][]) => V;

Is this a regression?

No or unsure. This never worked, or I haven't tried before.

Affected versions

This never worked

Browser/OS/Node environment

OS: Windows 10 Node version: 18.12.1 npm version: 8.19.2

augustjk commented 1 year ago

Interesting! Yeah, looks like the type inference for T becomes broader so you'd need to manually provide the types like choose<CheckoutStep, number> to get it to type check in its current state.

The suggestion of adding the third type parameter extending T does indeed fix this. It would be good to make it default to T to make it non-breaking for anyone who actually did specify only 2 type arguments before like

export declare const choose: <T, V, K extends T = T>(value: T, cases: [K, () => V][]) => V;