[reactive-element] Make context decorators work with standard decorators by justinfagnani · Pull Request #4151 · lit/lit

justinfagnani · 2023-08-28T19:45:07Z

Part of #4144

One thing to check on here is how optional fields are translated to auto-accessors and whether or not we have the types correct, and had them correct prior to this PR. The exist tests include a case of an optional field decorated with a non-optional context type. That doesn't exactly work with accessors, which can't be optional, and so have to include undefined in their type to get the same effect. This difference is that this setup requires the context include undefined, whereas optional fields don't. The actual type of optional fields is the same though, so it seems like something was wrong in the original types.

changeset-bot · 2023-08-28T19:45:11Z

🦋 Changeset detected

Latest commit: 43175a2

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/context	Minor

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-28T19:45:24Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +11% (-0.59ms - +2.00ms)
^{this-change vs tip-of-tree}

render

this-change: 73.12ms - 76.66ms
this-change, tip-of-tree, previous-release: unsure 🔍 -7% - +2% (-2.06ms - +0.62ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +1% (-1.48ms - +0.50ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-0.96ms - +0.83ms)
^{this-change vs tip-of-tree}

update

this-change: 789.74ms - 800.62ms
this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +4% (-2.86ms - +2.86ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-2.38ms - +1.72ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-6.95ms - +8.51ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 755.44ms - 762.15ms
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-8.47ms - +8.00ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 116.0.5845.96
Sample size: 50
Built by: Benchmarks #5375
Commit: 43175a2

render

Version	Avg time	vs
	73.12ms - 76.66ms	-

update

Version	Avg time	vs
	789.74ms - 800.62ms	-

update-reflect

Version	Avg time	vs
	755.44ms - 762.15ms	-

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

Browser: chrome-headless 116.0.5845.96
Sample size: 30
Built by: Benchmarks #5375
Commit: 43175a2

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	26.75ms - 28.84ms	-	unsure 🔍 -7% - +2% -2.06ms - +0.62ms	unsure 🔍 -6% - +4% -1.62ms - +1.23ms
tip-of-tree tip-of-tree	27.68ms - 29.35ms	unsure 🔍 -2% - +7% -0.62ms - +2.06ms	-	unsure 🔍 -3% - +6% -0.75ms - +1.80ms
previous-release previous-release	27.03ms - 28.95ms	unsure 🔍 -4% - +6% -1.23ms - +1.62ms	unsure 🔍 -6% - +3% -1.80ms - +0.75ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	71.81ms - 76.06ms	-	unsure 🔍 -4% - +4% -2.86ms - +2.86ms	unsure 🔍 -5% - +3% -4.03ms - +2.58ms
tip-of-tree tip-of-tree	72.02ms - 75.85ms	unsure 🔍 -4% - +4% -2.86ms - +2.86ms	-	unsure 🔍 -5% - +3% -3.90ms - +2.46ms
previous-release previous-release	72.13ms - 77.19ms	unsure 🔍 -4% - +5% -2.58ms - +4.03ms	unsure 🔍 -3% - +5% -2.46ms - +3.90ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	18.75ms - 20.82ms	-	unsure 🔍 -3% - +11% -0.59ms - +2.00ms	unsure 🔍 -7% - +9% -1.40ms - +1.68ms
tip-of-tree tip-of-tree	18.31ms - 19.86ms	unsure 🔍 -10% - +3% -2.00ms - +0.59ms	-	unsure 🔍 -10% - +4% -1.94ms - +0.81ms
previous-release previous-release	18.51ms - 20.78ms	unsure 🔍 -8% - +7% -1.68ms - +1.40ms	unsure 🔍 -4% - +10% -0.81ms - +1.94ms	-

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

Browser: chrome-headless 116.0.5845.96
Sample size: 50
Built by: Benchmarks #5375
Commit: 43175a2

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	48.86ms - 50.13ms	-	unsure 🔍 -3% - +1% -1.48ms - +0.50ms	unsure 🔍 -3% - +0% -1.55ms - +0.22ms
tip-of-tree tip-of-tree	49.23ms - 50.75ms	unsure 🔍 -1% - +3% -0.50ms - +1.48ms	-	unsure 🔍 -2% - +2% -1.15ms - +0.81ms
previous-release previous-release	49.55ms - 50.78ms	unsure 🔍 -0% - +3% -0.22ms - +1.55ms	unsure 🔍 -2% - +2% -0.81ms - +1.15ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	111.02ms - 113.86ms	-	unsure 🔍 -2% - +2% -2.38ms - +1.72ms	unsure 🔍 -3% - +1% -3.25ms - +1.06ms
tip-of-tree tip-of-tree	111.30ms - 114.25ms	unsure 🔍 -2% - +2% -1.72ms - +2.38ms	-	unsure 🔍 -3% - +1% -2.95ms - +1.43ms
previous-release previous-release	111.92ms - 115.15ms	unsure 🔍 -1% - +3% -1.06ms - +3.25ms	unsure 🔍 -1% - +3% -1.43ms - +2.95ms	-

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

Browser: chrome-headless 116.0.5845.96
Sample size: 50
Built by: Benchmarks #5375
Commit: 43175a2

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	44.05ms - 45.25ms	-	unsure 🔍 -2% - +2% -0.96ms - +0.83ms	unsure 🔍 -2% - +2% -0.95ms - +0.71ms
tip-of-tree tip-of-tree	44.05ms - 45.38ms	unsure 🔍 -2% - +2% -0.83ms - +0.96ms	-	unsure 🔍 -2% - +2% -0.94ms - +0.82ms
previous-release previous-release	44.20ms - 45.34ms	unsure 🔍 -2% - +2% -0.71ms - +0.95ms	unsure 🔍 -2% - +2% -0.82ms - +0.94ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	815.04ms - 825.22ms	-	unsure 🔍 -1% - +1% -6.95ms - +8.51ms	unsure 🔍 -1% - +1% -6.66ms - +8.89ms
tip-of-tree tip-of-tree	813.54ms - 825.17ms	unsure 🔍 -1% - +1% -8.51ms - +6.95ms	-	unsure 🔍 -1% - +1% -7.94ms - +8.61ms
previous-release previous-release	813.13ms - 824.90ms	unsure 🔍 -1% - +1% -8.89ms - +6.66ms	unsure 🔍 -1% - +1% -8.61ms - +7.94ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	808.91ms - 820.44ms	-	unsure 🔍 -1% - +1% -8.47ms - +8.00ms	unsure 🔍 -1% - +2% -5.06ms - +12.15ms
tip-of-tree tip-of-tree	809.03ms - 820.79ms	unsure 🔍 -1% - +1% -8.00ms - +8.47ms	-	unsure 🔍 -1% - +2% -4.90ms - +12.46ms
previous-release previous-release	804.74ms - 817.51ms	unsure 🔍 -1% - +1% -12.15ms - +5.06ms	unsure 🔍 -2% - +1% -12.46ms - +4.90ms	-

_{tachometer-reporter-action v2 for Benchmarks}

github-actions · 2023-08-28T19:47:12Z

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

rictic

This is a small subset of the tests in https://github.com/lit/lit/pull/4009/files

I can land that PR after this one and land all of the tests as part of that

packages/labs/context/src/lib/decorators/consume.ts

Co-authored-by: Benny Powers <bennypowers@users.noreply.github.com>

justinfagnani · 2023-08-29T20:04:50Z

I didn't copy over all of the tests because I'm not entirely happy with how the tests are organized right now. Ideally I would have just copied over the decorator tests, but there aren't tests for just the decorators. There are tests for scenarios, and some components of the tests directly use controllers or use decorators.

It's a little tricky because of the two-part nature of the protocol. It's easier to test a consumer if you have a provider and vice-versa. I don't think it's that great to reimplement half of the context protocol with the possibility of mistakes there. It would be great if there were common protocol interop tests. But even with a combined comsumer/provider test, that could use the controllers only and then we could have isolated and smaller tests for decorators.

packages/labs/context/src/lib/decorators/provide.ts

packages/reactive-element/src/test/decorators/property_test.ts

justinfagnani added 8 commits August 25, 2023 21:43

Make legacy-decorators work as standard decorators

8d344a4

Fix std-decorators package.json and tsconfig

139430c

Make standard property decorator reflect initial values

ad51e8f

Update bundle size

d8b59bd

Always wrap user accessors

89e513e

Fix how @Property interacts with other decorators

ab137c8

Make consume and provide not use decorateProperty

93d3080

Make context decorators work with standard decorators

1dc3408

justinfagnani requested a review from rictic August 28, 2023 19:45

justinfagnani requested review from benjamind and kevinpschaaf as code owners August 28, 2023 19:45

AndrewJakubowicz added the 3.0 label Aug 28, 2023

rictic reviewed Aug 29, 2023

View reviewed changes

packages/labs/context/src/lib/decorators/consume.ts Show resolved Hide resolved

packages/labs/context/src/lib/decorators/consume.ts Outdated Show resolved Hide resolved

justinfagnani and others added 7 commits August 29, 2023 11:32

Merge branch '3.0' into 3.0-hybrid-decorators-3

0e0ab86

Update .changeset/hungry-cups-destroy.md

3a6c78e

Co-authored-by: Benny Powers <bennypowers@users.noreply.github.com>

Remove commented out test lines

74a04e5

Make the property descriptors more static

e928a12

Better accessor test

382ffcf

Smaller getPropertyDescriptor

fba4276

Merge branch '3.0-hybrid-decorators-3' into 3.0-hybrid-decorators-4

5fae086

Address feedback

36d57c6

Base automatically changed from 3.0-hybrid-decorators-3 to 3.0 August 30, 2023 05:15

Merge branch '3.0' into 3.0-hybrid-decorators-4

7088c40

rictic approved these changes Aug 30, 2023

View reviewed changes

packages/labs/context/src/lib/decorators/provide.ts Show resolved Hide resolved

packages/reactive-element/src/test/decorators/property_test.ts Outdated Show resolved Hide resolved

Addrss feedback

43175a2

justinfagnani merged commit 5680d48 into 3.0 Aug 30, 2023

justinfagnani deleted the 3.0-hybrid-decorators-4 branch August 30, 2023 18:50

This was referenced Sep 28, 2023

Version Packages (pre) #4244

Closed

Version Packages (pre) #4246

Merged

Conversation

justinfagnani commented Aug 28, 2023

Uh oh!

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

github-actions bot commented Aug 28, 2023

Uh oh!

rictic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

justinfagnani commented Aug 29, 2023

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Aug 28, 2023 •

edited

Loading

github-actions bot commented Aug 28, 2023 •

edited

Loading