Add a warning when binding this.requestUpdate to an event listener by rictic · Pull Request #4473 · lit/lit

rictic · 2024-01-03T17:44:00Z

This is an uncommon use case to do in real code, but a reasonable thing to try when experimenting and debugging, so we want to help someone out with a warning here.

changeset-bot · 2024-01-03T17:44:04Z

🦋 Changeset detected

Latest commit: a91e150

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

This PR includes changesets to release 3 packages

Name	Type
@lit/reactive-element	Patch
lit-element	Patch
lit	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 · 2024-01-03T17:44:19Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +5% (-0.50ms - +0.58ms)
^{this-change vs tip-of-tree}

render

this-change: 49.16ms - 52.58ms
this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +2% (-0.88ms - +0.42ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: faster ✔ 1% - 5% (0.18ms - 1.65ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-0.77ms - +0.53ms)
^{this-change vs tip-of-tree}

update

this-change: 531.79ms - 540.23ms
this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +6% (-2.07ms - +2.68ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-1.67ms - +1.24ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-3.60ms - +4.23ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 533.97ms - 544.83ms
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-3.61ms - +3.47ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 120.0.6099.109
Sample size: 50
Built by: Benchmarks #5971
Commit: a91e150

render

Version	Avg time	vs
	49.16ms - 52.58ms	-

update

Version	Avg time	vs
	531.79ms - 540.23ms	-

update-reflect

Version	Avg time	vs
	533.97ms - 544.83ms	-

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

Browser: chrome-headless 120.0.6099.109
Sample size: 30
Built by: Benchmarks #5971
Commit: a91e150

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	18.45ms - 19.35ms	-	unsure 🔍 -5% - +2% -0.88ms - +0.42ms	unsure 🔍 -3% - +3% -0.65ms - +0.57ms
tip-of-tree tip-of-tree	18.66ms - 19.60ms	unsure 🔍 -2% - +5% -0.42ms - +0.88ms	-	unsure 🔍 -2% - +4% -0.44ms - +0.82ms
previous-release previous-release	18.52ms - 19.36ms	unsure 🔍 -3% - +3% -0.57ms - +0.65ms	unsure 🔍 -4% - +2% -0.82ms - +0.44ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	40.09ms - 43.58ms	-	unsure 🔍 -5% - +6% -2.07ms - +2.68ms	unsure 🔍 -1% - +11% -0.52ms - +4.16ms
tip-of-tree tip-of-tree	39.92ms - 43.14ms	unsure 🔍 -6% - +5% -2.68ms - +2.07ms	-	unsure 🔍 -2% - +9% -0.73ms - +3.75ms
previous-release previous-release	38.46ms - 41.58ms	unsure 🔍 -10% - +1% -4.16ms - +0.52ms	unsure 🔍 -9% - +2% -3.75ms - +0.73ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	11.52ms - 12.23ms	-	unsure 🔍 -4% - +5% -0.50ms - +0.58ms	unsure 🔍 -1% - +9% -0.10ms - +1.00ms
tip-of-tree tip-of-tree	11.43ms - 12.24ms	unsure 🔍 -5% - +4% -0.58ms - +0.50ms	-	unsure 🔍 -2% - +9% -0.18ms - +0.99ms
previous-release previous-release	11.00ms - 11.85ms	unsure 🔍 -8% - +1% -1.00ms - +0.10ms	unsure 🔍 -8% - +1% -0.99ms - +0.18ms	-

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

Browser: chrome-headless 120.0.6099.109
Sample size: 50
Built by: Benchmarks #5971
Commit: a91e150

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.39ms - 34.40ms	-	faster ✔ 1% - 5% 0.18ms - 1.65ms	unsure 🔍 -1% - +3% -0.43ms - +0.91ms
tip-of-tree tip-of-tree	34.27ms - 35.35ms	slower ❌ 0% - 5% 0.18ms - 1.65ms	-	slower ❌ 1% - 6% 0.45ms - 1.85ms
previous-release previous-release	33.21ms - 34.10ms	unsure 🔍 -3% - +1% -0.91ms - +0.43ms	faster ✔ 1% - 5% 0.45ms - 1.85ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	71.42ms - 73.84ms	-	unsure 🔍 -2% - +2% -1.67ms - +1.24ms	unsure 🔍 -0% - +4% -0.09ms - +2.74ms
tip-of-tree tip-of-tree	72.05ms - 73.65ms	unsure 🔍 -2% - +2% -1.24ms - +1.67ms	-	slower ❌ 1% - 4% 0.46ms - 2.63ms
previous-release previous-release	70.57ms - 72.04ms	unsure 🔍 -4% - +0% -2.74ms - +0.09ms	faster ✔ 1% - 4% 0.46ms - 2.63ms	-

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

Browser: chrome-headless 120.0.6099.109
Sample size: 50
Built by: Benchmarks #5971
Commit: a91e150

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	34.24ms - 35.07ms	-	unsure 🔍 -2% - +2% -0.77ms - +0.53ms	unsure 🔍 -2% - +1% -0.86ms - +0.34ms
tip-of-tree tip-of-tree	34.27ms - 35.28ms	unsure 🔍 -2% - +2% -0.53ms - +0.77ms	-	unsure 🔍 -2% - +1% -0.81ms - +0.52ms
previous-release previous-release	34.48ms - 35.35ms	unsure 🔍 -1% - +2% -0.34ms - +0.86ms	unsure 🔍 -2% - +2% -0.52ms - +0.81ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	554.24ms - 559.95ms	-	unsure 🔍 -1% - +1% -3.60ms - +4.23ms	unsure 🔍 -1% - +0% -7.37ms - +1.01ms
tip-of-tree tip-of-tree	554.10ms - 559.46ms	unsure 🔍 -1% - +1% -4.23ms - +3.60ms	-	unsure 🔍 -1% - +0% -7.57ms - +0.57ms
previous-release previous-release	557.21ms - 563.34ms	unsure 🔍 -0% - +1% -1.01ms - +7.37ms	unsure 🔍 -0% - +1% -0.57ms - +7.57ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	558.75ms - 564.12ms	-	unsure 🔍 -1% - +1% -3.61ms - +3.47ms	unsure 🔍 -1% - +1% -4.68ms - +3.57ms
tip-of-tree tip-of-tree	559.20ms - 563.80ms	unsure 🔍 -1% - +1% -3.47ms - +3.61ms	-	unsure 🔍 -1% - +1% -4.37ms - +3.40ms
previous-release previous-release	558.86ms - 565.12ms	unsure 🔍 -1% - +1% -3.57ms - +4.68ms	unsure 🔍 -1% - +1% -3.40ms - +4.37ms	-

_{tachometer-reporter-action v2 for Benchmarks}

github-actions · 2024-01-03T17:46:02Z

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

augustjk

Seems like a hyper specific case to be looking for, but I suppose there's no harm in adding just a dev mode warning.

.changeset/ninety-bananas-sneeze.md

packages/reactive-element/src/reactive-element.ts

Co-authored-by: Augustine Kim <augustinekim@google.com>

packages/reactive-element/src/reactive-element.ts

rictic · 2024-01-03T22:56:51Z

Seems like a hyper specific case to be looking for, but I suppose there's no harm in adding just a dev mode warning.

It is, but the check is easy, and the behavior was weird enough that it had me stumped for a bit.

AndrewJakubowicz

This is very subtle. Nice!

Add a warning when binding this.requestUpdate to an event listener

e0cc639

This is an uncommon use case to do in real code, but a reasonable thing to try when experimenting and debugging, so we want to help someone out with a warning here.

rictic requested a review from kevinpschaaf as a code owner January 3, 2024 17:44

Changeset.

5c0e523

augustjk reviewed Jan 3, 2024

View reviewed changes

.changeset/ninety-bananas-sneeze.md Show resolved Hide resolved

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

Update .changeset/ninety-bananas-sneeze.md

1c8227f

Co-authored-by: Augustine Kim <augustinekim@google.com>

rictic commented Jan 3, 2024

View reviewed changes

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

Remove short code

6415bb2

rictic requested a review from augustjk January 3, 2024 22:33

Merge branch 'main' into warn-request-update-event

a91e150

augustjk approved these changes Jan 3, 2024

View reviewed changes

AndrewJakubowicz approved these changes Jan 3, 2024

View reviewed changes

rictic merged commit 9a4d569 into main Jan 4, 2024

rictic deleted the warn-request-update-event branch January 4, 2024 19:10

lit-robot mentioned this pull request Jan 9, 2024

Version Packages #4482

Merged

Conversation

rictic commented Jan 3, 2024

Uh oh!

changeset-bot bot commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Jan 3, 2024 • 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 Jan 3, 2024

Uh oh!

augustjk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rictic commented Jan 3, 2024

Uh oh!

AndrewJakubowicz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Jan 3, 2024 •

edited

Loading

github-actions bot commented Jan 3, 2024 •

edited

Loading