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.

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

feat: Add support for prop: & attr: directives #4419

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

rschristian
Copy link
Member

First-pass impl of #4416

Copy link

github-actions bot commented Jun 22, 2024

📊 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

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

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

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

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

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

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

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

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

Copy link

github-actions bot commented Jun 22, 2024

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

Co-authored-by: Marvin Hagemeister <hello@marvinh.dev>
@preactjs preactjs deleted a comment from coveralls Jun 25, 2024
@preactjs preactjs deleted a comment from coveralls Jun 25, 2024
@preactjs preactjs deleted a comment from coveralls Jun 25, 2024
@preactjs preactjs deleted a comment from coveralls Jun 25, 2024
@preactjs preactjs deleted a comment from coveralls Jun 25, 2024
@preactjs preactjs deleted a comment from coveralls Jun 25, 2024
@preactjs preactjs deleted a comment from coveralls Jun 25, 2024
@preactjs preactjs deleted a comment from coveralls Jun 25, 2024
@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@rschristian
Copy link
Member Author

rschristian commented Jun 30, 2024

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 rschristian marked this pull request as ready for review June 30, 2024 20:36
@preactjs preactjs deleted a comment from coveralls Jun 30, 2024
@preactjs preactjs deleted a comment from coveralls Jun 30, 2024
@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

src/diff/props.js Outdated Show resolved Hide resolved
Co-authored-by: Jovi De Croock <decroockjovi@gmail.com>
@coveralls
Copy link

coveralls commented Jul 16, 2024

Coverage Status

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

@developit
Copy link
Member

developit commented Aug 23, 2024

@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').

@rschristian
Copy link
Member Author

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.


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.

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.

syntax for choosing setting a property vs attribute on elements
6 participants