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

Fix change-in-update warning from Tasks #3660

Merged
merged 3 commits into from Feb 10, 2023
Merged

Conversation

justinfagnani
Copy link
Collaborator

@justinfagnani justinfagnani commented Feb 9, 2023

Fixes #3566

The fix is to call the initial host.requestUpdate() (which is there to get the element to update and see the PENDING state of the task) in a microtask, so that it goes after the change-in-update check.

@changeset-bot
Copy link

changeset-bot bot commented Feb 9, 2023

🦋 Changeset detected

Latest commit: aa6aece

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/task 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
Copy link
Contributor

github-actions bot commented Feb 9, 2023

📊 Tachometer Benchmark Results

Summary

nop-update

  • lit-html-kitchen-sink: unsure 🔍 -10% - +12% (-2.41ms - +2.76ms)
    this-change vs tip-of-tree

render

  • lit-element-list: 75.18ms - 80.11ms
  • lit-html-kitchen-sink: unsure 🔍 -2% - +7% (-0.67ms - +2.01ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -8% - +1% (-0.95ms - +0.12ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -4% - +0% (-2.17ms - +0.14ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -2% - +2% (-0.99ms - +1.15ms)
    this-change vs tip-of-tree

update

  • lit-element-list: 683.94ms - 696.00ms
  • lit-html-kitchen-sink: unsure 🔍 -5% - +4% (-4.02ms - +3.00ms)
    this-change vs tip-of-tree
  • lit-html-repeat: unsure 🔍 -4% - +1% (-9.52ms - +3.73ms)
    this-change vs tip-of-tree
  • lit-html-template-heavy: unsure 🔍 -1% - +1% (-1.49ms - +0.59ms)
    this-change vs tip-of-tree
  • reactive-element-list: unsure 🔍 -0% - +1% (-2.68ms - +3.96ms)
    this-change vs tip-of-tree

update-reflect

  • lit-element-list: 687.25ms - 696.27ms
  • reactive-element-list: unsure 🔍 -1% - +0% (-3.96ms - +3.47ms)
    this-change vs tip-of-tree

Results

lit-element-list

render

VersionAvg timevs
75.18ms - 80.11ms-

update

VersionAvg timevs
683.94ms - 696.00ms-

update-reflect

VersionAvg timevs
687.25ms - 696.27ms-
lit-html-kitchen-sink

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
30.22ms - 32.53ms-unsure 🔍
-2% - +7%
-0.67ms - +2.01ms
unsure 🔍
-3% - +5%
-1.04ms - +1.50ms
tip-of-tree
tip-of-tree
30.02ms - 31.38msunsure 🔍
-6% - +2%
-2.01ms - +0.67ms
-unsure 🔍
-4% - +1%
-1.30ms - +0.42ms
previous-release
previous-release
30.62ms - 31.68msunsure 🔍
-5% - +3%
-1.50ms - +1.04ms
unsure 🔍
-1% - +4%
-0.42ms - +1.30ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
75.65ms - 80.03ms-unsure 🔍
-5% - +4%
-4.02ms - +3.00ms
unsure 🔍
-5% - +4%
-3.79ms - +2.93ms
tip-of-tree
tip-of-tree
75.61ms - 81.09msunsure 🔍
-4% - +5%
-3.00ms - +4.02ms
-unsure 🔍
-5% - +5%
-3.66ms - +3.83ms
previous-release
previous-release
75.71ms - 80.81msunsure 🔍
-4% - +5%
-2.93ms - +3.79ms
unsure 🔍
-5% - +5%
-3.83ms - +3.66ms
-

nop-update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
21.93ms - 26.04ms-unsure 🔍
-10% - +12%
-2.41ms - +2.76ms
unsure 🔍
-14% - +5%
-3.58ms - +1.33ms
tip-of-tree
tip-of-tree
22.23ms - 25.38msunsure 🔍
-11% - +10%
-2.76ms - +2.41ms
-unsure 🔍
-13% - +3%
-3.37ms - +0.76ms
previous-release
previous-release
23.77ms - 26.45msunsure 🔍
-6% - +15%
-1.33ms - +3.58ms
unsure 🔍
-3% - +14%
-0.76ms - +3.37ms
-
lit-html-repeat

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
11.35ms - 12.06ms-unsure 🔍
-8% - +1%
-0.95ms - +0.12ms
unsure 🔍
-4% - +5%
-0.52ms - +0.56ms
tip-of-tree
tip-of-tree
11.72ms - 12.52msunsure 🔍
-1% - +8%
-0.12ms - +0.95ms
-unsure 🔍
-1% - +9%
-0.13ms - +1.00ms
previous-release
previous-release
11.28ms - 12.09msunsure 🔍
-5% - +4%
-0.56ms - +0.52ms
unsure 🔍
-8% - +1%
-1.00ms - +0.13ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
263.61ms - 271.73ms-unsure 🔍
-4% - +1%
-9.52ms - +3.73ms
unsure 🔍
-2% - +3%
-4.78ms - +7.54ms
tip-of-tree
tip-of-tree
265.34ms - 275.80msunsure 🔍
-1% - +4%
-3.73ms - +9.52ms
-unsure 🔍
-1% - +4%
-2.71ms - +11.26ms
previous-release
previous-release
261.66ms - 270.93msunsure 🔍
-3% - +2%
-7.54ms - +4.78ms
unsure 🔍
-4% - +1%
-11.26ms - +2.71ms
-
lit-html-template-heavy

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
50.58ms - 51.92ms-unsure 🔍
-4% - +0%
-2.17ms - +0.14ms
unsure 🔍
-3% - +1%
-1.77ms - +0.36ms
tip-of-tree
tip-of-tree
51.33ms - 53.20msunsure 🔍
-0% - +4%
-0.14ms - +2.17ms
-unsure 🔍
-2% - +3%
-0.94ms - +1.56ms
previous-release
previous-release
51.13ms - 52.78msunsure 🔍
-1% - +3%
-0.36ms - +1.77ms
unsure 🔍
-3% - +2%
-1.56ms - +0.94ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
105.00ms - 106.47ms-unsure 🔍
-1% - +1%
-1.49ms - +0.59ms
unsure 🔍
-1% - +1%
-0.80ms - +1.24ms
tip-of-tree
tip-of-tree
105.46ms - 106.92msunsure 🔍
-1% - +1%
-0.59ms - +1.49ms
-unsure 🔍
-0% - +2%
-0.34ms - +1.70ms
previous-release
previous-release
104.81ms - 106.22msunsure 🔍
-1% - +1%
-1.24ms - +0.80ms
unsure 🔍
-2% - +0%
-1.70ms - +0.34ms
-
reactive-element-list

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
49.12ms - 50.74ms-unsure 🔍
-2% - +2%
-0.99ms - +1.15ms
unsure 🔍
-3% - +2%
-1.35ms - +0.84ms
tip-of-tree
tip-of-tree
49.16ms - 50.54msunsure 🔍
-2% - +2%
-1.15ms - +0.99ms
-unsure 🔍
-3% - +1%
-1.34ms - +0.67ms
previous-release
previous-release
49.45ms - 50.91msunsure 🔍
-2% - +3%
-0.84ms - +1.35ms
unsure 🔍
-1% - +3%
-0.67ms - +1.34ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
690.13ms - 694.21ms-unsure 🔍
-0% - +1%
-2.68ms - +3.96ms
unsure 🔍
-0% - +0%
-2.55ms - +3.01ms
tip-of-tree
tip-of-tree
688.91ms - 694.15msunsure 🔍
-1% - +0%
-3.96ms - +2.68ms
-unsure 🔍
-1% - +0%
-3.64ms - +2.81ms
previous-release
previous-release
690.06ms - 693.82msunsure 🔍
-0% - +0%
-3.01ms - +2.55ms
unsure 🔍
-0% - +1%
-2.81ms - +3.64ms
-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
718.88ms - 724.13ms-unsure 🔍
-1% - +0%
-3.96ms - +3.47ms
unsure 🔍
-0% - +1%
-2.44ms - +5.40ms
tip-of-tree
tip-of-tree
719.12ms - 724.37msunsure 🔍
-0% - +1%
-3.47ms - +3.96ms
-unsure 🔍
-0% - +1%
-2.20ms - +5.64ms
previous-release
previous-release
717.11ms - 722.93msunsure 🔍
-1% - +0%
-5.40ms - +2.44ms
unsure 🔍
-1% - +0%
-5.64ms - +2.20ms
-

tachometer-reporter-action v2 for Benchmarks

@e111077
Copy link
Member

e111077 commented Feb 10, 2023

What if you need DOM access or willUpdate property resolution?

@justinfagnani
Copy link
Collaborator Author

@e111077 that's a great question. We don't have any tests that cover and guarantee that ability, so if we want to preserve it we should add one.

There may be another way to solve #3566 where we just don't call host.requestUpdate() synchronously. Let me check on that.

Make host.requestUpdate() call async to avoid warnings
@justinfagnani justinfagnani changed the title Make Tasks run on hostUpdate() instead of hostUpdated() Fix change-in-update warning from Tasks Feb 10, 2023
@justinfagnani
Copy link
Collaborator Author

@e111077 I fixed this by putting host.requestUpdate() in a microtask and added a test that Tasks can see side effects of update() to ensure that behavior.

Copy link
Member

@e111077 e111077 left a comment

Choose a reason for hiding this comment

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

Wow great job!

.changeset/fast-apes-cross.md Outdated Show resolved Hide resolved
@justinfagnani justinfagnani merged commit 65df149 into main Feb 10, 2023
@justinfagnani justinfagnani deleted the task-update-timing branch February 10, 2023 07:00
@lit-robot lit-robot mentioned this pull request Mar 10, 2023
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.

[labs/task] Task improperly warns user about rendering
2 participants