[@labs/task] add render return value by taylor-vann · Pull Request #2821 · lit/lit

taylor-vann · 2022-05-02T20:10:24Z

Return value is requested to be typed

The types could be inferred from a StatusRender

  render<SR extends StatusRenderer<R>>(renderer: SR) {
    switch (this.status) {
      case TaskStatus.INITIAL:
        return renderer.initial?.();
      case TaskStatus.PENDING:
        return renderer.pending?.();
      case TaskStatus.COMPLETE:
        return renderer.complete?.(this.value!);
      case TaskStatus.ERROR:
        return renderer.error?.(this.error);
      default:
        // exhaustiveness check
        this.status as void;
    }
  }

Related to: #2301

changeset-bot · 2022-05-02T20:10:26Z

🦋 Changeset detected

Latest commit: c34e532

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

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 · 2022-05-02T20:10:50Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -2% - +2% (-0.63ms - +0.67ms)
^{this-change vs tip-of-tree}

render

lit-element-list: unsure 🔍 -0% - +3% (-0.17ms - +2.40ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -1% - +2% (-0.18ms - +0.61ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -1% - +11% (-0.15ms - +1.32ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -8% - +3% (-6.06ms - +1.96ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -7% - +21% (-5.42ms - +17.10ms)
^{this-change vs tip-of-tree}

update

lit-element-list: unsure 🔍 -6% - +1% (-52.69ms - +12.25ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -4% - +4% (-3.95ms - +3.36ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -4% - +6% (-17.72ms - +28.23ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -8% - +4% (-13.84ms - +6.73ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -15% - +12% (-175.45ms - +141.67ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: unsure 🔍 -2% - +1% (-16.47ms - +4.53ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -6% - +2% (-58.74ms - +21.86ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 102.0.5005.61
Sample size: 50
Built by: Benchmarks #3332
Commit: f6b5c85

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	85.00ms - 86.66ms	-	unsure 🔍 -0% - +3% -0.17ms - +2.40ms	faster ✔ 18% - 20% 18.72ms - 21.81ms
tip-of-tree tip-of-tree	83.74ms - 85.69ms	unsure 🔍 -3% - +0% -2.40ms - +0.17ms	-	faster ✔ 19% - 21% 19.75ms - 23.00ms
previous-release previous-release	104.78ms - 107.39ms	slower ❌ 22% - 26% 18.72ms - 21.81ms	slower ❌ 23% - 27% 19.75ms - 23.00ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	856.56ms - 872.43ms	-	unsure 🔍 -6% - +1% -52.69ms - +12.25ms	faster ✔ 36% - 38% 488.75ms - 520.45ms
tip-of-tree tip-of-tree	853.23ms - 916.20ms	unsure 🔍 -1% - +6% -12.25ms - +52.69ms	-	faster ✔ 33% - 38% 450.03ms - 518.72ms
previous-release previous-release	1355.37ms - 1382.82ms	slower ❌ 56% - 61% 488.75ms - 520.45ms	slower ❌ 49% - 60% 450.03ms - 518.72ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	854.70ms - 869.47ms	-	unsure 🔍 -2% - +1% -16.47ms - +4.53ms	faster ✔ 7% - 10% 67.47ms - 91.38ms
tip-of-tree tip-of-tree	860.59ms - 875.52ms	unsure 🔍 -1% - +2% -4.53ms - +16.47ms	-	faster ✔ 7% - 9% 61.45ms - 85.47ms
previous-release previous-release	932.10ms - 950.92ms	slower ❌ 8% - 11% 67.47ms - 91.38ms	slower ❌ 7% - 10% 61.45ms - 85.47ms	-

lit-html-kitchen-sink

Browser: chrome-headless 102.0.5005.61
Sample size: 30
Built by: Benchmarks #3332
Commit: f6b5c85

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.94ms - 34.43ms	-	unsure 🔍 -1% - +2% -0.18ms - +0.61ms	faster ✔ 13% - 16% 4.95ms - 6.59ms
tip-of-tree tip-of-tree	33.66ms - 34.28ms	unsure 🔍 -2% - +1% -0.61ms - +0.18ms	-	faster ✔ 13% - 17% 5.15ms - 6.83ms
previous-release previous-release	39.17ms - 40.74ms	slower ❌ 14% - 19% 4.95ms - 6.59ms	slower ❌ 15% - 20% 5.15ms - 6.83ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	89.87ms - 93.66ms	-	unsure 🔍 -4% - +4% -3.95ms - +3.36ms	unsure 🔍 -5% - +2% -4.94ms - +1.45ms
tip-of-tree tip-of-tree	88.93ms - 95.19ms	unsure 🔍 -4% - +4% -3.36ms - +3.95ms	-	unsure 🔍 -6% - +3% -5.50ms - +2.60ms
previous-release previous-release	90.94ms - 96.09ms	unsure 🔍 -2% - +5% -1.45ms - +4.94ms	unsure 🔍 -3% - +6% -2.60ms - +5.50ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	29.48ms - 30.20ms	-	unsure 🔍 -2% - +2% -0.63ms - +0.67ms	faster ✔ 9% - 14% 3.06ms - 4.67ms
tip-of-tree tip-of-tree	29.28ms - 30.36ms	unsure 🔍 -2% - +2% -0.67ms - +0.63ms	-	faster ✔ 9% - 14% 2.98ms - 4.79ms
previous-release previous-release	32.98ms - 34.43ms	slower ❌ 10% - 16% 3.06ms - 4.67ms	slower ❌ 10% - 16% 2.98ms - 4.79ms	-

lit-html-repeat

Browser: chrome-headless 102.0.5005.61
Sample size: 30
Built by: Benchmarks #3332
Commit: f6b5c85

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	12.46ms - 13.82ms	-	unsure 🔍 -1% - +11% -0.15ms - +1.32ms	faster ✔ 3% - 14% 0.39ms - 1.99ms
tip-of-tree tip-of-tree	12.27ms - 12.83ms	unsure 🔍 -10% - +1% -1.32ms - +0.15ms	-	faster ✔ 9% - 16% 1.28ms - 2.28ms
previous-release previous-release	13.91ms - 14.75ms	slower ❌ 3% - 16% 0.39ms - 1.99ms	slower ❌ 10% - 18% 1.28ms - 2.28ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	452.17ms - 479.98ms	-	unsure 🔍 -4% - +6% -17.72ms - +28.23ms	faster ✔ 20% - 27% 114.89ms - 165.99ms
tip-of-tree tip-of-tree	442.53ms - 479.11ms	unsure 🔍 -6% - +4% -28.23ms - +17.72ms	-	faster ✔ 20% - 28% 117.52ms - 173.87ms
previous-release previous-release	585.08ms - 627.95ms	slower ❌ 24% - 36% 114.89ms - 165.99ms	slower ❌ 25% - 39% 117.52ms - 173.87ms	-

lit-html-template-heavy

Browser: chrome-headless 102.0.5005.61
Sample size: 50
Built by: Benchmarks #3332
Commit: f6b5c85

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	66.96ms - 72.44ms	-	unsure 🔍 -8% - +3% -6.06ms - +1.96ms	faster ✔ 13% - 22% 10.13ms - 19.23ms
tip-of-tree tip-of-tree	68.82ms - 74.68ms	unsure 🔍 -3% - +9% -1.96ms - +6.06ms	-	faster ✔ 10% - 20% 7.96ms - 17.30ms
previous-release previous-release	80.74ms - 88.02ms	slower ❌ 14% - 28% 10.13ms - 19.23ms	slower ❌ 11% - 25% 7.96ms - 17.30ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	151.07ms - 165.04ms	-	unsure 🔍 -8% - +4% -13.84ms - +6.73ms	faster ✔ 8% - 19% 13.20ms - 35.27ms
tip-of-tree tip-of-tree	154.06ms - 169.15ms	unsure 🔍 -4% - +9% -6.73ms - +13.84ms	-	faster ✔ 5% - 17% 9.29ms - 32.08ms
previous-release previous-release	173.75ms - 190.83ms	slower ❌ 8% - 23% 13.20ms - 35.27ms	slower ❌ 5% - 20% 9.29ms - 32.08ms	-

reactive-element-list

Browser: chrome-headless 102.0.5005.61
Sample size: 50
Built by: Benchmarks #3332
Commit: f6b5c85

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	82.79ms - 98.88ms	-	unsure 🔍 -7% - +21% -5.42ms - +17.10ms	unsure 🔍 -8% - +19% -6.29ms - +15.83ms
tip-of-tree tip-of-tree	77.11ms - 92.87ms	unsure 🔍 -18% - +6% -17.10ms - +5.42ms	-	unsure 🔍 -14% - +11% -12.02ms - +9.87ms
previous-release previous-release	78.47ms - 93.66ms	unsure 🔍 -17% - +7% -15.83ms - +6.29ms	unsure 🔍 -12% - +14% -9.87ms - +12.02ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1020.80ms - 1250.82ms	-	unsure 🔍 -15% - +12% -175.45ms - +141.67ms	unsure 🔍 -12% - +17% -126.52ms - +182.09ms
tip-of-tree tip-of-tree	1043.54ms - 1261.86ms	unsure 🔍 -13% - +16% -141.67ms - +175.45ms	-	unsure 🔍 -10% - +18% -105.33ms - +194.67ms
previous-release previous-release	1005.15ms - 1210.90ms	unsure 🔍 -16% - +11% -182.09ms - +126.52ms	unsure 🔍 -17% - +9% -194.67ms - +105.33ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	947.41ms - 1002.01ms	-	unsure 🔍 -6% - +2% -58.74ms - +21.86ms	unsure 🔍 -4% - +4% -38.01ms - +35.27ms
tip-of-tree tip-of-tree	963.51ms - 1022.78ms	unsure 🔍 -2% - +6% -21.86ms - +58.74ms	-	unsure 🔍 -2% - +6% -21.34ms - +55.48ms
previous-release previous-release	951.64ms - 1000.51ms	unsure 🔍 -4% - +4% -35.27ms - +38.01ms	unsure 🔍 -6% - +2% -55.48ms - +21.34ms	-

_{tachometer-reporter-action v2 for Benchmarks}

justinfagnani · 2022-05-02T20:56:18Z

packages/labs/task/src/test/task_test.ts

        super.update(changedProperties);
        this.taskValue = this.task.value ?? this.task.error;
-        this.task.render({
+        this.task.render<string, string>({


Ideally this could all be inferred from both the task definition and the callbacks. What's preventing that now?

just talked with steve for a bit! and if i understand correctly, i agree :)

uploaded a change that infers types from the StatusRenderer provided by the user

taylor-vann · 2022-05-02T21:43:20Z

The user can explicitly declare typings for return type and error type:

Those are combined with the return types in the provided StatusRender;

taylor-vann · 2022-05-02T21:45:36Z

if a user doesn't provide types:

TS will do its best to infer return types from the provided StatusRender

taylor-vann · 2022-05-02T22:29:34Z

this doesn't quite cover all use cases in #2301
I want to see if i can infer the return type based on status

taylor-vann · 2022-05-04T19:15:18Z

The current code is a little gross. Took some liberties to get a functioning concept.

taylor-vann · 2022-05-05T19:11:33Z

related to: #2301

mohe2015 · 2022-05-07T10:32:41Z

packages/labs/task/src/task.ts

-  render(renderer: StatusRenderer<R>) {
+  render<SR extends StatusRenderer<R, E>>(
+    renderer: Partial<SR>
+  ): ReturnType<SR[this['status']]> | undefined {


I think this is suboptimal but I think I didn't manage better without type assertions. If I understand correctly this means that undefined is always a possible return value even when all 4 functions are defined and don't return undefined

Don't understand though why in your screenshot above this is not the case.

Why do we need the | undefined?

A partial record of callbacks will always return | undefined as a possible type. If it were a partial record of numbers it'd be the same: number | undefined

Actually if that's true than we don't need it

And there we go, we don't need it. Updated.

sorvell · 2022-05-09T15:12:27Z

packages/labs/task/src/task.ts

-  PENDING: 1,
-  COMPLETE: 2,
-  ERROR: 3,
+  INITIAL: 'initial',


Please add a comment that the strings are important as they are used for typing.

sounds good! done!

sorvell · 2022-05-09T15:16:59Z

packages/labs/task/src/task.ts

-  render(renderer: StatusRenderer<R>) {
+  render<SR extends StatusRenderer<R, E>>(
+    renderer: Partial<SR>
+  ): ReturnType<SR[this['status']]> | undefined {


Why do we need the | undefined?

sorvell · 2022-05-09T15:18:33Z

packages/labs/task/src/task.ts

-  complete?: (value: R) => unknown;
-  error?: (error: unknown) => unknown;
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+export type StatusRenderer<R, E, I = any, P = any, C = any, ER = any> = {


Why do we need types for I, P, C, ER. Isn't the idea that we're inferring them?

The i, P, C, ER is to hint to the TS compiler that these values should be infered.
The default value of any is so a type inference is not made at first, they could be any type.
When return values for StatusRenderer have a value of unknown then an unknown type is cast and anything that extends StatusRenderer will also have an unknown return type

Heres a small demo of that behavior in action.

In the demo below, the function can't infer a type from values set as unknown. However in can infer types on values with a default of any
[edit, need a second take on the demo]

Here's an example of the slight difference between any and unknown when type inferencing.
Extending an interface with an unknown type actually casts a type of unknown and it needs to be reduced later somehow.

https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgKogNYgPYHcQBC2AHgDwAqyAvMgK6Y74A0yAStXQ3iAHzIDeAKGQjkACwgAbSdgBcyABQBKan3IBuYaLF4AJgE95y1W00BfQYNCRYiFAHUpkgNKNCJAVpETpcxSqo+ACIfGSCmL3E9Q38TEOjwwQtBGHoEMGBsEGQZAHMiMkdpABEIGFAIXQLkCGJIEF0AZzQufAKeBQAjEnkiyVLykEqCpXlWCDBaKBByfQAHCFI+gYqqkgBteNwDIIBdPiFRZCgJqezu4gA6HW39ZXNLBCzGsGQLgEYOPIKFflC-YyBZAhJzYcJRW5GALBG47MxKSzWaDwJDIACCIH0swWazIlBocExLHYBMxB0i-yhJg0FOiVKBrAeVnAyLsyAAkiAYNBhh5DtpQfSYaDEkdYTFATCEhFkqkQOlMtk8hj9AUlk4VkNcTU6hAGs0VdjecQOhdehqyqsRmNTtMjeqSpatQVNuK9uSjidJtM3iRrqD7klHs9XhcAEzyF5QUC5L7YXIqn5-QWxIEg3zg8VC4Fu+GaJ4gF6+4gAZkjYGjIFjNGVmKTlNTwozLCzjZzCXhQA

@justinfagnani maybe you might know of a more ergonomic way to pull types out of an interface supplied by a client?

opted to use casting over any for type inference.

taylor-vann · 2022-06-07T18:24:00Z

removed use of type inference with any in favor of casting from the client interface.

e111077 · 2022-10-13T23:46:47Z

packages/labs/task/src/task.ts

 */
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
-export class Task<T extends [...unknown[]] = any, R = any> {
+export class Task<T extends unknown[] = any, R = any, E = any> {


Since you're casting all instances of E to as E, do you even need E on Task? can't _error simply just be Error and StatusRenderer<R, E> be StatusRenderer<R, E extends Error = Error>?

justinfagnani · 2023-07-10T20:14:51Z

Superseded by #4008

add render return value type

107d084

taylor-vann requested a review from sorvell May 2, 2022 20:10

taylor-vann requested a review from justinfagnani as a code owner May 2, 2022 20:10

justinfagnani reviewed May 2, 2022

View reviewed changes

taylor-vann added 3 commits May 2, 2022 14:08

infer types from StatusRenderer passed to render

7bbe130

allow for explicit types and infer types

beca8d8

opt-in for typed error

e0022da

taylor-vann added 3 commits May 2, 2022 14:47

remove error typing, out of scope

9d9415d

error as required

0cec632

remove taskConfig typing

6e628b5

use linter to hush any init

5132db5

mohe2015 reviewed May 7, 2022

View reviewed changes

sorvell requested changes May 9, 2022

View reviewed changes

taylor-vann added 10 commits May 18, 2022 14:48

Merge branch 'main' into task__initial_typed_value

4792539

remove unknown spreads

05437b3

revert T args as first arg

6c41420

Merge branch 'main' into task__initial_typed_value

de5ab5d

Merge branch 'main' into task__initial_typed_value

877e313

remove undefined, it is implied

d56e2a0

add comment about status infering types

934507e

add changeset

94d5dff

Merge branch 'main' into task__initial_typed_value

e0bc7eb

use cast over any

76185b3

taylor-vann added 2 commits June 8, 2022 13:42

revert TaskStatus to enum

9ca1d6b

remove TaskStatus comment

c34e532

taylor-vann mentioned this pull request Oct 13, 2022

[labs/task] render does not return a useful type #2301

Closed

e111077 requested changes Oct 13, 2022

View reviewed changes

taylor-vann changed the title ~~[@task] add render return value~~ [@labs/task] add render return value Oct 31, 2022

justinfagnani mentioned this pull request Jul 10, 2023

[labs/task] Infer the return type of Task.render() #4008

Merged

justinfagnani closed this Jul 10, 2023

Conversation

taylor-vann commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented May 2, 2022 • 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

render

update

update-reflect

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taylor-vann commented May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taylor-vann commented May 2, 2022

Uh oh!

taylor-vann commented May 2, 2022

Uh oh!

taylor-vann commented May 4, 2022

Uh oh!

taylor-vann commented May 5, 2022

Uh oh!

mohe2015 May 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taylor-vann May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taylor-vann May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taylor-vann commented May 2, 2022 •

edited

Loading

changeset-bot bot commented May 2, 2022 •

edited

Loading

github-actions bot commented May 2, 2022 •

edited

Loading

taylor-vann commented May 2, 2022 •

edited

Loading

mohe2015 May 7, 2022 •

edited

Loading

taylor-vann May 24, 2022 •

edited

Loading

taylor-vann May 24, 2022 •

edited

Loading