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

fix: don't set idl on custom elements that are interested in the corresponding content attribute #4478

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hesxenon
Copy link
Contributor

No description provided.

@hesxenon
Copy link
Contributor Author

hesxenon commented Aug 21, 2024

On the contrary - preact is breaking things now because it guesses what the user wants now. It presumes that when I write web components I want preact to take care of the IDL side of things only, which is not what I want when I put a (content) attribute name into the observedAttributes array. This array is by definition there to inform the browser that you are interested in changes to these content attributes and in combination with the MDN glossary on IDL this means (as I understood it) that the IDL attribute should reflect the content attribute, not the other way around. In other words:

class MyCustomElement extends HTMLElement {
  static observedAttributes = ["href", "disabled", "foobar"];
  
  href;

  disabled;
  
  foobar;

  connectedCallback() {
    console.log(this.href, this.disabled, this.fobar);
  }
}
customElements.define("my-custom-element", MyCustomElement);

// render with preact
<my-custom-element href="/some/url" disabled foobar="bazqux"></my-custom-element>

will (confusingly) lead to the following HTML:

<my-custom-element href="/some/url"></my-custom-element>

but it will log all three properties.

This is also out of line with the following example

// given this element in the dom
<input value="foo" />

const input = document.querySelector("input")
input.value = "bar"
console.log(input.value, input.getAttribute("value")) // logs bar, foo, since IDL doesn't update content on purpose

Long story short: the current behaviour of preact breaks webcomponents that try to follow IDL and Content rules. This PR would fix that. Components that currently indicate they are interested in an attribute by specifying its name in observed Attributes that then expect this attribute to be treated like a prop are, well... let's say they're a mystery to me and imho they're going against the spec.

Btw, wouldn't the current behaviour also mean that e.g. stenciljs components that define

@property({
  reflect: false // the default
})
someProp

would "break" in the sense that checks for the content attribute someProp would fail? Those could be either done with getAttribute from JS or, probably more importantly, from CSS - meaning that styling that relies on the content attribute would fail because of preacts assumptions?

@hesxenon hesxenon marked this pull request as draft August 21, 2024 07:24
@hesxenon
Copy link
Contributor Author

Converted to draft because regardless of the outcome tests are still missing

@rschristian

This comment was marked as off-topic.

@hesxenon
Copy link
Contributor Author

preact is a js framework

and how many users do you know that even know about the IDL vs. Content gap? I'd say preact is primarily seen as a "rendering soluition" and falling into the mentioned gap while the MDN docs explicitly mention observedAttributes as a way to well, observe attributes shouldn't just be dismissed by saying "js (IDL) first, tough luck" imho 😅.

My point is (and that's what that example was supposed to show) that there is also a gap between content and IDL and that's by design and should be taken into consideration by webcomponent authors following "existing rules". And following those rules leads to incompatibilities with preact (and react for that matter) which basically leads to "you can use preact/react as long as you stay in the ecosystem" all while stating "we're closer to the platform than the rest".

…ed yet without incurring additional runtime cost
@rschristian

This comment was marked as off-topic.

@hesxenon
Copy link
Contributor Author

and pretending it doesn't exist doesn't help anyone.

sad but true

we're not web components

true, but you claim to be compatible with webcomponents by staying closer to the platform - which webcomponents can I use with preact with this behaviour then that aren't also written with preact/react in mind (which is the antithesis to webcomponents...)?

use a workaround

I'd be happy to know which workaround this could be?

perf and size

This one line shouldn't break perf and surely doesn't break size? And it shouldn't break existing projects since for those to break they'd have to use webcomponents that explicitly go against the mentioned gap.

Honestly I'm finding myself between a rock and a hard place here since I'm now faced with either changing the webcomponents to consider being used within react/preact or dropping react/preact entirely or using a custom preact fork (wasn't able to adapt react as easily) for the time being.

@coveralls
Copy link

coveralls commented Aug 21, 2024

Coverage Status

coverage: 99.485% (-0.001%) from 99.486%
when pulling 5ca6c52 on hesxenon:main
into 2c6df95 on preactjs:main.

@rschristian

This comment was marked as off-topic.

@hesxenon
Copy link
Contributor Author

I am not against the "sigil" approach - but this is a much smaller change, both internally and externally?

The proposed workaround then is to design the webcomponent with preact in mind - since uppercasing the content attribute while lowercasing the IDL introduces complexity in the webcomponent for the purpose of making it compatible with a framework :/

Plus it means that the IDL attribute does not technically reflect the content attribute, which does go against the definition of IDL attributes?

but don't intend on having those attribute set from themselves

Keeping in mind the existing "native elements" like select, input, div etc - they all set their IDL attributes according to the content attributes - that's what "IDL reflects content" means, no? And the IDL attribute is named like the content attribute with very few exceptions that mostly relate to camelCasing kebab-cased attributes.

I don't think use of that API sends the signal that you apply here.

I agree - in isolation it does not send the signal that I apply here. But in combination with the definition of IDL attributes and existing implementations it does, doesn't it? IDL attributes should reflect their content counterparts (and may transform them), observedAttributes means "I'm interested in changes to the content attribute" and existing implementations update their IDL attributes if the content attribute changes.

But I think I've found a "loophole" here. Quoting from MDN:

The IDL attribute is always going to use (but might transform) the underlying content attribute to return a value when you get it and is going to save something in the content attribute when you set it

"is going to save something in the content attribute when you set it" being the key part here. That sounds to me like webcomponents should reflect the IDL change back into the content... I don't see yet how to prevent infinite loops here but that's hardly preacts problem...

I'd still be interested on your thoughts about that and how that relates to the aforementioned existing implementations - e.g. why document.querySelector("input").value = "foobar" does set the IDL attribute but does not update the content attribute

@hesxenon
Copy link
Contributor Author

hesxenon commented Oct 8, 2024

since this PR has been approved, can I get some pointers regarding tests? I don't think this should be marked as ready to merge without tests?

@marvinhagemeister
Copy link
Member

FYI: The person that approved it has no approval rights and is a bot. They've been going over the preact and graphqljs repo and approving every open PR. It's a spam bot.

@hesxenon
Copy link
Contributor Author

hesxenon commented Oct 8, 2024 via email

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.

5 participants