Skip to content

Conversation

@JoviDeCroock
Copy link
Member

Fixes #4391

When doing tests it looks like setAttribute('popover', '') is the only one that results in proper behavior here. We should double check what we do in RTS for this as well

@github-actions
Copy link

github-actions bot commented May 21, 2024

📊 Tachometer Benchmark Results

Summary

duration

  • create10k: unsure 🔍 -1% - +1% (-12.27ms - +9.25ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -1% - +1% (-0.20ms - +0.15ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -3% - +1% (-2.26ms - +0.55ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -2% - +5% (-0.24ms - +0.75ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -1% - +3% (-0.35ms - +1.76ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -5% - +2% (-0.10ms - +0.04ms)
    preact-local vs preact-main
  • todo: unsure 🔍 -2% - +2% (-0.54ms - +0.69ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -3% - +2% (-0.80ms - +0.47ms)
    preact-local vs preact-main

usedJSHeapSize

  • create10k: unsure 🔍 -0% - +0% (-0.01ms - +0.01ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -3% - +2% (-0.50ms - +0.30ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -0% - +1% (-0.01ms - +0.04ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
    preact-local vs preact-main
  • todo: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
    preact-local vs preact-main

Results

create10k

duration

VersionAvg timevs preact-localvs preact-main
preact-local878.94ms - 894.17ms-unsure 🔍
-1% - +1%
-12.27ms - +9.25ms
preact-main880.46ms - 895.66msunsure 🔍
-1% - +1%
-9.25ms - +12.27ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local27.30ms - 27.31ms-unsure 🔍
-0% - +0%
-0.01ms - +0.01ms
preact-main27.30ms - 27.31msunsure 🔍
-0% - +0%
-0.01ms - +0.01ms
-
filter-list

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.56ms - 16.82ms-unsure 🔍
-1% - +1%
-0.20ms - +0.15ms
preact-main16.60ms - 16.83msunsure 🔍
-1% - +1%
-0.15ms - +0.20ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.76ms - 1.77ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms
preact-main1.76ms - 1.77msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms
-
hydrate1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local72.30ms - 72.91ms-unsure 🔍
-3% - +1%
-2.26ms - +0.55ms
preact-main72.09ms - 74.83msunsure 🔍
-1% - +3%
-0.55ms - +2.26ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local14.35ms - 14.89ms-unsure 🔍
-3% - +2%
-0.50ms - +0.30ms
preact-main14.42ms - 15.01msunsure 🔍
-2% - +3%
-0.30ms - +0.50ms
-
many-updates

duration

VersionAvg timevs preact-localvs preact-main
preact-local15.00ms - 15.86ms-unsure 🔍
-2% - +5%
-0.24ms - +0.75ms
preact-main14.92ms - 15.43msunsure 🔍
-5% - +2%
-0.75ms - +0.24ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local4.95ms - 4.95ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms
preact-main4.94ms - 4.95msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms
-
replace1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local69.86ms - 71.46ms-unsure 🔍
-1% - +3%
-0.35ms - +1.76ms
preact-main69.27ms - 70.64msunsure 🔍
-2% - +0%
-1.76ms - +0.35ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.78ms - 3.81ms-unsure 🔍
-0% - +1%
-0.01ms - +0.04ms
preact-main3.76ms - 3.80msunsure 🔍
-1% - +0%
-0.04ms - +0.01ms
-

run-warmup-0

VersionAvg timevs preact-localvs preact-main
preact-local29.36ms - 30.33ms-unsure 🔍
-0% - +4%
+0.00ms - +1.09ms
preact-main29.05ms - 29.55msfaster ✔
0% - 4%
0.00ms - 1.09ms
-

run-warmup-1

VersionAvg timevs preact-localvs preact-main
preact-local34.46ms - 36.28ms-unsure 🔍
-4% - +4%
-1.38ms - +1.25ms
preact-main34.49ms - 36.38msunsure 🔍
-4% - +4%
-1.25ms - +1.38ms
-

run-warmup-2

VersionAvg timevs preact-localvs preact-main
preact-local26.32ms - 26.85ms-unsure 🔍
-2% - +1%
-0.53ms - +0.19ms
preact-main26.52ms - 27.00msunsure 🔍
-1% - +2%
-0.19ms - +0.53ms
-

run-warmup-3

VersionAvg timevs preact-localvs preact-main
preact-local24.99ms - 25.82ms-unsure 🔍
-1% - +3%
-0.29ms - +0.86ms
preact-main24.72ms - 25.52msunsure 🔍
-3% - +1%
-0.86ms - +0.29ms
-

run-warmup-4

VersionAvg timevs preact-localvs preact-main
preact-local26.29ms - 27.57ms-faster ✔
0% - 7%
0.06ms - 1.92ms
preact-main27.24ms - 28.60msslower ❌
0% - 7%
0.06ms - 1.92ms
-

run-final

VersionAvg timevs preact-localvs preact-main
preact-local23.65ms - 24.72ms-unsure 🔍
-2% - +4%
-0.48ms - +0.93ms
preact-main23.50ms - 24.42msunsure 🔍
-4% - +2%
-0.93ms - +0.48ms
-
text-update

duration

VersionAvg timevs preact-localvs preact-main
preact-local2.03ms - 2.13ms-unsure 🔍
-5% - +2%
-0.10ms - +0.04ms
preact-main2.06ms - 2.16msunsure 🔍
-2% - +5%
-0.04ms - +0.10ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local0.98ms - 0.98ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms
preact-main0.98ms - 0.98msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms
-
todo

duration

VersionAvg timevs preact-localvs preact-main
preact-local29.30ms - 30.41ms-unsure 🔍
-2% - +2%
-0.54ms - +0.69ms
preact-main29.51ms - 30.06msunsure 🔍
-2% - +2%
-0.69ms - +0.54ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.25ms - 1.26ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms
preact-main1.25ms - 1.25msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms
-
update10th1k

duration

VersionAvg timevs preact-localvs preact-main
preact-local29.48ms - 30.24ms-unsure 🔍
-3% - +2%
-0.80ms - +0.47ms
preact-main29.52ms - 30.54msunsure 🔍
-2% - +3%
-0.47ms - +0.80ms
-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.74ms - 3.74ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms
preact-main3.74ms - 3.74msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms
-

tachometer-reporter-action v2 for Benchmarks

@JoviDeCroock JoviDeCroock force-pushed the support-boolean-attribute branch from 68e5cda to d315fc7 Compare May 21, 2024 08:42
@github-actions
Copy link

github-actions bot commented May 21, 2024

Size Change: +72 B (+0.12%)

Total Size: 61.5 kB

Filename Size Change
dist/preact.js 4.65 kB +12 B (+0.26%)
dist/preact.min.js 4.68 kB +12 B (+0.26%)
dist/preact.min.module.js 4.67 kB +12 B (+0.26%)
dist/preact.min.umd.js 4.71 kB +11 B (+0.23%)
dist/preact.module.js 4.67 kB +12 B (+0.26%)
dist/preact.umd.js 4.72 kB +13 B (+0.28%)
ℹ️ View Unchanged
Filename Size
compat/dist/compat.js 4.09 kB
compat/dist/compat.module.js 4.01 kB
compat/dist/compat.umd.js 4.14 kB
debug/dist/debug.js 3.71 kB
debug/dist/debug.module.js 3.71 kB
debug/dist/debug.umd.js 3.79 kB
devtools/dist/devtools.js 231 B
devtools/dist/devtools.module.js 240 B
devtools/dist/devtools.umd.js 314 B
hooks/dist/hooks.js 1.55 kB
hooks/dist/hooks.module.js 1.59 kB
hooks/dist/hooks.umd.js 1.63 kB
jsx-runtime/dist/jsxRuntime.js 976 B
jsx-runtime/dist/jsxRuntime.module.js 949 B
jsx-runtime/dist/jsxRuntime.umd.js 1.06 kB
test-utils/dist/testUtils.js 453 B
test-utils/dist/testUtils.module.js 454 B
test-utils/dist/testUtils.umd.js 536 B

compressed-size-action

@coveralls
Copy link

coveralls commented May 21, 2024

Coverage Status

coverage: 99.61%. remained the same
when pulling e16bb25 on support-boolean-attribute
into 85bed33 on main.

@rschristian
Copy link
Member

rschristian commented May 21, 2024

RTS looks correct, as it matches other boolean attributes for the most part.

Not sure if it's smaller/faster, but we can set the prop, but we'd need to coerce it to the right value first, as attribute reflection uses the provided prop value (whereas the DOM property will coerce the value to auto/manual internally itself -- shame attribute reflection doesn't use the result of this coercion) Oh nvm, all nullish values end up as empty strings in our props. Would need that to change.

JoviDeCroock and others added 2 commits May 22, 2024 07:43
Co-authored-by: Ryan Christian <[email protected]>
Co-authored-by: Ryan Christian <[email protected]>
@JoviDeCroock JoviDeCroock merged commit b7c43d9 into main May 22, 2024
@JoviDeCroock JoviDeCroock deleted the support-boolean-attribute branch May 22, 2024 05:58
@robertknight
Copy link
Member

Thanks for the quick turnaround @JoviDeCroock!

I wonder if there is a more general way to test whether attributes behave like this?

@JoviDeCroock JoviDeCroock mentioned this pull request Jun 30, 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.

Boolean values not handled as expected for popover prop

6 participants