preact icon indicating copy to clipboard operation
preact copied to clipboard

Preact + media-chrome: Props are not passed correctly

Open DCsunset opened this issue 1 year ago • 7 comments

  • [x] Check if updating to the latest Preact version resolves the issue (using Preact v10.13.1)

Describe the bug When using Preact with react components in media-chrome package, the props are not passed to the component correctly. For example, the rates for MediaPlaybackButton couldn't be set correctly. (it works correctly when using React)

To Reproduce

StackBlitz: https://stackblitz.com/edit/create-preact-starter-l7wbnq?file=src%2Findex.jsx

Steps to reproduce the behavior:

  1. Click on playback rate button "1x"
  2. The next rate is "1.2x"

Expected behavior What should have happened when following the steps above?

  • The next playback rate is supposed to be "2x" as the props are set to rates = "1 2 3" (docs: https://www.media-chrome.org/docs/en/components/media-playback-rate-button)
  • The props are not passed to the component when viewing the source of the html

DCsunset avatar Sep 01 '24 15:09 DCsunset

This is running afoul of our props vs attribute detection, as they've unfortunately defined a rates getter & setter which means Preact assumes they want this set as a property, rather than an attribute.

#4478 would address this, though I'm not particularly fond of it (others may disagree).

rschristian avatar Sep 02 '24 03:09 rschristian

I see. Is there any workaround for this? I tried capitalizing it but it still doesn't work.

DCsunset avatar Sep 04 '24 16:09 DCsunset

Refs, for example:

<MediaPlaybackRateButton ref={(d) => d.setAttribute('rates', '1 2 3')} />

rschristian avatar Sep 05 '24 03:09 rschristian

Just a tiny note:

unfortunately defined a getter

having a property is what you'd expect from a web component, is it not? IDL attributes are supposed to be reflections of their content counterpart; preacts current behaviour forces CEs to either:

  1. use "shadow attributes" - i.e. content attributes that aren't represented as IDL
  2. change the content attribute when the idl part changes (which kinda switches the intended order of things imho but I might be wrong here)

hesxenon avatar Sep 13 '24 12:09 hesxenon

having a property is what you'd expect from a web component, is it not?

Potentially, but it's the combination of defining a getter/setter yet expecting attributes to be set. Preact assumes that if you've set a getter/setter, you'd expect to Preact/your framework use those rather than switch to attributes.

change the content attribute when the idl part changes

Potentially, yes. Being a JS framework, I don't think this is problematic; properties are the native interface for interacting with DOM elements. Most properties don't need to be reflected back to attributes, and a few shouldn't. That's not to say the use case is invalid, but that, IMO, properties should be the interface of choice and "the intended order" so to speak.

rschristian avatar Sep 13 '24 12:09 rschristian

you'd expect your framework to use those

But I don't expect any framework, that's why I'm writing a CE ;) anyway, this discussion should probably be done in the mentioned PR, if at all.

And about the order: :woman_shrugging: possibly. I just find it odd that since the content attribute is there first and the CE is actively upgraded to read the content attribute into its IDL attribute that from there on out the IDL attribute should take charge. In my case I've done just that and it works as expected. Looking through webcomponents.org (specifically githubs own CEs) many also do just that.

hesxenon avatar Sep 13 '24 12:09 hesxenon

I just find it odd that since the content attribute is there first

Depends on your usage. If you're registering a Preact component as a custom element, and are already using it in your HTML doc with defined attributes, sure, I agree. This issue shows a different usage though, with a Preact wrapper around a custom element -- it doesn't exist until Preact renders it. I don't think there is a right or wrong here, it's always going to be a guess.

I'm not sure if there's any consensus/strong opinions one way or the other but we'll update the PR when/if there is.

rschristian avatar Sep 13 '24 12:09 rschristian

Custom Elements that define public properties are supposed to reflect them to attributes, since that's how all of the built-in elements work. Sure, a CE can just not do that, but the overwhelming majority of them do. It's part of what makes a Custom Element an Element, rather than just a framework-agnostic blob of JS mounted into the DOM.

Things look/feel broken when elements don't do this:

const el = document.createElement('foo-whatever')

el.setAttribute('rates', '1')
el.getAttribute('rates')  // '1'
console.log(el.rates) // '1'

el.rates = '2'
console.log(el.rates) // '2'
console.log(el.getAttribute('rates')) // '1'  ??

developit avatar Oct 27 '24 15:10 developit

Closing this as after further research I align with @developit's explanation here.

JoviDeCroock avatar Nov 01 '24 08:11 JoviDeCroock

@rschristian Don't wanna "necro" this, but while I can follow your explanation I don't think people that use frameworks view their written JSX expressions as an alternative way to create JS instances and set values on them. They see "html in JS and somehow there's functions now". That's a POV that only thinks about the content attributes in my experience. Which is why trying to explicitly control e.g. a details elements open state can be really confusing because the details element does exactly what's being suggested here.

I think both react and preact need to think long and hard whether they still want to take the "JS First" Position when it comes to actual elements.

hesxenon avatar Nov 01 '24 09:11 hesxenon

I don't think people that use frameworks view their written JSX expressions as an alternative way to create JS instances and set values on them. They see "html in JS and somehow there's functions now". That's a POV that only thinks about the content attributes in my experience.

Don't know what to say besides that's an incorrect interpretation of what JSX is. People may hold that view but it's a misunderstanding.

Which is why trying to explicitly control e.g. a details elements open state can be really confusing because the details element does exactly what's being suggested here.

Not sure I follow, <details> reflects the value of .open to the open attribute just fine. It's acting exactly how we're recommending users build their custom components.

const details = document.createElement('details');

details.open = true;
details.getAttribute('open'); // `""`

details.open = false;
details.getAttribute('open'); // `null`

The problem in this issue is that the custom element doesn't reflect the property back to the attribute, yet expects the attribute to be set. This is a divergence from the way most other properties on the platform work. It can make sense in some situations (like .value, as it's dangerous to reflect that to an attribute) but it doesn't here.

rschristian avatar Nov 03 '24 10:11 rschristian

It's acting exactly how we're recommending users build their custom components.

Yes, and for the most part I agree with this recommendation but there are a few points that should not be omitted.

This is not the default in most web component frameworks.

E.g. the lit docs state

Attributes should generally be considered input to the element from its owner, rather than under control of the element itself, so reflecting properties to attributes should be done sparingly.

A similar sentiment can be found when using stencil.js

In some cases it may be useful to keep a Prop in sync with an attribute.

Imho this reads as "the default should be to not reflect IDL to content". Depending on your POV this is at odds with the definition about IDL on MDN but well...

Intention vs Reality

To quote Alan Kay:

I made up the term object-oriented, and I can tell you I did not have C++ in mind.

Yes, JSX in the context of react is not a HTML first templating language, yes people misunderstand that. That doesn't make the problems go away (sadly). (P)React simply stating "this is not how you're supposed to use these things" doesn't prevent people from falling into this pit. Maybe a "how to write CEs in Preact" docs page would be nice?

State -> IDL <-> Content

Try the following:

  1. render a details element
  2. control its open prop
  3. close the details element
  4. you're not in control anymore

CEs changing an IDL attribute that has been set by (P)React state creates an "out-of-sync" state. This is made even worse when the next re-render sets the prop to the controlled value again, because that re-render might be triggered from something else entirely. Imagine closing the details element, typing into a controlled input somewhere else and suddenly the details element opens again because the (P)React state is still "open".

I hope I got the point across that controlling CEs has some pitfalls still, even if they are implemented "correctly". Sure, bridging that gap is out of scope for this issue but I don't think either Preact nor React can claim "web component compatibility" when the whole premise of the rendering model can be broken without a fix.

hesxenon avatar Nov 05 '24 22:11 hesxenon