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

Preact + media-chrome: Props are not passed correctly #4486

Open
1 task done
DCsunset opened this issue Sep 1, 2024 · 7 comments
Open
1 task done

Preact + media-chrome: Props are not passed correctly #4486

DCsunset opened this issue Sep 1, 2024 · 7 comments

Comments

@DCsunset
Copy link

DCsunset commented Sep 1, 2024

  • 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?

@rschristian
Copy link
Member

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

@DCsunset
Copy link
Author

DCsunset commented Sep 4, 2024

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

@rschristian
Copy link
Member

Refs, for example:

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

@hesxenon
Copy link
Contributor

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)

@rschristian
Copy link
Member

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.

@hesxenon
Copy link
Contributor

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: 🤷‍♀️ 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.

@rschristian
Copy link
Member

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.

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

No branches or pull requests

3 participants