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.

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

Preserve event handler return values #3812

Merged
merged 2 commits into from
Dec 4, 2022
Merged

Preserve event handler return values #3812

merged 2 commits into from
Dec 4, 2022

Conversation

developit
Copy link
Member

I'm not aware of any bugs caused by this, but currently we drop the return value of event handlers rather than returning it to the DOM. This corrects that.

@developit
Preserve event handler return values
05b024f 
I'm not aware of any bugs caused by this, but currently we drop the return value of event handlers rather than returning it to the DOM. This corrects that.
@github-actions
Copy link

github-actions bot commented Nov 22, 2022 •
edited

📊 Tachometer Benchmark Results

Summary

duration

  • 02_replace1k: unsure 🔍 -9% - +6% (-16.30ms - +11.03ms)
    preact-local vs preact-master
  • 03_update10th1k_x16: unsure 🔍 -2% - +5% (-0.80ms - +2.14ms)
    preact-local vs preact-master
  • 07_create10k: unsure 🔍 -2% - +1% (-36.28ms - +14.71ms)
    preact-local vs preact-master
  • filter_list: unsure 🔍 -2% - +2% (-0.76ms - +0.59ms)
    preact-local vs preact-master
  • hydrate1k: unsure 🔍 -1% - +1% (-1.14ms - +1.51ms)
    preact-local vs preact-master
  • many_updates: unsure 🔍 -1% - +5% (-0.37ms - +1.62ms)
    preact-local vs preact-master
  • text_update: faster ✔ 1% - 4% (0.02ms - 0.11ms)
    preact-local vs preact-master
  • todo: unsure 🔍 -1% - +1% (-0.59ms - +0.49ms)
    preact-local vs preact-master

usedJSHeapSize

  • 02_replace1k: unsure 🔍 -6% - +7% (-0.24ms - +0.25ms)
    preact-local vs preact-master
  • 03_update10th1k_x16: unsure 🔍 +0% - +0% (+0.00ms - +0.01ms)
    preact-local vs preact-master
  • 07_create10k: unsure 🔍 -0% - +0% (-0.03ms - +0.02ms)
    preact-local vs preact-master
  • filter_list: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
    preact-local vs preact-master
  • hydrate1k: unsure 🔍 -6% - +2% (-0.90ms - +0.27ms)
    preact-local vs preact-master
  • many_updates: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
    preact-local vs preact-master
  • text_update: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
    preact-local vs preact-master
  • todo: unsure 🔍 -0% - -0% (-0.00ms - -0.00ms)
    preact-local vs preact-master

Results

02_replace1k

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master168.54ms - 186.85ms-unsure 🔍
-6% - +9%
-11.03ms - +16.30ms		unsure 🔍
-4% - +12%
-6.78ms - +19.99ms
preact-local164.91ms - 185.21msunsure 🔍
-9% - +6%
-16.30ms - +11.03ms		-unsure 🔍
-6% - +11%
-10.11ms - +18.05ms
preact-hooks161.32ms - 180.85msunsure 🔍
-11% - +4%
-19.99ms - +6.78ms		unsure 🔍
-10% - +6%
-18.05ms - +10.11ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master3.67ms - 4.02ms-unsure 🔍
-7% - +6%
-0.25ms - +0.24ms		unsure 🔍
-10% - +4%
-0.42ms - +0.16ms
preact-local3.68ms - 4.03msunsure 🔍
-6% - +7%
-0.24ms - +0.25ms		-unsure 🔍
-10% - +4%
-0.41ms - +0.17ms
preact-hooks3.75ms - 4.20msunsure 🔍
-4% - +11%
-0.16ms - +0.42ms		unsure 🔍
-4% - +11%
-0.17ms - +0.41ms		-

run-warmup-0

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master1.24ms - 1.35ms-unsure 🔍
-5% - +6%
-0.06ms - +0.08ms		unsure 🔍
-6% - +5%
-0.08ms - +0.06ms
preact-local1.24ms - 1.33msunsure 🔍
-6% - +5%
-0.08ms - +0.06ms		-unsure 🔍
-6% - +4%
-0.08ms - +0.05ms
preact-hooks1.26ms - 1.35msunsure 🔍
-5% - +6%
-0.06ms - +0.08ms		unsure 🔍
-4% - +7%
-0.05ms - +0.08ms		-

run-warmup-1

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master0.89ms - 1.21ms-unsure 🔍
-23% - +25%
-0.24ms - +0.25ms		unsure 🔍
-41% - +1%
-0.58ms - +0.06ms
preact-local0.85ms - 1.22msunsure 🔍
-24% - +23%
-0.25ms - +0.24ms		-unsure 🔍
-43% - +1%
-0.60ms - +0.06ms
preact-hooks1.03ms - 1.58msunsure 🔍
-8% - +57%
-0.06ms - +0.58ms		unsure 🔍
-9% - +61%
-0.06ms - +0.60ms		-

run-warmup-2

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master0.51ms - 0.67ms-unsure 🔍
-22% - +14%
-0.14ms - +0.09ms		unsure 🔍
-8% - +32%
-0.04ms - +0.17ms
preact-local0.54ms - 0.70msunsure 🔍
-15% - +24%
-0.09ms - +0.14ms		-unsure 🔍
-4% - +38%
-0.02ms - +0.19ms
preact-hooks0.47ms - 0.59msunsure 🔍
-27% - +6%
-0.17ms - +0.04ms		unsure 🔍
-30% - +1%
-0.19ms - +0.02ms		-

run-warmup-3

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master0.58ms - 1.27ms-unsure 🔍
-32% - +92%
-0.19ms - +0.62ms		unsure 🔍
-56% - +34%
-0.61ms - +0.39ms
preact-local0.50ms - 0.93msunsure 🔍
-60% - +14%
-0.62ms - +0.19ms		-unsure 🔍
-63% - +1%
-0.75ms - +0.10ms
preact-hooks0.67ms - 1.40msunsure 🔍
-45% - +69%
-0.39ms - +0.61ms		unsure 🔍
-22% - +113%
-0.10ms - +0.75ms		-

run-warmup-4

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master0.32ms - 0.46ms-unsure 🔍
-20% - +28%
-0.07ms - +0.10ms		unsure 🔍
-27% - +17%
-0.11ms - +0.07ms
preact-local0.33ms - 0.43msunsure 🔍
-26% - +19%
-0.10ms - +0.07ms		-unsure 🔍
-26% - +10%
-0.11ms - +0.04ms
preact-hooks0.35ms - 0.47msunsure 🔍
-19% - +29%
-0.07ms - +0.11ms		unsure 🔍
-12% - +30%
-0.04ms - +0.11ms		-

run-final

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master0.37ms - 0.48ms-unsure 🔍
-20% - +17%
-0.08ms - +0.07ms		unsure 🔍
-21% - +12%
-0.10ms - +0.06ms
preact-local0.37ms - 0.49msunsure 🔍
-17% - +20%
-0.07ms - +0.08ms		-unsure 🔍
-22% - +15%
-0.10ms - +0.07ms
preact-hooks0.39ms - 0.50msunsure 🔍
-14% - +23%
-0.06ms - +0.10ms		unsure 🔍
-16% - +23%
-0.07ms - +0.10ms		-
03_update10th1k_x16

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master42.72ms - 44.67ms-unsure 🔍
-5% - +2%
-2.14ms - +0.80ms		unsure 🔍
-4% - +2%
-1.85ms - +1.05ms
preact-local43.27ms - 45.47msunsure 🔍
-2% - +5%
-0.80ms - +2.14ms		-unsure 🔍
-3% - +4%
-1.27ms - +1.81ms
preact-hooks43.03ms - 45.17msunsure 🔍
-2% - +4%
-1.05ms - +1.85ms		unsure 🔍
-4% - +3%
-1.81ms - +1.27ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master3.45ms - 3.46ms-unsure 🔍
-0% - -0%
-0.01ms - -0.00ms		faster ✔
1% - 1%
0.02ms - 0.03ms
preact-local3.46ms - 3.46msunsure 🔍
+0% - +0%
+0.00ms - +0.01ms		-faster ✔
0% - 1%
0.01ms - 0.02ms
preact-hooks3.47ms - 3.48msslower ❌
1% - 1%
0.02ms - 0.03ms		slower ❌
0% - 1%
0.01ms - 0.02ms		-
07_create10k

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master1798.69ms - 1839.80ms-unsure 🔍
-1% - +2%
-14.71ms - +36.28ms		unsure 🔍
-2% - +1%
-30.59ms - +23.98ms
preact-local1793.37ms - 1823.54msunsure 🔍
-2% - +1%
-36.28ms - +14.71ms		-unsure 🔍
-2% - +1%
-37.54ms - +9.36ms
preact-hooks1804.60ms - 1840.49msunsure 🔍
-1% - +2%
-23.98ms - +30.59ms		unsure 🔍
-1% - +2%
-9.36ms - +37.54ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master25.49ms - 25.52ms-unsure 🔍
-0% - +0%
-0.02ms - +0.03ms		unsure 🔍
-0% - -0%
-0.04ms - -0.00ms
preact-local25.48ms - 25.51msunsure 🔍
-0% - +0%
-0.03ms - +0.02ms		-unsure 🔍
-0% - -0%
-0.05ms - -0.01ms
preact-hooks25.51ms - 25.54msunsure 🔍
+0% - +0%
+0.00ms - +0.04ms		unsure 🔍
+0% - +0%
+0.01ms - +0.05ms		-
filter_list

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master33.09ms - 34.07ms-unsure 🔍
-2% - +2%
-0.59ms - +0.76ms		unsure 🔍
-2% - +2%
-0.69ms - +0.83ms
preact-local33.03ms - 33.96msunsure 🔍
-2% - +2%
-0.76ms - +0.59ms		-unsure 🔍
-2% - +2%
-0.76ms - +0.72ms
preact-hooks32.94ms - 34.09msunsure 🔍
-2% - +2%
-0.83ms - +0.69ms		unsure 🔍
-2% - +2%
-0.72ms - +0.76ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master1.61ms - 1.61ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms		faster ✔
1% - 1%
0.02ms - 0.02ms
preact-local1.61ms - 1.61msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms		-faster ✔
1% - 1%
0.02ms - 0.02ms
preact-hooks1.63ms - 1.63msslower ❌
1% - 1%
0.02ms - 0.02ms		slower ❌
1% - 1%
0.02ms - 0.02ms		-
hydrate1k

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master127.44ms - 129.41ms-unsure 🔍
-1% - +1%
-1.51ms - +1.14ms		unsure 🔍
-1% - +1%
-1.39ms - +1.14ms
preact-local127.72ms - 129.50msunsure 🔍
-1% - +1%
-1.14ms - +1.51ms		-unsure 🔍
-1% - +1%
-1.13ms - +1.25ms
preact-hooks127.76ms - 129.34msunsure 🔍
-1% - +1%
-1.14ms - +1.39ms		unsure 🔍
-1% - +1%
-1.25ms - +1.13ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master14.99ms - 15.82ms-unsure 🔍
-2% - +6%
-0.27ms - +0.90ms		unsure 🔍
-3% - +5%
-0.44ms - +0.77ms
preact-local14.69ms - 15.50msunsure 🔍
-6% - +2%
-0.90ms - +0.27ms		-unsure 🔍
-5% - +3%
-0.75ms - +0.45ms
preact-hooks14.80ms - 15.68msunsure 🔍
-5% - +3%
-0.77ms - +0.44ms		unsure 🔍
-3% - +5%
-0.45ms - +0.75ms		-
many_updates

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master32.55ms - 33.99ms-unsure 🔍
-5% - +1%
-1.62ms - +0.37ms		unsure 🔍
-4% - +2%
-1.38ms - +0.60ms
preact-local33.20ms - 34.59msunsure 🔍
-1% - +5%
-0.37ms - +1.62ms		-unsure 🔍
-2% - +4%
-0.74ms - +1.21ms
preact-hooks32.98ms - 34.34msunsure 🔍
-2% - +4%
-0.60ms - +1.38ms		unsure 🔍
-4% - +2%
-1.21ms - +0.74ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master4.64ms - 4.65ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms		unsure 🔍
-0% - -0%
-0.02ms - -0.02ms
preact-local4.64ms - 4.65msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms		-unsure 🔍
-0% - -0%
-0.02ms - -0.02ms
preact-hooks4.67ms - 4.67msunsure 🔍
+0% - +0%
+0.02ms - +0.02ms		unsure 🔍
+0% - +0%
+0.02ms - +0.02ms		-
text_update

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master2.95ms - 3.02ms-slower ❌
1% - 4%
0.02ms - 0.11ms		faster ✔
1% - 4%
0.03ms - 0.12ms
preact-local2.90ms - 2.94msfaster ✔
1% - 4%
0.02ms - 0.11ms		-faster ✔
4% - 6%
0.11ms - 0.18ms
preact-hooks3.03ms - 3.09msslower ❌
1% - 4%
0.03ms - 0.12ms		slower ❌
4% - 6%
0.11ms - 0.18ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master0.82ms - 0.82ms-unsure 🔍
-0% - -0%
-0.00ms - -0.00ms		faster ✔
2% - 2%
0.01ms - 0.01ms
preact-local0.82ms - 0.82msunsure 🔍
+0% - +0%
+0.00ms - +0.00ms		-faster ✔
2% - 2%
0.01ms - 0.01ms
preact-hooks0.84ms - 0.84msslower ❌
2% - 2%
0.01ms - 0.01ms		slower ❌
2% - 2%
0.01ms - 0.01ms		-
todo

duration

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master60.19ms - 60.86ms-unsure 🔍
-1% - +1%
-0.49ms - +0.59ms		faster ✔
1% - 3%
0.74ms - 1.82ms
preact-local60.06ms - 60.89msunsure 🔍
-1% - +1%
-0.59ms - +0.49ms		-faster ✔
1% - 3%
0.73ms - 1.92ms
preact-hooks61.38ms - 62.22msslower ❌
1% - 3%
0.74ms - 1.82ms		slower ❌
1% - 3%
0.73ms - 1.92ms		-

usedJSHeapSize

VersionAvg timevs preact-mastervs preact-localvs preact-hooks
preact-master1.11ms - 1.11ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms		faster ✔
2% - 2%
0.03ms - 0.03ms
preact-local1.10ms - 1.11msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms		-faster ✔
2% - 3%
0.03ms - 0.03ms
preact-hooks1.13ms - 1.13msslower ❌
2% - 3%
0.03ms - 0.03ms		slower ❌
2% - 3%
0.03ms - 0.03ms		-

tachometer-reporter-action v2 for Benchmarks

@coveralls
Copy link

coveralls commented Nov 22, 2022 •
edited

Coverage Status

Coverage remained the same at 99.531% when pulling 9336cb1 on event-handler-return-values into 915fd30 on master.

@github-actions
Copy link

github-actions bot commented Nov 22, 2022 •
edited

Size Change: +5 B (0%)

Total Size: 52.9 kB

Filename Size Change
dist/preact.js 4.05 kB +1 B
dist/preact.min.js 4.08 kB +3 B (0%)
dist/preact.min.module.js 4.08 kB +3 B (0%)
dist/preact.min.umd.js 4.1 kB -1 B
dist/preact.module.js 4.07 kB +1 B
dist/preact.umd.js 4.12 kB -2 B (0%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3.81 kB 0 B
compat/dist/compat.module.js 3.75 kB 0 B
compat/dist/compat.umd.js 3.87 kB 0 B
debug/dist/debug.js 3 kB 0 B
debug/dist/debug.module.js 3.01 kB 0 B
debug/dist/debug.umd.js 3.08 kB 0 B
devtools/dist/devtools.js 231 B 0 B
devtools/dist/devtools.module.js 240 B 0 B
devtools/dist/devtools.umd.js 315 B 0 B
hooks/dist/hooks.js 1.49 kB 0 B
hooks/dist/hooks.module.js 1.52 kB 0 B
hooks/dist/hooks.umd.js 1.57 kB 0 B
jsx-runtime/dist/jsxRuntime.js 358 B 0 B
jsx-runtime/dist/jsxRuntime.module.js 324 B 0 B
jsx-runtime/dist/jsxRuntime.umd.js 439 B 0 B
test-utils/dist/testUtils.js 442 B 0 B
test-utils/dist/testUtils.module.js 444 B 0 B
test-utils/dist/testUtils.umd.js 526 B 0 B

compressed-size-action

@andrewiggins andrewiggins merged commit 226d77d into master Dec 4, 2022
@andrewiggins andrewiggins deleted the event-handler-return-values branch December 4, 2022 19:22
andrewiggins added a commit that referenced this pull request Dec 4, 2022
@developit @andrewiggins
Preserve event handler return values (#3812)
eccd0f9 
I'm not aware of any bugs caused by this, but currently we drop the return value of event handlers rather than returning it to the DOM. This corrects that.

Co-authored-by: Andre Wiggins <[email protected]>
@andrewiggins
Copy link
Member

Perhaps this change is still good to do this but I just realized that in modern browsers the return value is ignored: MDN docs

andrewiggins added a commit that referenced this pull request Dec 5, 2022
@andrewiggins
Merge pull request #3819 from preactjs/return-event-handlers
8096aa9 
Preserve event handler return values (#3812)
developit added a commit to Shopify/remote-dom that referenced this pull request Jun 22, 2023
@developit
store event handler return values in an internal property and pass th…
32d19b1 
…em back to the host (demo waiting on preactjs/preact#3812)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewiggins andrewiggins andrewiggins approved these changes

@marvinhagemeister marvinhagemeister marvinhagemeister approved these changes

@JoviDeCroock JoviDeCroock JoviDeCroock approved these changes

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@developit @coveralls @andrewiggins @marvinhagemeister @JoviDeCroock