preact icon indicating copy to clipboard operation
preact copied to clipboard
verified publisher icon preactjs

feat: Add support for `prop:` & `attr:` directives

Open rschristian opened this issue 1 year ago • 14 comments

First-pass impl of #4416

rschristian avatar Jun 22 '24 14:06 rschristian

📊 Tachometer Benchmark Results

Summary

duration

  • create10k: unsure 🔍 -1% - +3% (-5.73ms - +22.85ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -0% - +1% (-0.05ms - +0.12ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -2% - +4% (-1.08ms - +3.11ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -3% - +1% (-0.43ms - +0.25ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -0% - +4% (-0.18ms - +3.25ms)
    preact-local vs preact-main
  • text-update: unsure 🔍 -5% - +3% (-0.10ms - +0.06ms)
    preact-local vs preact-main
  • todo: unsure 🔍 -1% - +1% (-0.28ms - +0.30ms)
    preact-local vs preact-main
  • update10th1k: unsure 🔍 -3% - +3% (-0.88ms - +0.96ms)
    preact-local vs preact-main

usedJSHeapSize

  • create10k: unsure 🔍 +0% - +0% (+0.00ms - +0.00ms)
    preact-local vs preact-main
  • filter-list: unsure 🔍 -0% - +0% (-0.00ms - +0.00ms)
    preact-local vs preact-main
  • hydrate1k: unsure 🔍 -1% - +1% (-0.13ms - +0.14ms)
    preact-local vs preact-main
  • many-updates: unsure 🔍 -0% - +0% (-0.00ms - +0.01ms)
    preact-local vs preact-main
  • replace1k: unsure 🔍 -1% - +1% (-0.03ms - +0.02ms)
    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
  • Browser: chrome-headless
  • Sample size: 50
  • Built by: Benchmarks #1594

  • Commit: a9bb51f

duration

VersionAvg timevs preact-localvs preact-main
preact-local863.04ms - 889.28ms-unsure 🔍
-1% - +3%
-5.73ms - +22.85ms
preact-main861.94ms - 873.26msunsure 🔍
-3% - +1%
-22.85ms - +5.73ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local25.16ms - 25.16ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms
preact-main25.15ms - 25.15msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms		-
filter-list
  • Browser: chrome-headless
  • Sample size: 50
  • Built by: Benchmarks #1594

  • Commit: a9bb51f

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.66ms - 16.77ms-unsure 🔍
-0% - +1%
-0.05ms - +0.12ms
preact-main16.61ms - 16.74msunsure 🔍
-1% - +0%
-0.12ms - +0.05ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.69ms - 1.69ms-unsure 🔍
-0% - +0%
-0.00ms - +0.00ms
preact-main1.69ms - 1.70msunsure 🔍
-0% - +0%
-0.00ms - +0.00ms		-
hydrate1k
  • Browser: chrome-headless
  • Sample size: 50
  • Built by: Benchmarks #1594

  • Commit: a9bb51f

duration

VersionAvg timevs preact-localvs preact-main
preact-local68.98ms - 72.76ms-unsure 🔍
-2% - +4%
-1.08ms - +3.11ms
preact-main68.95ms - 70.75msunsure 🔍
-4% - +1%
-3.11ms - +1.08ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local15.55ms - 15.55ms-unsure 🔍
-1% - +1%
-0.13ms - +0.14ms
preact-main15.41ms - 15.68msunsure 🔍
-1% - +1%
-0.14ms - +0.13ms		-
many-updates
  • Browser: chrome-headless
  • Sample size: 50
  • Built by: Benchmarks #1594

  • Commit: a9bb51f

duration

VersionAvg timevs preact-localvs preact-main
preact-local16.41ms - 16.97ms-unsure 🔍
-3% - +1%
-0.43ms - +0.25ms
preact-main16.59ms - 16.97msunsure 🔍
-1% - +3%
-0.25ms - +0.43ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local4.61ms - 4.62ms-unsure 🔍
-0% - +0%
-0.00ms - +0.01ms
preact-main4.61ms - 4.62msunsure 🔍
-0% - +0%
-0.01ms - +0.00ms		-
replace1k
  • Browser: chrome-headless
  • Sample size: 100
  • Built by: Benchmarks #1594

  • Commit: a9bb51f

duration

VersionAvg timevs preact-localvs preact-main
preact-local73.46ms - 75.79ms-unsure 🔍
-0% - +4%
-0.18ms - +3.25ms
preact-main71.84ms - 74.35msunsure 🔍
-4% - +0%
-3.25ms - +0.18ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.52ms - 3.55ms-unsure 🔍
-1% - +1%
-0.03ms - +0.02ms
preact-main3.52ms - 3.55msunsure 🔍
-1% - +1%
-0.02ms - +0.03ms		-

run-warmup-0

VersionAvg timevs preact-localvs preact-main
preact-local29.50ms - 29.90ms-slower ❌
2% - 4%
0.44ms - 1.06ms
preact-main28.71ms - 29.18msfaster ✔
1% - 4%
0.44ms - 1.06ms		-

run-warmup-1

VersionAvg timevs preact-localvs preact-main
preact-local33.88ms - 35.73ms-unsure 🔍
-2% - +6%
-0.55ms - +2.07ms
preact-main33.12ms - 34.96msunsure 🔍
-6% - +2%
-2.07ms - +0.55ms		-

run-warmup-2

VersionAvg timevs preact-localvs preact-main
preact-local24.97ms - 25.40ms-unsure 🔍
-1% - +1%
-0.38ms - +0.33ms
preact-main24.93ms - 25.49msunsure 🔍
-1% - +1%
-0.33ms - +0.38ms		-

run-warmup-3

VersionAvg timevs preact-localvs preact-main
preact-local30.10ms - 31.53ms-unsure 🔍
-0% - +7%
-0.07ms - +2.07ms
preact-main29.01ms - 30.62msunsure 🔍
-7% - +0%
-2.07ms - +0.07ms		-

run-warmup-4

VersionAvg timevs preact-localvs preact-main
preact-local22.06ms - 23.51ms-unsure 🔍
-9% - +1%
-2.09ms - +0.20ms
preact-main22.84ms - 24.62msunsure 🔍
-1% - +9%
-0.20ms - +2.09ms		-

run-final

VersionAvg timevs preact-localvs preact-main
preact-local22.88ms - 23.89ms-unsure 🔍
-2% - +4%
-0.46ms - +0.96ms
preact-main22.64ms - 23.63msunsure 🔍
-4% - +2%
-0.96ms - +0.46ms		-
text-update
  • Browser: chrome-headless
  • Sample size: 190
  • Built by: Benchmarks #1594

  • Commit: a9bb51f

duration

VersionAvg timevs preact-localvs preact-main
preact-local1.97ms - 2.07ms-unsure 🔍
-5% - +3%
-0.10ms - +0.06ms
preact-main1.98ms - 2.10msunsure 🔍
-3% - +5%
-0.06ms - +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
  • Browser: chrome-headless
  • Sample size: 50
  • Built by: Benchmarks #1594

  • Commit: a9bb51f

duration

VersionAvg timevs preact-localvs preact-main
preact-local26.73ms - 27.02ms-unsure 🔍
-1% - +1%
-0.28ms - +0.30ms
preact-main26.61ms - 27.11msunsure 🔍
-1% - +1%
-0.30ms - +0.28ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local1.25ms - 1.25ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms
preact-main1.25ms - 1.25msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms		-
update10th1k
  • Browser: chrome-headless
  • Sample size: 50
  • Built by: Benchmarks #1594

  • Commit: a9bb51f

duration

VersionAvg timevs preact-localvs preact-main
preact-local31.07ms - 31.92ms-unsure 🔍
-3% - +3%
-0.88ms - +0.96ms
preact-main30.64ms - 32.27msunsure 🔍
-3% - +3%
-0.96ms - +0.88ms		-

usedJSHeapSize

VersionAvg timevs preact-localvs preact-main
preact-local3.56ms - 3.56ms-unsure 🔍
+0% - +0%
+0.00ms - +0.00ms
preact-main3.55ms - 3.55msunsure 🔍
-0% - -0%
-0.00ms - -0.00ms		-

tachometer-reporter-action v2 for Benchmarks

github-actions[bot] avatar Jun 22 '24 14:06 github-actions[bot]

Size Change: +255 B (+0.41%)

Total Size: 62.2 kB

Filename Size Change
dist/preact.js 4.75 kB +43 B (+0.91%)
dist/preact.min.js 4.78 kB +43 B (+0.91%)
dist/preact.min.module.js 4.78 kB +44 B (+0.93%)
dist/preact.min.umd.js 4.8 kB +43 B (+0.9%)
dist/preact.module.js 4.77 kB +43 B (+0.91%)
dist/preact.umd.js 4.82 kB +39 B (+0.82%)
ℹ️ View Unchanged
Filename Size
compat/dist/compat.js 4.1 kB
compat/dist/compat.module.js 4.03 kB
compat/dist/compat.umd.js 4.17 kB
debug/dist/debug.js 3.7 kB
debug/dist/debug.module.js 3.71 kB
debug/dist/debug.umd.js 3.78 kB
devtools/dist/devtools.js 259 B
devtools/dist/devtools.module.js 273 B
devtools/dist/devtools.umd.js 345 B
hooks/dist/hooks.js 1.53 kB
hooks/dist/hooks.module.js 1.56 kB
hooks/dist/hooks.umd.js 1.6 kB
jsx-runtime/dist/jsxRuntime.js 981 B
jsx-runtime/dist/jsxRuntime.module.js 956 B
jsx-runtime/dist/jsxRuntime.umd.js 1.06 kB
test-utils/dist/testUtils.js 451 B
test-utils/dist/testUtils.module.js 456 B
test-utils/dist/testUtils.umd.js 536 B

compressed-size-action

github-actions[bot] avatar Jun 22 '24 14:06 github-actions[bot]

Coverage Status

coverage: 99.611%. remained the same when pulling 17fee0233349e1798835806e76819aa3ce19e90e on feat/prop-attr-directives into 4c20c23c16dd60f380ce9fe98afc93041a7e1562 on main.

coveralls avatar Jun 25 '24 03:06 coveralls

Coverage Status

coverage: 99.611%. remained the same when pulling 17fee0233349e1798835806e76819aa3ce19e90e on feat/prop-attr-directives into 4c20c23c16dd60f380ce9fe98afc93041a7e1562 on main.

coveralls avatar Jun 25 '24 03:06 coveralls

I'll mark this as "ready", though I'm happy to still discuss whether we even want to do this and how it should interact with other things like RTS.

IMO, it is a nice bit of power to give users though it is a bit of a deviation and might confuse some.

Edit: What in the world is up with Coveralls

rschristian avatar Jun 30 '24 20:06 rschristian

Coverage Status

coverage: 99.611%. remained the same when pulling 806e605fa53f74abac84f2567e50c7f9ad73d25d on feat/prop-attr-directives into 16aeb9f77edaa5015bc7ffc9237149e1502e1e73 on main.

coveralls avatar Jun 30 '24 20:06 coveralls

Coverage Status

coverage: 99.611%. remained the same when pulling 806e605fa53f74abac84f2567e50c7f9ad73d25d on feat/prop-attr-directives into 16aeb9f77edaa5015bc7ffc9237149e1502e1e73 on main.

coveralls avatar Jun 30 '24 20:06 coveralls

Coverage Status

coverage: 99.612%. remained the same when pulling a9bb51fefa04b51ff2fa1f7ca08b095ebed4805d on feat/prop-attr-directives into f7f9d9bf4309d09bcd0a432acd72787b7d9baecb on main.

coveralls avatar Jul 16 '24 20:07 coveralls

@rschristian I like this direction, but I would love to avoid using namespaces for the prefixes. I believe vue uses something like <foo .value="bar"> for properties and <foo @value="bar"> for attributes. JSX doesn't support that though.

Interestingly, JSX does allow for two suffixes: $ and -. The - suffix makes a decent fit for attributes because - at the end of an identifier is invalid so it's an uncommon property name (obj['foo-']):

<div prop$="foo" />;
<div attr-="bar" />;

The other tidbit I wanted to add to this discussion is that we do technically already have a "force to use attribute" syntax, which is to uppercase the name (<div ATTR="bar" />). Unless a Custom Element happens to have an all-upper-case property (which would be extremely weird), it'll always hit setAttribute('NAME') which is case-insensitive and results in attributeChangedCallback('name').

developit avatar Aug 23 '24 19:08 developit

The other tidbit I wanted to add to this discussion is that we do technically already have a "force to use attribute" syntax, which is to uppercase the name (<div ATTR="bar" />).

I'm fully aware, but it's a hard thing to push users toward; as stable as it is, it's an obvious hack that most raise an eyebrow at when it's suggested. There's a few threads recently in particular w/ users uneasy after I've suggested using capitalization to get around it, hence, this PR.

Also doesn't cover the "set this is as a property" case at all.

It's a shame directives require some opt-in, I don't love how unclear $ & - as suffixes are comparatively. Not as easy to spot either.

rschristian avatar Aug 23 '24 20:08 rschristian

https://github.com/preactjs/preact/issues/3790 (boolean attributes) was closed in favor of https://github.com/preactjs/preact/issues/4416. Is there a plan for it?

lilnasy avatar Nov 26 '24 14:11 lilnasy

Not at this time. data- attributes, being user defined, I don't think can be safely treated as boolean attributes? It's impossible for the framework to know whether you expect to use it as an enumerated attribute or a boolean.

rschristian avatar Nov 26 '24 23:11 rschristian

What if there was a bool: directive?

lilnasy avatar Nov 29 '24 16:11 lilnasy

I don't think we want to get into every possibility; I'm not sure even explicit prop/attr directives will land as-is.

If you want boolean, probably going to be on you to ensure they're added/removed when necessary via refs.

rschristian avatar Nov 30 '24 00:11 rschristian