Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Jan 4, 2024

Conversation

rictic
Copy link
Collaborator

@rictic rictic commented Jan 3, 2024

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.

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.
Copy link

changeset-bot bot commented Jan 3, 2024

🦋 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

Copy link
Contributor

github-actions bot commented Jan 3, 2024

📊 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

render

VersionAvg timevs
49.16ms - 52.58ms-

update

VersionAvg timevs
531.79ms - 540.23ms-

update-reflect

VersionAvg timevs
533.97ms - 544.83ms-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs 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.60msunsure 🔍
-2% - +5%
-0.42ms - +0.88ms
-unsure 🔍
-2% - +4%
-0.44ms - +0.82ms
previous-release
previous-release
18.52ms - 19.36msunsure 🔍
-3% - +3%
-0.57ms - +0.65ms
unsure 🔍
-4% - +2%
-0.82ms - +0.44ms
-

update

VersionAvg timevs 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.14msunsure 🔍
-6% - +5%
-2.68ms - +2.07ms
-unsure 🔍
-2% - +9%
-0.73ms - +3.75ms
previous-release
previous-release
38.46ms - 41.58msunsure 🔍
-10% - +1%
-4.16ms - +0.52ms
unsure 🔍
-9% - +2%
-3.75ms - +0.73ms
-

nop-update

VersionAvg timevs 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.24msunsure 🔍
-5% - +4%
-0.58ms - +0.50ms
-unsure 🔍
-2% - +9%
-0.18ms - +0.99ms
previous-release
previous-release
11.00ms - 11.85msunsure 🔍
-8% - +1%
-1.00ms - +0.10ms
unsure 🔍
-8% - +1%
-0.99ms - +0.18ms
-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs 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.35msslower ❌
0% - 5%
0.18ms - 1.65ms
-slower ❌
1% - 6%
0.45ms - 1.85ms
previous-release
previous-release
33.21ms - 34.10msunsure 🔍
-3% - +1%
-0.91ms - +0.43ms
faster ✔
1% - 5%
0.45ms - 1.85ms
-

update

VersionAvg timevs 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.65msunsure 🔍
-2% - +2%
-1.24ms - +1.67ms
-slower ❌
1% - 4%
0.46ms - 2.63ms
previous-release
previous-release
70.57ms - 72.04msunsure 🔍
-4% - +0%
-2.74ms - +0.09ms
faster ✔
1% - 4%
0.46ms - 2.63ms
-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs 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.28msunsure 🔍
-2% - +2%
-0.53ms - +0.77ms
-unsure 🔍
-2% - +1%
-0.81ms - +0.52ms
previous-release
previous-release
34.48ms - 35.35msunsure 🔍
-1% - +2%
-0.34ms - +0.86ms
unsure 🔍
-2% - +2%
-0.52ms - +0.81ms
-

update

VersionAvg timevs 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.46msunsure 🔍
-1% - +1%
-4.23ms - +3.60ms
-unsure 🔍
-1% - +0%
-7.57ms - +0.57ms
previous-release
previous-release
557.21ms - 563.34msunsure 🔍
-0% - +1%
-1.01ms - +7.37ms
unsure 🔍
-0% - +1%
-0.57ms - +7.57ms
-

update-reflect

VersionAvg timevs 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.80msunsure 🔍
-1% - +1%
-3.47ms - +3.61ms
-unsure 🔍
-1% - +1%
-4.37ms - +3.40ms
previous-release
previous-release
558.86ms - 565.12msunsure 🔍
-1% - +1%
-3.57ms - +4.68ms
unsure 🔍
-1% - +1%
-3.40ms - +4.37ms
-

tachometer-reporter-action v2 for Benchmarks

Copy link
Contributor

github-actions bot commented Jan 3, 2024

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

Copy link
Member

@augustjk augustjk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
packages/reactive-element/src/reactive-element.ts Outdated Show resolved Hide resolved
Co-authored-by: Augustine Kim <augustinekim@google.com>
@rictic rictic requested a review from augustjk January 3, 2024 22:33
@rictic
Copy link
Collaborator Author

rictic commented Jan 3, 2024

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.

Copy link
Contributor

@AndrewJakubowicz AndrewJakubowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very subtle. Nice!

@rictic rictic merged commit 9a4d569 into main Jan 4, 2024
9 of 10 checks passed
@rictic rictic deleted the warn-request-update-event branch January 4, 2024 19:10
@lit-robot lit-robot mentioned this pull request Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants