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

fix(core): support popover boolean attribute #4393

Merged
merged 3 commits into from
May 22, 2024
Merged

fix(core): support popover boolean attribute #4393

merged 3 commits into from
May 22, 2024

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 GitHub Actions
Copy link

github-actions bot commented May 21, 2024 •
edited

📊 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

@github-actions GitHub Actions
Copy link

github-actions bot commented May 21, 2024 •
edited

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 •
edited

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 •
edited

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.

rschristian
rschristian reviewed May 22, 2024
View reviewed changes
src/diff/props.js Outdated Show resolved Hide resolved
JoviDeCroock and others added 2 commits May 22, 2024 07:43
@JoviDeCroock @rschristian
Update render.test.js
df195e2 
Co-authored-by: Ryan Christian <33403762+rschristian@users.noreply.github.com>
@JoviDeCroock @rschristian
Update props.js
e16bb25 
Co-authored-by: Ryan Christian <33403762+rschristian@users.noreply.github.com>
@JoviDeCroock JoviDeCroock merged commit b7c43d9 into main May 22, 2024
13 checks passed
@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rschristian rschristian rschristian approved these changes

@marvinhagemeister marvinhagemeister marvinhagemeister 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.

Boolean values not handled as expected for popover prop
5 participants
@JoviDeCroock @coveralls @rschristian @robertknight @marvinhagemeister