Add syntax to bind to properties instead of attributes #2819
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Visit the preview URL for this PR (updated for commit dfd2082): https://yew-rs-api--pr2819-properties-not-attri-4sjqxi5x.web.app (expires Sun, 21 Aug 2022 16:07:06 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Benchmark - SSRYew Master
Pull Request
|
Size Comparison
✅ None of the examples has changed their size significantly. |
hamza1311
force-pushed
the
properties-not-attributes
branch
from
f8f506f
to
ea353cc
Compare
packages/yew-macro/src/props/prop.rs
Outdated
| pub label: HtmlDashedName, | ||
| /// Punctuation between `label` and `value`. | ||
| pub value: Expr, | ||
| } | ||
| impl Parse for Prop { | ||
| fn parse(input: ParseStream) -> syn::Result<Self> { | ||
| let at = input.parse::<Token![@]>().map(|_| true).unwrap_or(false); |
There was a problem hiding this comment.
Let's discuss some syntax and why I think using @ is not not intuitive.
@{}is used for dynamic tags already. We're currently not supporting dynamic attribute names, but if we were to, I'd reuse the syntax there. Since this feature does something else, I find it confusing.- vue syntax calls this style of modifying what a prop assignment means a "directive". There's a directive for callbacks, i.e.
@click=v-on:click= a callback on the click event. And there's a directive for attribute binding,:href=v-bind:href= sets the attribute of the element. - Rust has alternative syntax for something akin to directives: Attributes
My preferred solution: use Token![:], i.e. :attr-name. An eventual dynamic attribute name would look like :@{expr}={value}. When we want to support further directives, we could use #[attr] attr-name={value} and #[on] event={handler} (not so sure about a short-hand form for event handlers, $click?).
Can we also support a syntax such as .prop-name (or #[prop] prop-name) to explicitly assign to a property? This is important for the complication with the property reflecting an attribute not being always named the same. See below on the comment about "class".
| ApplyAttributeAs::Property => { | ||
| match key { | ||
| // need to be attributes because, otherwise query selectors fail | ||
| "class" => el.set_attribute(key, value).expect("invalid attribute key"), |
There was a problem hiding this comment.
Instead of special casing this at runtime, would it be better to do this handling in the macro?
To explain, HTML attributes are reflected by properties on the HtmlElement. The complication is that this mapping changes some names.
:classis reflected by.className. Imo,html! { <div class="foo" /> }could be rewritten - handling this mismatch - intohtml! { <div .className="foo /> }.- some other attributes that change their name while reflected as a property
The approach would be that the syntax name would rewrite some several well-known attribute names into their proper property names, and assign those at runtime instead. .name would bypass this rewriting and just assign to the property as named. At least, afaik, the list of attributes that get reflected by the samely named property is small. I can't find a good list though to link here.
Another special case (why is this so messy?... I hate browsers) is the value attribute. This is reflected by the defaultValue property because, well, it only assigns the default value of an <input>. The developer expection (and the current implementation, I believe) is that it dynamically assigns the value property instead (and bypasses the value attribute completely). Similarly with :selected is reflected as .defaultSelected but I think dev expectation of dynamically binding to .selected is preferred. See also #2530
There was a problem hiding this comment.
I removed the special casing entirely as the properties are no longer set by default. They are explicitly opt-in so it's user's responsibility to set the correct one.
|
I am not sure whether making properties default is better than existing behaviour. Most issues mentioned in this pull request can be solved without making properties default.
Other things to consider:
|
You're right, it's better to keep the option to the default as attributes and give the option to set as property I made it use
|
| .expect("could not remove attribute"), | ||
| ApplyAttributeAs::Property => { | ||
| let key = JsValue::from_str(key); | ||
| js_sys::Reflect::set(el.as_ref(), &key, &JsValue::UNDEFINED) |
There was a problem hiding this comment.
Sometimes setters may not be expecting undefined and this can cause panic.
I am not sure what is the best solution here.
Although, there is an option to leave the last set value as-is and do nothing to avoid setting something unintended, it's equally unclean to leave a residual value.
I don't think either set to undefined or leave as-is is semantically better than the other option as JavaScript only allows properties to listen to get and set but not delete event.
So I am fine with it being handled either way, just bringing this up as an alternative option.
There was a problem hiding this comment.
It already used to panic when attribute couldn't be set so I think consistency here is fine. If it is to be changed, it should be done for both properties and attributes
| /// A collection of attributes for an element | ||
| #[derive(PartialEq, Eq, Clone, Debug)] | ||
| pub enum Attributes { | ||
| /// Static list of attributes. | ||
| /// | ||
| /// Allows optimizing comparison to a simple pointer equality check and reducing allocations, | ||
| /// if the attributes do not change on a node. | ||
| Static(&'static [[&'static str; 2]]), | ||
| Static(&'static [(&'static str, &'static str, ApplyAttributeAs)]), |
There was a problem hiding this comment.
Instead of making properties a variant here, it might be cleaner to make a separate properties field on VTag.
So we do not have to check for every entry whether it is a property or an attribute during the diffing process and a property and an attribute with the same name can co-exist.
There was a problem hiding this comment.
That's for a different PR, imo. We can break the data into separate struct. &'static str and AttrValue::Static should be the same (after - optimizations) so that can be used. It should also reduce the code complexity
| async fn macro_syntax_works() { | ||
| #[function_component] | ||
| fn Comp() -> Html { | ||
| html! { <a href="https://example.com/" ~alt="abc" /> } |
There was a problem hiding this comment.
| html! { <a href="https://example.com/" ~alt="abc" /> } | |
| html! { <a href="https://example.com/" alt~="abc" /> } |
Maybe it's cleaner to denote it on the operator than in front of the name?
Although I am fine with it at the beginning as well.
There was a problem hiding this comment.
I believe in front of the name is better: ~ arguably modifies the name, not the assignment and ~= is confusing since it can be read as alt(~=) instead of (alt~)= and the reader is left wondering what the ~= operator does, is it something like +=?
There was a problem hiding this comment.
That also causes parsing problems since ident*=expr is valid Rust but ~ can't be used there so parser doesn't know what to do
| .. | ||
| }| { | ||
| ( | ||
| label.to_lit_str(), |
There was a problem hiding this comment.
For properties, maybe we should apply a snake_case to camelCase conversion here like web_sys.
We only need to do this with html! macro as at runtime property names are set as string.
// snake case as we assume a it's like a field in Properties for an html element.
html! { <custom-element ~some_prop={"value"} /> };
// camel case as it's a string.
el.add_property("someProp", "value");There was a problem hiding this comment.
I'm not sure if this is a good idea. We don't convert snake_case to kebab-case for attributes so this will be just another differentiator between the two. We can add it for both though, but that is for a future PR
| /// Set the given key as property on the element | ||
| /// | ||
| /// [`js_sys::Reflect`] is used for setting properties. | ||
| pub fn add_property(&mut self, key: &'static str, value: impl Into<AttrValue>) { |
There was a problem hiding this comment.
In the future, we should allow properties to be anything Into<JsValue>, but Into<AttrValue> is fine for this pull request.
There was a problem hiding this comment.
This sounds like opening gates for a disaster. onclick and ~onclick have different meanings then. The former is handled through Yew while the other would completely bypass Yew (and all the bubbling, etc) and apply directly to the element, all without use of noderef. Unless someone comes with a genuine need for this, I wouldn't want to add this
|
@WorldSEnder can you leave review here? It's blocked because of your review |
WorldSEnder
changed the title
|
Why did we choose to have the special syntax to demark explicitly setting a property, and not the attribute instead (as per #1322)? Or have I misread? UPDATE: I see some rationale to retain the default as attr given SSR. I think I’d personally rather SSR require explicit attr setting and error out if a default property was set. Perhaps even better, introduce a new dom! macro to distinguish it from html! To me, the html! macro is more about manipulating the dom as in manipulating properties… The distinction between attributes and properties is overall quite confusing, and surprising to the Yew developer, and needing to explicitly address a property perhaps adds to that given the prevalence of Props elsewhere. |
For one, to conserve a bit of backwards compatibility. For example type-safety dictates that attributes must be stringly typed ( For two, because the current syntax gives us wiggle room. The current syntax without any qualifiers reserves to do secret tricks that match and mix attributes and properties and try to do "what you meant". And third, because properties and attributes are just named differently. When typing out what looks like html, dashed casing like EDIT: I would love to learn more about the interaction with custom elements. Do you happen to use them and can you share if the default should be different there? I think a lot of the arguments above apply to default elements shipped by the browser, but are they applicable to custom elements? |
Thanks for the explanations. Perhaps the existing approach could be preserved but a new, complimentary, dom-focused macro introduced for properties. My main concern is the developer’s mental consideration of whether specifying a property or attribute is required. My assertion is that most devs don’t understand the difference between attributes and properties. Are these concerns shared?
Custom elements as per Components? They are definitely dom-focused and more consistent with what I’m thinking of with a separate dom-focused macro. |
Description
Fixes #1322
Fixes #2530
Properties are set by using
js_sys::Reflect. While it doesn't give much benifit today (they're still strings), it opens the door for more opportunities since we can now define specific types for properties. This will help with statically typed DOM, whenever that becomes a thing.This PR also introduces new syntax,
~key="value"which can be used to forcefully set as properties.classis set as always attribute, since it is not a property. Classes are stored inclassListin DOM. Since they're always strings, it's not worth the extra JS calls to add toclassList.Checklist