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

Prefer to set property when property & attribute are identical #319

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

Conversation

qeleb
Copy link

@qeleb qeleb commented Apr 25, 2024

Changes

  1. It saves 5+ bytes for every single attribute that's instead set as a property, so I've added a list of reflected attributes to the Properties list.
  2. Fix 1 typo in a comment

Bundle size win

Example code

render(
  () => (
    <>
      <a id={Math.random() > 0.5 ? 'linkA' : 'linkB'}>link</a>                   {/* id */}
      <button name={Math.random() > 0.5 ? 'name a' : 'name b'}>name btn</button> {/* name */}
      <link rel={Math.random() > 0.5 ? 'preload' : 'preconnect'}>link</link>     {/* rel */}
      <img src={logo} alt={Math.random() > 0.5 ? 'a img' : 'b img'} />           {/* src */}
      <input type={Math.random() > 0.5 ? 'text' : 'password'} />                 {/* type */}
    </>
  ),
  document.body
);

OLD output

(() => {
  return [
    ((c = G()), e(() => a(c, 'id', Math.random() > 0.5 ? 'linkA' : 'linkB')), c),
    ((r = k()), e(() => a(r, 'name', Math.random() > 0.5 ? 'name a' : 'name b')), r),
    ((o = z()), e(() => a(o, 'rel', Math.random() > 0.5 ? 'preload' : 'preconnect')), o),
    ((n = E()),
    a(n, 'src', 'data:image/svg+xml,somesvg'),
    e(() => a(n, 'alt', Math.random() > 0.5 ? 'a img' : 'b img')),
    n),
    ((t = B()), e(() => a(t, 'type', Math.random() > 0.5 ? 'text' : 'password')), t),
  ];
}, document.body);

NEW output (-0.1KB)

(() => {
  return [
    ((c = A()), e(() => (c.id = Math.random() > 0.5 ? 'linkA' : 'linkB')), c),
    ((r = G()), e(() => (r.name = Math.random() > 0.5 ? 'name a' : 'name b')), r),
    ((o = k()), e(() => (o.rel = Math.random() > 0.5 ? 'preload' : 'preconnect')), o),
    ((n = O()),
    (n.src = 'data:image/svg+xml,somesvg'),
    e(() => (n.alt = Math.random() > 0.5 ? 'a img' : 'b img')),
    n),
    ((t = E()), e(() => (t.type = Math.random() > 0.5 ? 'text' : 'password')), t),
  ];
  var t, n, l, o, r, c, f;
}, document.body);

Notes

  1. There are more reflected properties to be added in the future for further gains, although some properties like maxlength (string)/maxLength (number) will require slightly more effort to coerce types

@ryansolid
Copy link
Owner

Trying to think if there is a downside. When I started Solid I actually used all properties but we switched to attributes for consistency reasons. We still use properties for webcomponents. So trying to remember why we made this choice. I might have to do some digging to remind myself of why.

@qeleb
Copy link
Author

qeleb commented Apr 25, 2024

Trying to think if there is a downside. When I started Solid I actually used all properties but we switched to attributes for consistency reasons. We still use properties for webcomponents. So trying to remember why we made this choice. I might have to do some digging to remind myself of why.

Ah please do.
I’ve been writing
prop:src=, prop:alt=, & prop:href= for months at work and I was hoping this is something the compiler could take on specifically for the cases where the attribute & property have the exact same behavior.

We definitely cannot do this for all of them though— there are some tricky ones like checked where the attr:checked === prop:defaultChecked (but not prop:checked)

there is a similarly weird case for the value property.

To the best of my understanding, this optimization is safe for all IDL attribute reflection properties so I made a very careful list of only a few.

@qeleb
Copy link
Author

qeleb commented May 3, 2024

@ryansolid Have you gotten a chance to review this? I’ve been taking a look at how Preact handles this and it’s interesting. I may follow their approach

It seems to specially handle value/defaultValue, checked/defaultChecked, then set everything as properties except for href because of this quirk (although this can still be handled with some consideration) & a few others

@qeleb
Copy link
Author

qeleb commented May 14, 2024

@ryansolid Would you be willing to merge this optimization if it were opt-in only like omitNestedClosingTags?

@ryansolid
Copy link
Owner

@ryansolid Would you be willing to merge this optimization if it were opt-in only like omitNestedClosingTags?

That might be a good way to start it. Mostly I don't want to break in preparation for SolidStart release.

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.

None yet

2 participants