Skip to content

[reactive-element] Fix a bug in change detection with decorated standard private accessors#4999

Merged
justinfagnani merged 4 commits intomainfrom
private-changes
Sep 29, 2025
Merged

[reactive-element] Fix a bug in change detection with decorated standard private accessors#4999
justinfagnani merged 4 commits intomainfrom
private-changes

Conversation

@justinfagnani
Copy link
Collaborator

Fixes #4998

@justinfagnani justinfagnani requested a review from sorvell May 22, 2025 22:14
@changeset-bot
Copy link

changeset-bot bot commented May 22, 2025

🦋 Changeset detected

Latest commit: 014af72

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 Patch
lit-element 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
Copy link
Contributor

github-actions bot commented May 22, 2025

📊 Tachometer Benchmark Results

Summary

nop-update

  • this-change, tip-of-tree, previous-release: unsure 🔍 -7% - +8% (-0.82ms - +0.94ms)
    this-change vs tip-of-tree

render

  • this-change: 43.51ms - 57.38ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -7% - +3% (-1.53ms - +0.69ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: slower ❌ 0% - 3% (0.08ms - 1.16ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -17% - +41% (-7.69ms - +19.71ms)
    this-change vs tip-of-tree

update

  • this-change: 458.19ms - 465.44ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +6% (-1.54ms - +2.27ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +1% (-1.53ms - +0.90ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-5.94ms - +3.05ms)
    this-change vs tip-of-tree

update-reflect

  • this-change: 440.07ms - 442.52ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -0% - +1% (-1.07ms - +3.60ms)
    this-change vs tip-of-tree

Results

this-change

render

VersionAvg timevs
43.51ms - 57.38ms-

update

VersionAvg timevs
458.19ms - 465.44ms-

update-reflect

VersionAvg timevs
440.07ms - 442.52ms-
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
19.11ms - 20.82ms-unsure 🔍
-7% - +3%
-1.53ms - +0.69ms
unsure 🔍
-7% - +4%
-1.46ms - +0.74ms
tip-of-tree
tip-of-tree
19.68ms - 21.09msunsure 🔍
-4% - +8%
-0.69ms - +1.53ms
-unsure 🔍
-5% - +5%
-0.92ms - +1.05ms
previous-release
previous-release
19.64ms - 21.01msunsure 🔍
-4% - +7%
-0.74ms - +1.46ms
unsure 🔍
-5% - +5%
-1.05ms - +0.92ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
34.72ms - 37.56ms-unsure 🔍
-4% - +6%
-1.54ms - +2.27ms
unsure 🔍
-5% - +4%
-1.96ms - +1.63ms
tip-of-tree
tip-of-tree
34.51ms - 37.05msunsure 🔍
-6% - +4%
-2.27ms - +1.54ms
-unsure 🔍
-6% - +3%
-2.21ms - +1.15ms
previous-release
previous-release
35.21ms - 37.41msunsure 🔍
-5% - +5%
-1.63ms - +1.96ms
unsure 🔍
-3% - +6%
-1.15ms - +2.21ms
-

nop-update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
11.60ms - 12.77ms-unsure 🔍
-7% - +8%
-0.82ms - +0.94ms
unsure 🔍
-10% - +4%
-1.25ms - +0.47ms
tip-of-tree
tip-of-tree
11.47ms - 12.79msunsure 🔍
-8% - +7%
-0.94ms - +0.82ms
-unsure 🔍
-11% - +4%
-1.36ms - +0.47ms
previous-release
previous-release
11.95ms - 13.21msunsure 🔍
-4% - +10%
-0.47ms - +1.25ms
unsure 🔍
-4% - +11%
-0.47ms - +1.36ms
-
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
35.00ms - 35.81ms-slower ❌
0% - 3%
0.08ms - 1.16ms
unsure 🔍
-0% - +3%
-0.00ms - +1.04ms
tip-of-tree
tip-of-tree
34.43ms - 35.15msfaster ✔
0% - 3%
0.08ms - 1.16ms
-unsure 🔍
-2% - +1%
-0.59ms - +0.39ms
previous-release
previous-release
34.55ms - 35.22msunsure 🔍
-3% - +0%
-1.04ms - +0.00ms
unsure 🔍
-1% - +2%
-0.39ms - +0.59ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
69.36ms - 71.17ms-unsure 🔍
-2% - +1%
-1.53ms - +0.90ms
unsure 🔍
-3% - +0%
-2.34ms - +0.33ms
tip-of-tree
tip-of-tree
69.77ms - 71.39msunsure 🔍
-1% - +2%
-0.90ms - +1.53ms
-unsure 🔍
-3% - +1%
-1.97ms - +0.58ms
previous-release
previous-release
70.29ms - 72.25msunsure 🔍
-0% - +3%
-0.33ms - +2.34ms
unsure 🔍
-1% - +3%
-0.58ms - +1.97ms
-
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
46.40ms - 66.32ms-unsure 🔍
-17% - +41%
-7.69ms - +19.71ms
unsure 🔍
-24% - +27%
-13.07ms - +15.02ms
tip-of-tree
tip-of-tree
40.94ms - 59.75msunsure 🔍
-34% - +12%
-19.71ms - +7.69ms
-unsure 🔍
-33% - +14%
-18.69ms - +8.62ms
previous-release
previous-release
45.49ms - 65.28msunsure 🔍
-26% - +23%
-15.02ms - +13.07ms
unsure 🔍
-18% - +38%
-8.62ms - +18.69ms
-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
454.02ms - 460.57ms-unsure 🔍
-1% - +1%
-5.94ms - +3.05ms
unsure 🔍
-1% - +1%
-6.89ms - +3.24ms
tip-of-tree
tip-of-tree
455.66ms - 461.82msunsure 🔍
-1% - +1%
-3.05ms - +5.94ms
-unsure 🔍
-1% - +1%
-5.31ms - +4.56ms
previous-release
previous-release
455.26ms - 462.98msunsure 🔍
-1% - +2%
-3.24ms - +6.89ms
unsure 🔍
-1% - +1%
-4.56ms - +5.31ms
-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree
vs previous-release
previous-release
this-change
459.66ms - 462.90ms-unsure 🔍
-0% - +1%
-1.07ms - +3.60ms
unsure 🔍
-0% - +1%
-0.19ms - +4.00ms
tip-of-tree
tip-of-tree
458.34ms - 461.70msunsure 🔍
-1% - +0%
-3.60ms - +1.07ms
-unsure 🔍
-0% - +1%
-1.50ms - +2.79ms
previous-release
previous-release
458.04ms - 460.71msunsure 🔍
-1% - +0%
-4.00ms - +0.19ms
unsure 🔍
-1% - +0%
-2.79ms - +1.50ms
-

tachometer-reporter-action v2 for Benchmarks

@github-actions
Copy link
Contributor

github-actions bot commented May 22, 2025

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.

need size check update as per comment above but otherwise looks good to me. suggestions for minor fixes.

@justinfagnani justinfagnani merged commit 6c174c8 into main Sep 29, 2025
11 checks passed
@justinfagnani justinfagnani deleted the private-changes branch September 29, 2025 01:51
@lit-robot lit-robot mentioned this pull request Dec 18, 2025
kevinpschaaf added a commit to breadboard-ai/breadboard that referenced this pull request Jan 15, 2026
)

Fixes b/475312122

When the record button is clicked, the private `#recorder` field is set,
which is what changes the template from the "record" button to the
"stop" button. Since `#recorder` wasn't marked as `state()`, the
template wasn't re-rendered based on this assignment, and since clicking
the button immediately disables it, there was no way to stop recording.

Appears to have been a latent bug, exposed by
#7458 (found via
bisect). That PR resulted in `lit` being bumped from `3.3.1` to `3.3.2`,
which included a bugfix in the lit core library:
lit/lit#4999 "Fix a bug in change detection with
decorated standard private accessors".

The reason the fix in Lit broke Opal was because that in the same
microtask that `#recorder` was being set, the `#clearValues()` method
was being called which did set the `@state() #value` field to `null`.
Since `null` is the initial value, this should not have triggered a
re-render but did, because of the Lit bug above: Lit reading `newValue`
in `requestUpdate` incorrectly returned `undefined` for a private field,
hence setting to `null` always failed equality check and incorrectly
triggered an update (compensating for `#recorder` not being marked as
state). The fix to Lit means that nothing was triggering a re-render
after the record button was clicked. Thus ensuring that the `#recorder`
field was properly marked as `@state` is all that's needed.
kevinpschaaf added a commit to breadboard-ai/breadboard that referenced this pull request Jan 16, 2026
)

Fixes b/475312122

When the record button is clicked, the private `#recorder` field is set,
which is what changes the template from the "record" button to the
"stop" button. Since `#recorder` wasn't marked as `state()`, the
template wasn't re-rendered based on this assignment, and since clicking
the button immediately disables it, there was no way to stop recording.

Appears to have been a latent bug, exposed by
#7458 (found via
bisect). That PR resulted in `lit` being bumped from `3.3.1` to `3.3.2`,
which included a bugfix in the lit core library:
lit/lit#4999 "Fix a bug in change detection with
decorated standard private accessors".

The reason the fix in Lit broke Opal was because that in the same
microtask that `#recorder` was being set, the `#clearValues()` method
was being called which did set the `@state() #value` field to `null`.
Since `null` is the initial value, this should not have triggered a
re-render but did, because of the Lit bug above: Lit reading `newValue`
in `requestUpdate` incorrectly returned `undefined` for a private field,
hence setting to `null` always failed equality check and incorrectly
triggered an update (compensating for `#recorder` not being marked as
state). The fix to Lit means that nothing was triggering a re-render
after the record button was clicked. Thus ensuring that the `#recorder`
field was properly marked as `@state` is all that's needed.
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.

[reactive-element] Reactive properties don't handle private names properly

3 participants