Fix choose directive type inference by remziatay · Pull Request #4240 · lit/lit

remziatay · 2023-09-28T18:59:10Z

changeset-bot · 2023-09-28T18:59:13Z

🦋 Changeset detected

Latest commit: 5f0a5ba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
lit-html	Patch
lit	Patch
@lit-examples/nextjs	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-09-28T18:59:24Z

📊 Tachometer Benchmark Results

Summary

⏳ Benchmarks are currently running. Results below are out of date.

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -13% - +1% (-2.43ms - +0.21ms)
^{this-change vs tip-of-tree}

render

this-change: 69.24ms - 71.18ms
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +7% (-0.74ms - +1.92ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +2% (-1.45ms - +1.20ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +0% (-2.16ms - +0.04ms)
^{this-change vs tip-of-tree}

update

this-change: 720.60ms - 729.91ms
this-change, tip-of-tree, previous-release: unsure 🔍 -9% - +0% (-5.74ms - +0.30ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +2% (-1.51ms - +1.92ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-3.93ms - +8.93ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 711.20ms - 720.22ms
this-change, tip-of-tree, previous-release: unsure 🔍 -0% - +1% (-3.65ms - +7.80ms)
^{this-change vs tip-of-tree}

Results

⏳ Benchmarks are currently running. Results below are out of date.

this-change

Browser: chrome-headless 117.0.5938.92
Sample size: 50
Built by: Benchmarks #5562
Commit: 91c85cb

render

Version	Avg time	vs
	69.24ms - 71.18ms	-

update

Version	Avg time	vs
	720.60ms - 729.91ms	-

update-reflect

Version	Avg time	vs
	711.20ms - 720.22ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 117.0.5938.92
Sample size: 30
Built by: Benchmarks #5562
Commit: 91c85cb

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	26.11ms - 27.93ms	-	unsure 🔍 -3% - +7% -0.74ms - +1.92ms	unsure 🔍 -4% - +5% -1.12ms - +1.39ms
tip-of-tree tip-of-tree	25.45ms - 27.41ms	unsure 🔍 -7% - +3% -1.92ms - +0.74ms	-	unsure 🔍 -7% - +3% -1.76ms - +0.85ms
previous-release previous-release	26.02ms - 27.75ms	unsure 🔍 -5% - +4% -1.39ms - +1.12ms	unsure 🔍 -3% - +7% -0.85ms - +1.76ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	56.21ms - 59.36ms	-	unsure 🔍 -9% - +0% -5.74ms - +0.30ms	unsure 🔍 -4% - +4% -2.09ms - +2.21ms
tip-of-tree tip-of-tree	57.93ms - 63.09ms	unsure 🔍 -1% - +10% -0.30ms - +5.74ms	-	unsure 🔍 -0% - +10% -0.19ms - +5.75ms
previous-release previous-release	56.26ms - 59.20ms	unsure 🔍 -4% - +4% -2.21ms - +2.09ms	unsure 🔍 -9% - +0% -5.75ms - +0.19ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	16.96ms - 17.96ms	-	unsure 🔍 -13% - +1% -2.43ms - +0.21ms	unsure 🔍 -11% - -0% -2.06ms - +0.04ms
tip-of-tree tip-of-tree	17.34ms - 19.79ms	unsure 🔍 -1% - +14% -0.21ms - +2.43ms	-	unsure 🔍 -8% - +9% -1.43ms - +1.63ms
previous-release previous-release	17.55ms - 19.39ms	unsure 🔍 -0% - +12% -0.04ms - +2.06ms	unsure 🔍 -9% - +8% -1.63ms - +1.43ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 117.0.5938.92
Sample size: 50
Built by: Benchmarks #5562
Commit: 91c85cb

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	49.09ms - 50.96ms	-	unsure 🔍 -3% - +2% -1.45ms - +1.20ms	unsure 🔍 -2% - +2% -1.12ms - +1.14ms
tip-of-tree tip-of-tree	49.22ms - 51.09ms	unsure 🔍 -2% - +3% -1.20ms - +1.45ms	-	unsure 🔍 -2% - +3% -1.00ms - +1.26ms
previous-release previous-release	49.39ms - 50.65ms	unsure 🔍 -2% - +2% -1.14ms - +1.12ms	unsure 🔍 -3% - +2% -1.26ms - +1.00ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	100.74ms - 103.23ms	-	unsure 🔍 -1% - +2% -1.51ms - +1.92ms	unsure 🔍 -2% - +2% -1.61ms - +2.16ms
tip-of-tree tip-of-tree	100.61ms - 102.95ms	unsure 🔍 -2% - +1% -1.92ms - +1.51ms	-	unsure 🔍 -2% - +2% -1.77ms - +1.91ms
previous-release previous-release	100.29ms - 103.13ms	unsure 🔍 -2% - +2% -2.16ms - +1.61ms	unsure 🔍 -2% - +2% -1.91ms - +1.77ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 117.0.5938.92
Sample size: 50
Built by: Benchmarks #5562
Commit: 91c85cb

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	43.55ms - 45.02ms	-	unsure 🔍 -5% - +0% -2.16ms - +0.04ms	unsure 🔍 -5% - +1% -2.07ms - +0.26ms
tip-of-tree tip-of-tree	44.52ms - 46.17ms	unsure 🔍 -0% - +5% -0.04ms - +2.16ms	-	unsure 🔍 -2% - +3% -1.07ms - +1.38ms
previous-release previous-release	44.28ms - 46.09ms	unsure 🔍 -1% - +5% -0.26ms - +2.07ms	unsure 🔍 -3% - +2% -1.38ms - +1.07ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	743.36ms - 753.19ms	-	unsure 🔍 -1% - +1% -3.93ms - +8.93ms	unsure 🔍 -1% - +1% -4.62ms - +8.49ms
tip-of-tree tip-of-tree	741.63ms - 749.92ms	unsure 🔍 -1% - +1% -8.93ms - +3.93ms	-	unsure 🔍 -1% - +1% -6.56ms - +5.44ms
previous-release previous-release	742.00ms - 750.68ms	unsure 🔍 -1% - +1% -8.49ms - +4.62ms	unsure 🔍 -1% - +1% -5.44ms - +6.56ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	738.16ms - 747.25ms	-	unsure 🔍 -0% - +1% -3.65ms - +7.80ms	unsure 🔍 -0% - +1% -2.79ms - +10.27ms
tip-of-tree tip-of-tree	737.15ms - 744.11ms	unsure 🔍 -1% - +0% -7.80ms - +3.65ms	-	unsure 🔍 -1% - +1% -4.18ms - +7.51ms
previous-release previous-release	734.28ms - 743.66ms	unsure 🔍 -1% - +0% -10.27ms - +2.79ms	unsure 🔍 -1% - +1% -7.51ms - +4.18ms	-

_{tachometer-reporter-action v2 for Benchmarks}

rictic

Mind adding a regression test to the choose_test.ts? Something like:

  if (false as boolean) {
    // Type-only regression test of https://github.com/lit/lit/issues/4220
    type CheckoutStep = 'register' | 'delivery' | 'payment';
    let step = 'register' as CheckoutStep;
    return choose(step, [
      // @ts-expect-error 'test' is not assignable to 'CheckoutStep'
      ['test', () => 1],
      // @ts-expect-error 'random' is not assignable to 'CheckoutStep'
      ['random', () => 2],
      // this should compile fine
      ['register', () => 3],
    ]);
  }

rictic · 2023-09-28T19:16:43Z

packages/lit-html/src/directives/choose.ts

 * ```
 */
-export const choose = <T, V>(
+export const choose = <T, V, K extends T = T>(


Nice! Yeah this fixes the issue. Playground demo

rictic · 2023-09-28T19:41:19Z

Also please add a changeset by running npm run changeset at the root of the monorepo. This seems like a patch level change to lit-html, with a description like: Improved the type of the choose directive.

remziatay · 2023-09-29T17:09:35Z

Mind adding a regression test to the choose_test.ts? Something like:

Of course! Done

augustjk

Thank you for the PR and sorry it took so long to get it in! I fixed the build error for the test and made the changelog a bit more detailed.

Requested change has been addressed

packages/lit-html/src/test/directives/choose_test.ts

.changeset/good-grapes-double.md

AndrewJakubowicz

Great! Thank you!

Add third type parameter to choose.ts

91c85cb

remziatay requested a review from kevinpschaaf as a code owner September 28, 2023 18:59

rictic previously requested changes Sep 28, 2023

View reviewed changes

remziatay added 2 commits September 29, 2023 18:58

Add regression test

9385cf1

Add changeset

840258f

AndrewJakubowicz requested a review from rictic October 20, 2023 17:58

augustjk added 3 commits October 27, 2023 11:05

Wrap type-only test with test.skip

34cb244

Add more detail to changelog

a75f49f

Merge branch 'main' of https://github.com/lit/lit into patch-4

bf7bf4d

augustjk approved these changes Oct 27, 2023

View reviewed changes

augustjk requested a review from AndrewJakubowicz October 27, 2023 18:09

AndrewJakubowicz reviewed Oct 27, 2023

View reviewed changes

packages/lit-html/src/test/directives/choose_test.ts Outdated Show resolved Hide resolved

Fix typo in test description

3afa4bf

augustjk reviewed Oct 27, 2023

View reviewed changes

.changeset/good-grapes-double.md Outdated Show resolved Hide resolved

Update .changeset/good-grapes-double.md

83f560a

AndrewJakubowicz approved these changes Oct 27, 2023

View reviewed changes

augustjk added 2 commits October 27, 2023 11:30

Merge branch 'main' into patch-4

78a39af

Add 'lit' package to changeset

5f0a5ba

augustjk enabled auto-merge (squash) October 27, 2023 21:37

augustjk merged commit edf998c into lit:main Oct 27, 2023

This was referenced Oct 27, 2023

Version Packages #4338

Closed

Version Packages #4341

Merged

remziatay deleted the patch-4 branch November 20, 2023 10:12

CxsLucyfer mentioned this pull request Jan 29, 2024

[Snyk] Upgrade @lit/localize-tools from 0.6.3 to 0.7.1 CxsLucyfer/lit#3

Open

Conversation

remziatay commented Sep 28, 2023

Uh oh!

changeset-bot bot commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

update-reflect

Uh oh!

rictic left a comment

Choose a reason for hiding this comment

Uh oh!

rictic Sep 28, 2023

Choose a reason for hiding this comment

Uh oh!

rictic commented Sep 28, 2023

Uh oh!

remziatay commented Sep 29, 2023

Uh oh!

augustjk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AndrewJakubowicz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

changeset-bot bot commented Sep 28, 2023 •

edited

Loading

github-actions bot commented Sep 28, 2023 •

edited

Loading