[labs/task] Fix Task.render()'s return type always being undefined by mlcui-corp · Pull Request #4070 · lit/lit

mlcui-corp · 2023-08-04T01:05:51Z

In TypeScript:

unknown is the top type: everything can be assigned to unknown, and unknown can be only assigned to unknown.
never is the bottom type: never can be assigned to everything, and only never can be assigned to never.
Assuming a Cat can be assigned to an Animal, but not the other way around:
- (read-only) arrays are covariant: a Cat[] can be assigned to an Animal[], but not the other way around. In other words, arrays don't change the "order" of types, as expected.
- function arguments are contravariant: a (x: Animal) => boolean can be assigned to a (x: Cat) => boolean, but not the other way around. In other words, function arguments flip the "order" of types.

The right-hand of the extends in MaybeReturnType is a (...args: unknown[]) => ?. If (...args: T[]) => ? is assignable to that, then unknown is assignable to T… but unknown can only be assigned to unknown, so T must be unknown. No functions can be assigned to a type of (...args: unknown[]) => ? unless it is also that type.

Therefore, the extends clause in MaybeReturnType was always failing for all functions, apart from the ones which had an arguments array of unknown[]. This caused render's return type to only include undefined.

The function type in MaybeReturnType should be a function type which can have any function arguments, so we should use (...args: never[]) => ? instead - as never can be assigned to everything, so everything can be assigned to (...args: never[]) => ?.

In TypeScript: - `unknown` is the top type: everything can be assigned to `unknown`, and `unknown` can be only assigned to `unknown`. - `never` is the bottom type: `never` can be assigned to everything, and only `never` can be assigned to `never`. - Assuming a `Cat` can be assigned to an `Animal`, but not the other way around: - (read-only) arrays are covariant: a `Cat[]` can be assigned to an `Animal[]`, but not the other way around. In other words, arrays don't change the "order" of types, as expected. - function arguments are contravariant: a `(x: Animal) => boolean` can be assigned to a `(x: Cat) => boolean`, but not the other way around. In other words, function arguments flip the "order" of types. The right-hand of the `extends` in `MaybeReturnType` is a `(...args: unknown[]) => ?`. If `(...args: T[]) => ?` is assignable to that, then `unknown` is assignable to `T`… but `unknown` can only be assigned to `unknown`, so `T` must be `unknown`. No functions can be assigned to a type of `(...args: unknown[]) => ?` unless it is also that type. Therefore, the `extends` clause in `MaybeReturnType` was always failing for all functions, apart from the ones which had an arguments array of `unknown[]`. This caused `render`'s return type to only include `undefined`. The function type in `MaybeReturnType` should be a function type which can have _any_ function arguments, so we should use `(...args: never[]) => ?` instead - as `never` can be assigned to everything, so everything can be assigned to `(...args: never[]) => ?`.

changeset-bot · 2023-08-04T01:05:54Z

🦋 Changeset detected

Latest commit: 8fbe5f2

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

This PR includes changesets to release 1 package

Name	Type
@lit-labs/task	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-08-04T01:06:08Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -8% - +4% (-2.06ms - +1.00ms)
^{this-change vs tip-of-tree}

render

this-change: 80.75ms - 84.48ms
this-change, tip-of-tree, previous-release: unsure 🔍 -6% - +2% (-2.11ms - +0.55ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-0.91ms - +0.93ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-1.02ms - +1.14ms)
^{this-change vs tip-of-tree}

update

this-change: 876.86ms - 891.83ms
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +4% (-3.03ms - +3.72ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +1% (-2.34ms - +1.17ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +0% (-8.83ms - +2.75ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 841.06ms - 848.32ms
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-4.90ms - +6.17ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 115.0.5790.110
Sample size: 50
Built by: Benchmarks #5160
Commit: 8fbe5f2

render

Version	Avg time	vs
	80.75ms - 84.48ms	-

update

Version	Avg time	vs
	876.86ms - 891.83ms	-

update-reflect

Version	Avg time	vs
	841.06ms - 848.32ms	-

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

Browser: chrome-headless 115.0.5790.110
Sample size: 30
Built by: Benchmarks #5160
Commit: 8fbe5f2

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	31.44ms - 33.40ms	-	unsure 🔍 -6% - +2% -2.11ms - +0.55ms	unsure 🔍 -6% - +3% -1.92ms - +0.91ms
tip-of-tree tip-of-tree	32.29ms - 34.10ms	unsure 🔍 -2% - +7% -0.55ms - +2.11ms	-	unsure 🔍 -3% - +5% -1.09ms - +1.64ms
previous-release previous-release	31.90ms - 33.94ms	unsure 🔍 -3% - +6% -0.91ms - +1.92ms	unsure 🔍 -5% - +3% -1.64ms - +1.09ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	86.28ms - 91.20ms	-	unsure 🔍 -3% - +4% -3.03ms - +3.72ms	unsure 🔍 -8% - +0% -7.36ms - +0.22ms
tip-of-tree tip-of-tree	86.08ms - 90.71ms	unsure 🔍 -4% - +3% -3.72ms - +3.03ms	-	faster ✔ 0% - 8% 0.22ms - 7.61ms
previous-release previous-release	89.43ms - 95.20ms	unsure 🔍 -0% - +8% -0.22ms - +7.36ms	slower ❌ 0% - 9% 0.22ms - 7.61ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	23.03ms - 25.16ms	-	unsure 🔍 -8% - +4% -2.06ms - +1.00ms	unsure 🔍 -4% - +8% -1.05ms - +1.86ms
tip-of-tree tip-of-tree	23.53ms - 25.73ms	unsure 🔍 -4% - +9% -1.00ms - +2.06ms	-	unsure 🔍 -2% - +10% -0.54ms - +2.42ms
previous-release previous-release	22.70ms - 24.68ms	unsure 🔍 -8% - +4% -1.86ms - +1.05ms	unsure 🔍 -10% - +2% -2.42ms - +0.54ms	-

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

Browser: chrome-headless 115.0.5790.110
Sample size: 50
Built by: Benchmarks #5160
Commit: 8fbe5f2

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	58.86ms - 60.26ms	-	unsure 🔍 -2% - +2% -0.91ms - +0.93ms	unsure 🔍 -2% - +1% -1.35ms - +0.54ms
tip-of-tree tip-of-tree	58.95ms - 60.14ms	unsure 🔍 -2% - +2% -0.93ms - +0.91ms	-	unsure 🔍 -2% - +1% -1.29ms - +0.45ms
previous-release previous-release	59.33ms - 60.60ms	unsure 🔍 -1% - +2% -0.54ms - +1.35ms	unsure 🔍 -1% - +2% -0.45ms - +1.29ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	126.49ms - 129.02ms	-	unsure 🔍 -2% - +1% -2.34ms - +1.17ms	unsure 🔍 -2% - +1% -2.19ms - +1.24ms
tip-of-tree tip-of-tree	127.13ms - 129.56ms	unsure 🔍 -1% - +2% -1.17ms - +2.34ms	-	unsure 🔍 -1% - +1% -1.56ms - +1.79ms
previous-release previous-release	127.08ms - 129.38ms	unsure 🔍 -1% - +2% -1.24ms - +2.19ms	unsure 🔍 -1% - +1% -1.79ms - +1.56ms	-

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

Browser: chrome-headless 115.0.5790.110
Sample size: 50
Built by: Benchmarks #5160
Commit: 8fbe5f2

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	51.53ms - 53.10ms	-	unsure 🔍 -2% - +2% -1.02ms - +1.14ms	unsure 🔍 -2% - +2% -0.87ms - +1.20ms
tip-of-tree tip-of-tree	51.52ms - 53.00ms	unsure 🔍 -2% - +2% -1.14ms - +1.02ms	-	unsure 🔍 -2% - +2% -0.90ms - +1.11ms
previous-release previous-release	51.47ms - 52.83ms	unsure 🔍 -2% - +2% -1.20ms - +0.87ms	unsure 🔍 -2% - +2% -1.11ms - +0.90ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	893.93ms - 901.43ms	-	unsure 🔍 -1% - +0% -8.83ms - +2.75ms	unsure 🔍 -1% - +1% -5.49ms - +5.52ms
tip-of-tree tip-of-tree	896.31ms - 905.13ms	unsure 🔍 -0% - +1% -2.75ms - +8.83ms	-	unsure 🔍 -0% - +1% -2.92ms - +9.03ms
previous-release previous-release	893.63ms - 901.69ms	unsure 🔍 -1% - +1% -5.52ms - +5.49ms	unsure 🔍 -1% - +0% -9.03ms - +2.92ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	890.18ms - 898.37ms	-	unsure 🔍 -1% - +1% -4.90ms - +6.17ms	unsure 🔍 -1% - +1% -4.74ms - +7.17ms
tip-of-tree tip-of-tree	889.92ms - 897.36ms	unsure 🔍 -1% - +1% -6.17ms - +4.90ms	-	unsure 🔍 -1% - +1% -5.13ms - +6.28ms
previous-release previous-release	888.74ms - 897.38ms	unsure 🔍 -1% - +1% -7.17ms - +4.74ms	unsure 🔍 -1% - +1% -6.28ms - +5.13ms	-

_{tachometer-reporter-action v2 for Benchmarks}

mlcui-corp · 2023-08-04T03:20:16Z

@rictic Does this need a changeset?

rictic · 2023-08-04T03:26:10Z

Yes, run npm run changeset to create one. I think this is a bug fix, so this would count as a patch change (not a major or minor)

mlcui-corp · 2023-08-04T03:30:25Z

Done (as a new commit). I assume this PR will be squashed when merged - let me know if you'd like me to squash it on my branch.

rictic · 2023-08-04T06:32:19Z

Yep, will squash and merge!

justinfagnani

We need to update the test associated with this too: https://github.com/lit/lit/blob/main/packages/labs/task/src/test/task_test.ts#L1029

The current test is intended to show that render() does return a type based on the callback return types - which it does now because the callbacks have no arguments and so can be assigned to (...args: unknown[]) => ?.

I think the only change needed to show the typing failing is to add an argument to the complete callback in the last assertion:

    accept<number>(
      el.task.render({
        initial: () => 123,
-        complete: () => 123,
+        complete: (x: string) => Number(x),
        pending: () => 123,
        error: () => 123,
      })
    );

mlcui-corp · 2023-08-07T00:26:31Z

Done. A new type-only test was added to test for this, which fails if the change is reverted.

justinfagnani · 2023-08-10T20:44:39Z

Thanks a lot @mlcui-google!

mlcui-corp requested a review from kevinpschaaf as a code owner August 4, 2023 01:05

rictic approved these changes Aug 4, 2023

View reviewed changes

Add changeset for Task.render() return type fix

446d782

justinfagnani requested changes Aug 4, 2023

View reviewed changes

Add test for Task.render() return type with function arguments

8fbe5f2

justinfagnani approved these changes Aug 10, 2023

View reviewed changes

justinfagnani merged commit 3be62b0 into lit:main Aug 10, 2023

This was referenced Aug 10, 2023

Version Packages #4083

Closed

Version Packages #4094

Merged

Conversation

mlcui-corp commented Aug 4, 2023

Uh oh!

changeset-bot bot commented Aug 4, 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 Aug 4, 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!

mlcui-corp commented Aug 4, 2023

Uh oh!

rictic commented Aug 4, 2023

Uh oh!

mlcui-corp commented Aug 4, 2023

Uh oh!

rictic commented Aug 4, 2023

Uh oh!

justinfagnani left a comment

Choose a reason for hiding this comment

Uh oh!

mlcui-corp commented Aug 7, 2023

Uh oh!

justinfagnani commented Aug 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Aug 4, 2023 •

edited

Loading

github-actions bot commented Aug 4, 2023 •

edited

Loading