Skip to content

Fix bailouts breaking select value setting #4738

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix bailouts breaking select value setting #4738

wants to merge 3 commits into from
+72 −6

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Mar 26, 2025 •
edited
Loading

Resolves #4737

The problem as seen in the test and reproduction is that we are doing this diff in two runs, the first runbeing the one where we change the select value, we descend one level due to a bail (memo) and we bubble back up to the setProperty call. This setProperty call relies on the selected option being mounted, which is not the case this will happen in the second run.

To counteract this edge case which is increasingly likely to happen with the bail we implement in signals we defer the property setting to happen post-diff.

@JoviDeCroock JoviDeCroock marked this pull request as draft March 26, 2025 16:05
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 26, 2025 •
edited
Loading

📊 Tachometer Benchmark Results

Summary

duration

  • create10k: slower ❌ 0% - 1% (2.25ms - 9.63ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -1% - +3% (-0.13ms - +0.56ms)
    preact-local vs preact-main
  • hydrate1k: slower ❌ 10% - 15% (7.49ms - 10.27ms)
    preact-local vs preact-main
  • many-updates: faster ✔ 7% - 15% (1.10ms - 2.40ms)
    preact-local vs preact-main
  • replace1k: slower ❌ 12% - 17% (8.76ms - 11.40ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -4% - +4% (-0.08ms - +0.09ms)
    preact-local vs preact-main
  • todo: slower ❌ 5% - 7% (1.85ms - 2.53ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -0% - +5% (-0.05ms - +1.68ms)
    preact-local vs preact-main

usedJSHeapSize

  • create10k: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -1% - +2% (-0.01ms - +0.04ms)
    preact-local vs preact-main
  • hydrate1k: slower ❌ 4% - 17% (0.27ms - 1.12ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -1% - +0% (-0.03ms - +0.01ms)
    preact-local vs preact-main
  • replace1k: slower ❌ 1% - 2% (0.03ms - 0.05ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -5% - +3% (-0.06ms - +0.03ms)
    preact-local vs preact-main
  • todo: unsure 🔍 -0% - +1% (-0.00ms - +0.01ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -0% - +0% (-0.00ms - +0.01ms)
    preact-local vs preact-main

Results

create10k

duration

VersionAvg timevs preact-localvs preact-main
preact-local1007.98ms - 1013.78ms-slower ❌
0% - 1%
2.25ms - 9.63ms
preact-main1002.66ms - 1007.22msfaster ✔
0% - 1%
2.25ms - 9.63ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local19.19ms - 19.19ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms
preact-main19.18ms - 19.18msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms		-
filter-list

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.83ms - 17.48ms-unsure 🔍
-1% - +3%
-0.13ms - +0.56ms
preact-main16.82ms - 17.06msunsure 🔍
-3% - +1%
-0.56ms - +0.13ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.55ms - 1.59ms-unsure 🔍
-1% - +2%
-0.01ms - +0.04ms
preact-main1.54ms - 1.57msunsure 🔍
-2% - +1%
-0.04ms - +0.01ms		-
hydrate1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local79.09ms - 81.21ms-slower ❌
10% - 15%
7.49ms - 10.27ms
preact-main70.37ms - 72.17msfaster ✔
9% - 13%
7.49ms - 10.27ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local7.21ms - 7.85ms-slower ❌
4% - 17%
0.27ms - 1.12ms
preact-main6.56ms - 7.12msfaster ✔
4% - 15%
0.27ms - 1.12ms		-
many-updates

duration

VersionAvg timevs preact-localvs preact-main
preact-local14.15ms - 15.44ms-faster ✔
7% - 15%
1.10ms - 2.40ms
preact-main16.46ms - 16.63msslower ❌
7% - 17%
1.10ms - 2.40ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.78ms - 3.80ms-unsure 🔍
-1% - +0%
-0.03ms - +0.01ms
preact-main3.78ms - 3.82msunsure 🔍
-0% - +1%
-0.01ms - +0.03ms		-
replace1k
  • Browser: chrome-headless
  • Sample size: 100
  • Built by: CI #4664

  • Commit: 9cba734

duration

VersionAvg timevs preact-localvs preact-main
preact-local78.87ms - 80.25ms-slower ❌
12% - 17%
8.76ms - 11.40ms
preact-main68.35ms - 70.60msfaster ✔
11% - 14%
8.76ms - 11.40ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.02ms - 3.04ms-slower ❌
1% - 2%
0.03ms - 0.05ms
preact-main2.99ms - 2.99msfaster ✔
1% - 2%
0.03ms - 0.05ms		-

run-warmup-0

VersionAvg timevs preact-localvs preact-main
preact-local26.03ms - 26.93ms-unsure 🔍
-2% - +3%
-0.44ms - +0.76ms
preact-main25.92ms - 26.72msunsure 🔍
-3% - +2%
-0.76ms - +0.44ms		-

run-warmup-1

VersionAvg timevs preact-localvs preact-main
preact-local33.55ms - 34.98ms-unsure 🔍
-1% - +5%
-0.36ms - +1.65ms
preact-main32.91ms - 34.32msunsure 🔍
-5% - +1%
-1.65ms - +0.36ms		-

run-warmup-2

VersionAvg timevs preact-localvs preact-main
preact-local32.70ms - 33.85ms-slower ❌
11% - 16%
3.14ms - 4.71ms
preact-main28.82ms - 29.87msfaster ✔
10% - 14%
3.14ms - 4.71ms		-

run-warmup-3

VersionAvg timevs preact-localvs preact-main
preact-local25.50ms - 27.03ms-slower ❌
0% - 7%
0.01ms - 1.86ms
preact-main24.81ms - 25.84msfaster ✔
0% - 7%
0.01ms - 1.86ms		-

run-warmup-4

VersionAvg timevs preact-localvs preact-main
preact-local21.46ms - 21.93ms-faster ✔
13% - 17%
3.14ms - 4.32ms
preact-main24.88ms - 25.96msslower ❌
14% - 20%
3.14ms - 4.32ms		-

run-final

VersionAvg timevs preact-localvs preact-main
preact-local22.30ms - 22.60ms-unsure 🔍
-0% - +5%
-0.01ms - +1.07ms
preact-main21.40ms - 22.44msunsure 🔍
-5% - +0%
-1.07ms - +0.01ms		-
text-update
  • Browser: chrome-headless
  • Sample size: 120
  • Built by: CI #4664

  • Commit: 9cba734

duration

VersionAvg timevs preact-localvs preact-main
preact-local2.04ms - 2.16ms-unsure 🔍
-4% - +4%
-0.08ms - +0.09ms
preact-main2.03ms - 2.15msunsure 🔍
-4% - +4%
-0.09ms - +0.08ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.16ms - 1.22ms-unsure 🔍
-5% - +3%
-0.06ms - +0.03ms
preact-main1.17ms - 1.24msunsure 🔍
-3% - +5%
-0.03ms - +0.06ms		-
todo
  • Browser: chrome-headless
  • Sample size: 100
  • Built by: CI #4664

  • Commit: 9cba734

duration

VersionAvg timevs preact-localvs preact-main
preact-local36.92ms - 37.35ms-slower ❌
5% - 7%
1.85ms - 2.53ms
preact-main34.68ms - 35.20msfaster ✔
5% - 7%
1.85ms - 2.53ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.25ms - 1.26ms-unsure 🔍
-0% - +1%
-0.00ms - +0.01ms
preact-main1.24ms - 1.25msunsure 🔍
-1% - +0%
-0.01ms - +0.00ms		-
update10th1k
  • Browser: chrome-headless
  • Sample size: 130
  • Built by: CI #4664

  • Commit: 9cba734

duration

VersionAvg timevs preact-localvs preact-main
preact-local33.84ms - 35.06ms-unsure 🔍
-0% - +5%
-0.05ms - +1.68ms
preact-main33.02ms - 34.24msunsure 🔍
-5% - +0%
-1.68ms - +0.05ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local2.96ms - 2.97ms-unsure 🔍
-0% - +0%
-0.00ms - +0.01ms
preact-main2.96ms - 2.97msunsure 🔍
-0% - +0%
-0.01ms - +0.00ms		-

tachometer-reporter-action v2 for CI

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 26, 2025 •
edited
Loading

Size Change: +255 B (+0.32%)

Total Size: 78.8 kB

Filename Size Change
dist/preact.js 4.79 kB +36 B (+0.76%)
dist/preact.min.js 4.81 kB +37 B (+0.78%)
dist/preact.min.module.js 4.81 kB +36 B (+0.75%)
dist/preact.min.umd.js 4.83 kB +37 B (+0.77%)
dist/preact.mjs 4.8 kB +36 B (+0.76%)
dist/preact.module.js 4.8 kB +36 B (+0.76%)
dist/preact.umd.js 4.84 kB +37 B (+0.77%)
ℹ️ View Unchanged
Filename Size
compat/dist/compat.js 4.12 kB
compat/dist/compat.mjs 4.05 kB
compat/dist/compat.module.js 4.05 kB
compat/dist/compat.umd.js 4.19 kB
debug/dist/debug.js 3.82 kB
debug/dist/debug.mjs 3.82 kB
debug/dist/debug.module.js 3.82 kB
debug/dist/debug.umd.js 3.9 kB
devtools/dist/devtools.js 260 B
devtools/dist/devtools.mjs 274 B
devtools/dist/devtools.module.js 274 B
devtools/dist/devtools.umd.js 346 B
hooks/dist/hooks.js 1.54 kB
hooks/dist/hooks.mjs 1.57 kB
hooks/dist/hooks.module.js 1.57 kB
hooks/dist/hooks.umd.js 1.61 kB
jsx-runtime/dist/jsxRuntime.js 978 B
jsx-runtime/dist/jsxRuntime.mjs 952 B
jsx-runtime/dist/jsxRuntime.module.js 952 B
jsx-runtime/dist/jsxRuntime.umd.js 1.05 kB
test-utils/dist/testUtils.js 473 B
test-utils/dist/testUtils.mjs 477 B
test-utils/dist/testUtils.module.js 477 B
test-utils/dist/testUtils.umd.js 555 B

compressed-size-action

JoviDeCroock
JoviDeCroock commented Mar 26, 2025
View reviewed changes
@coveralls
Copy link

coveralls commented Mar 26, 2025 •
edited
Loading

Coverage Status

coverage: 99.611% (+0.001%) from 99.61%
when pulling 9cba734 on fix-memo
into bb804cd on main.

@luisherranz luisherranz mentioned this pull request Mar 27, 2025
Open
1 task
Copilot
Copilot AI reviewed Apr 8, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/diff/index.js:590

  • [nitpick] Consider using strict equality (===) when comparing 'nodeType' to 'select' to prevent potential type coercion issues.
if (nodeType == 'select') {

@JoviDeCroock JoviDeCroock changed the title Fix memo breaking context update boundaries Fix bailouts breaking select value setting Apr 8, 2025
marvinhagemeister
marvinhagemeister approved these changes Apr 8, 2025
View reviewed changes
Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

LGTM, seems like a reasonable fix, though the defer is unfortunate.

@JoviDeCroock JoviDeCroock marked this pull request as ready for review April 8, 2025 09:36
rschristian
rschristian approved these changes Apr 8, 2025
View reviewed changes
Copy link
Member

@rschristian rschristian left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Reasonable fix but pretty rough...

I'm almost wondering if we can suggest that be handled in user land instead. It's not a super niche scenario but it should be pretty easy to handle in app code

@JoviDeCroock JoviDeCroock force-pushed the fix-memo branch 3 times, most recently from 419423e to 9cba734 Compare April 25, 2025 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@marvinhagemeister marvinhagemeister marvinhagemeister approved these changes

@rschristian rschristian rschristian approved these changes

@developit developit Awaiting requested review from developit

@andrewiggins andrewiggins Awaiting requested review from andrewiggins

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

select value update fails when set to a newly added option simultaneously
4 participants
@JoviDeCroock @coveralls @marvinhagemeister @rschristian