[reactive-element] 3.0 hybrid decorators by justinfagnani · Pull Request #4140 · lit/lit

justinfagnani · 2023-08-24T21:59:32Z

Part of #4144

Make a single set of decorators that work in TypeScript with experimentalDecorators set to either true or false, and in Babel with version equal to "2023-05" (not working because of lack of metadata). This works by duck-typing the decorator arguments.

After this effort we will have one set of decorators that work under the following cases:

TypeScript with experimentalDecorators: true and no accessor keyword
TypeScript with experimentalDecorators: true and accessor keywords
TypeScript with experimentalDecorators: false and accessor keywords
Babel with experimentalDecorators: false and accessor keywords

The trade-off is that we have to have the same behavior in each configuration:

We can't skip reflection for initial property values
We have to wrap accessors as we can't detect the difference between auto-accessors and hand-written accessors with experimental decorators

Tasks:

changeset-bot · 2023-08-24T21:59:36Z

🦋 Changeset detected

Latest commit: 940151b

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

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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-25T01:08:24Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: faster ✔ 1% - 13% (0.11ms - 2.81ms)
^{this-change vs tip-of-tree}

render

this-change: 71.79ms - 75.90ms
this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +6% (-1.19ms - +1.69ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +1% (-1.57ms - +0.71ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +1% (-1.05ms - +0.55ms)
^{this-change vs tip-of-tree}

update

this-change: 784.94ms - 803.60ms
this-change, tip-of-tree, previous-release: unsure 🔍 -7% - +2% (-5.46ms - +1.71ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +1% (-2.52ms - +1.53ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -0% - +1% (-3.48ms - +9.00ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 750.61ms - 762.69ms
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-5.75ms - +6.41ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 116.0.5845.96
Sample size: 50
Built by: Benchmarks #5350
Commit: 940151b

render

Version	Avg time	vs
	71.79ms - 75.90ms	-

update

Version	Avg time	vs
	784.94ms - 803.60ms	-

update-reflect

Version	Avg time	vs
	750.61ms - 762.69ms	-

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

Browser: chrome-headless 116.0.5845.96
Sample size: 30
Built by: Benchmarks #5350
Commit: 940151b

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	27.28ms - 29.42ms	-	unsure 🔍 -4% - +6% -1.19ms - +1.69ms	unsure 🔍 -7% - +5% -2.01ms - +1.51ms
tip-of-tree tip-of-tree	27.14ms - 29.07ms	unsure 🔍 -6% - +4% -1.69ms - +1.19ms	-	unsure 🔍 -8% - +4% -2.20ms - +1.21ms
previous-release previous-release	27.20ms - 30.00ms	unsure 🔍 -5% - +7% -1.51ms - +2.01ms	unsure 🔍 -4% - +8% -1.21ms - +2.20ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	76.61ms - 81.17ms	-	unsure 🔍 -7% - +2% -5.46ms - +1.71ms	unsure 🔍 -3% - +6% -2.37ms - +4.32ms
tip-of-tree tip-of-tree	77.99ms - 83.53ms	unsure 🔍 -2% - +7% -1.71ms - +5.46ms	-	unsure 🔍 -1% - +8% -0.85ms - +6.54ms
previous-release previous-release	75.46ms - 80.37ms	unsure 🔍 -5% - +3% -4.32ms - +2.37ms	unsure 🔍 -8% - +1% -6.54ms - +0.85ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	18.45ms - 20.46ms	-	faster ✔ 1% - 13% 0.11ms - 2.81ms	unsure 🔍 -14% - +1% -2.86ms - +0.34ms
tip-of-tree tip-of-tree	20.02ms - 21.81ms	slower ❌ 0% - 15% 0.11ms - 2.81ms	-	unsure 🔍 -6% - +8% -1.33ms - +1.73ms
previous-release previous-release	19.47ms - 21.96ms	unsure 🔍 -2% - +15% -0.34ms - +2.86ms	unsure 🔍 -8% - +6% -1.73ms - +1.33ms	-

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

Browser: chrome-headless 116.0.5845.96
Sample size: 50
Built by: Benchmarks #5350
Commit: 940151b

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	49.91ms - 51.41ms	-	unsure 🔍 -3% - +1% -1.57ms - +0.71ms	unsure 🔍 -3% - +2% -1.29ms - +0.96ms
tip-of-tree tip-of-tree	50.24ms - 51.95ms	unsure 🔍 -1% - +3% -0.71ms - +1.57ms	-	unsure 🔍 -2% - +3% -0.93ms - +1.47ms
previous-release previous-release	49.98ms - 51.66ms	unsure 🔍 -2% - +3% -0.96ms - +1.29ms	unsure 🔍 -3% - +2% -1.47ms - +0.93ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	116.18ms - 119.23ms	-	unsure 🔍 -2% - +1% -2.52ms - +1.53ms	unsure 🔍 -1% - +2% -1.40ms - +2.85ms
tip-of-tree tip-of-tree	116.87ms - 119.54ms	unsure 🔍 -1% - +2% -1.53ms - +2.52ms	-	unsure 🔍 -1% - +3% -0.78ms - +3.21ms
previous-release previous-release	115.50ms - 118.47ms	unsure 🔍 -2% - +1% -2.85ms - +1.40ms	unsure 🔍 -3% - +1% -3.21ms - +0.78ms	-

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

Browser: chrome-headless 116.0.5845.96
Sample size: 50
Built by: Benchmarks #5350
Commit: 940151b

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	42.67ms - 43.71ms	-	unsure 🔍 -2% - +1% -1.05ms - +0.55ms	unsure 🔍 -2% - +1% -0.94ms - +0.49ms
tip-of-tree tip-of-tree	42.84ms - 44.04ms	unsure 🔍 -1% - +2% -0.55ms - +1.05ms	-	unsure 🔍 -2% - +2% -0.75ms - +0.80ms
previous-release previous-release	42.93ms - 43.90ms	unsure 🔍 -1% - +2% -0.49ms - +0.94ms	unsure 🔍 -2% - +2% -0.80ms - +0.75ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	784.14ms - 792.13ms	-	unsure 🔍 -0% - +1% -3.48ms - +9.00ms	unsure 🔍 -1% - +1% -7.21ms - +5.53ms
tip-of-tree tip-of-tree	780.59ms - 790.16ms	unsure 🔍 -1% - +0% -9.00ms - +3.48ms	-	unsure 🔍 -1% - +0% -10.49ms - +3.29ms
previous-release previous-release	784.02ms - 793.93ms	unsure 🔍 -1% - +1% -5.53ms - +7.21ms	unsure 🔍 -0% - +1% -3.29ms - +10.49ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	775.69ms - 785.02ms	-	unsure 🔍 -1% - +1% -5.75ms - +6.41ms	unsure 🔍 -1% - +0% -9.07ms - +3.30ms
tip-of-tree tip-of-tree	776.13ms - 783.92ms	unsure 🔍 -1% - +1% -6.41ms - +5.75ms	-	unsure 🔍 -1% - +0% -8.84ms - +2.42ms
previous-release previous-release	779.17ms - 787.30ms	unsure 🔍 -0% - +1% -3.30ms - +9.07ms	unsure 🔍 -0% - +1% -2.42ms - +8.84ms	-

_{tachometer-reporter-action v2 for Benchmarks}

github-actions · 2023-08-25T01:10:24Z

The size of lit-html.js and lit-core.min.js are as expected.

packages/reactive-element/tsconfig.std-decorators-tests.json

packages/reactive-element/src/legacy-decorators/state.ts

packages/reactive-element/src/legacy-decorators/query.ts

packages/reactive-element/src/legacy-decorators/query-assigned-nodes.ts

packages/reactive-element/src/legacy-decorators/event-options.ts

packages/reactive-element/src/legacy-decorators/query-all.ts

packages/reactive-element/src/legacy-decorators/custom-element.ts

packages/reactive-element/src/std-decorators/query-async.ts

packages/reactive-element/src/test/std-decorators/queryAsync_test.ts

AndrewJakubowicz

Nice!!! – all comments are non blocking nits

augustjk · 2023-08-29T20:39:13Z

packages/reactive-element/src/legacy-decorators/property.ts

 const legacyProperty = (
-  options: PropertyDeclaration,
+  options: PropertyDeclaration | undefined,
  proto: Object,


i know this is already merged but should we use object rather than Object here?

It's ultimately not that helpful of a type, but it's Object the because the prototype extends from Object the value.

augustjk · 2023-08-29T20:39:53Z

packages/reactive-element/src/legacy-decorators/property.ts

+          )
+        : legacyProperty(
+            options,
+            protoOrTarget as Object,


Should this be type object?

justinfagnani requested a review from rictic August 24, 2023 21:59

justinfagnani changed the base branch from main to 3.0 August 24, 2023 21:59

justinfagnani force-pushed the 3.0-hybrid-decorators branch from d38782a to 2be0a12 Compare August 25, 2023 22:37

augustjk added the 3.0 label Aug 25, 2023

justinfagnani added 2 commits August 25, 2023 21:43

Make legacy-decorators work as standard decorators

8d344a4

Fix std-decorators package.json and tsconfig

139430c

justinfagnani force-pushed the 3.0-hybrid-decorators branch from 8a524f1 to 139430c Compare August 26, 2023 04:45

justinfagnani marked this pull request as ready for review August 26, 2023 04:45

justinfagnani requested a review from kevinpschaaf as a code owner August 26, 2023 04:45

justinfagnani requested review from AndrewJakubowicz and augustjk August 26, 2023 04:45

justinfagnani mentioned this pull request Aug 26, 2023

[reactive-element] Hybrid decorators #4144

Closed

10 tasks

rictic reviewed Aug 28, 2023

View reviewed changes

rictic approved these changes Aug 28, 2023

View reviewed changes

justinfagnani changed the title ~~3.0 hybrid decorators~~ [reactive-element] 3.0 hybrid decorators Aug 28, 2023

Address feedback

13fe425

AndrewJakubowicz reviewed Aug 28, 2023

View reviewed changes

packages/reactive-element/src/legacy-decorators/query-assigned-nodes.ts Outdated Show resolved Hide resolved

AndrewJakubowicz reviewed Aug 28, 2023

View reviewed changes

packages/reactive-element/src/legacy-decorators/event-options.ts Outdated Show resolved Hide resolved