Implicit optional attributes #1637

siku2 · 2020-10-29T15:53:42Z

Description

See #1550 for further details.

This is still very WIP but from the looks of it this is gonna require A LOT of breaking changes.

Closes: #1550
Closes: #1623 (this was required to support optional prop_or*)

Goals

Get rid of Transformer trait. It's already very confusing and it just keeps getting worse. Moves complexity out of the macro making it more transparent.
Rethink the implicit transformations done by the Transformer trait. It's no longer possible to sustain all of them without specialization. They also make it very easy to write suboptimal code because they obscure what's happening under the hood.
Unify the component and element prop behaviour.

TODO

Update a ton of documentation 😭 (done by @cecton)
Add a proper optional property attribute for Option<T> types. #[prop_or*] can't be used for this because types end up being Option<Option<T>> which is very confusing. (done by @cecton)

it's gonna be gone soon anyway *shrugs*

siku2 · 2020-10-30T17:15:08Z

@jstarry, I'd love to hear your thoughts on the following two points:

1. Unifying attributes with props

Attributes now behave like a prop with type #[prop_or_default] Option<Cow<'static, str>> and listeners behave like #[prop_or_default] Option<Callback<EVENT>>.
The reason for this (apart from internal consistency) is that it helps move us toward a properly typed DOM (which will come into play when we switch to binding to properties instead of attributes [#1322]).
This comes with the breaking change that attributes no longer accept any old impl ToString type.

2. Pesky transformations

Rust really doesn't like it but now we do actually need to have a "transformer" trait to support implicit optional attributes.
Specifically, for optional props we need at least T -> Option<T> and Option<T> -> Option<T>.
Everything else is technically optional, but we probably want it for convenience.

The &T -> T transformation is desired because props currently have to own their data (with the exception of static references of course). However, a big problem with this transformation is that it hides a potentially very expensive operation which can lure the user into writing non-optimal code. My current implementation requires that T implements the marker trait ImplicitClone. It allows the user to easily pass down things like &Rc<T>, &Callback, and even &String (though that last one arguably shouldn't be implicit, we just don't really have a better solution right now) but stops them from accidentally cloning their ExpensiveState struct all over the app.

So far we haven't introduced any type conversions, we've basically only expanded on Rust's type coercions, but there's at least one area where we want to allow some type conversions: strings. Specifically, we want [&'static str | String] -> Cow<'static, str> and &'static str -> String so we can use string literals for props.

This is where the problem begins. In an ideal world we could just implement the clone conversion as &T -> V where T: ImplicitClone and [T -> V] and the two conversions for optional props as T -> Option<V> where [T -> V] and Option<T> -> Option<V> where [T -> V] but Rust isn't there yet. None of these can be implemented today.
Currently, in order to support &'static str -> Cow<'static, str> we have to manually implement:

&'static str -> Cow<'static, str>
&'static str -> Option<Cow<'static, str>>
Option<&'static str> -> Option<Cow<'static, str>>

If we want to support the ImplicitClone conversion for a type transformation, each of the three implementations also has to be implemented for the reference.
This isn't a big deal internally, we can use a macro. The problem is that, as a user, implementing this for a custom type is a nightmare.

Btw, the situation isn't all that different from before, it's just that the new optional attributes really makes this problem obvious.

jstarry · 2020-11-05T16:01:28Z

@siku2 sorry for the delay. First off, I whole-heartedly agree with your stated goals. As for your two points, I just skimmed and your arguments seem reasonable but I haven't expended any serious thought into it. I trust your knowledge over the finer details of Rust's type system over my own at this point. That being said, I'd like to learn more and I can find time over the weekend to dive into this more if you would like to talk things out more. But if you're pretty comfortable with the current direction, by all means blaze ahead 👍

siku2 · 2020-11-07T21:56:09Z

But if you're pretty comfortable with the current direction, by all means blaze ahead +1

@jstarry
Hah, that's the problem, I'm really not :D
I feel like the current solution is too complicated to easily explain (which isn't a good thing for something that happens behind the scenes) and it's probably not a good idea to fight the compiler this much in the first place.
Without the specialization feature I don't think it's possible to fully realise my approach without residing to some hacks. Since specialization seems to be nowhere near stabilization we'll just have to accept that for now.

I was hoping your input might give me some inspiration. If you do happen to come up with an idea, as crazy as it might be, I would love to hear it.
For now I'll just push ahead with the current approach, maybe I can come up with something while writing the documentation.

Oh and I'd also love to know which transformations (if any?) you think are essential.

jstarry · 2020-11-15T18:15:10Z

This is where the problem begins. In an ideal world we could just implement the clone conversion as &T -> V where T: ImplicitClone and [T -> V] and the two conversions for optional props as T -> Option where [T -> V] and Option -> Option where [T -> V] but Rust isn't there yet

Yeah, I think it's ok to skip those.

The &T -> T transformation is desired because props currently have to own their data

I think it's better to have the clones be explicit and avoid adding the ImplicitClone trait market.

we need ... &'static str -> String

Do we?

jstarry · 2020-11-22T16:14:07Z

I think it's better to have the clones be explicit and avoid adding the ImplicitClone trait market.

Actually this would be really nice for callbacks

siku2 · 2020-11-22T16:24:47Z

I think it's better to have the clones be explicit and avoid adding the ImplicitClone trait market.

Actually this would be really nice for callbacks

Yeah, I originally wanted to remove implicit cloning altogether but I think it's really nice for things like Rc which is why I came up with the ImplicitClone marker in the first place. I want to remove it from String though because there it's deceiving at best.

we need ... &'static str -> String

Do we?

We don't really, but I don't think it's harmful either. I added it because many components accept String props and if you want to pass down a string literal you have to use <Foo prop="value".to_owned() /> anyway. I'm perfectly happy to remove it but this is much less of a problem than the Option<_> stuff.

jstarry · 2021-01-23T16:15:34Z

Hey @siku2 what are your latest thoughts on this PR? Think it's ready to come out of draft mode? When we make a decision on it, I would like to have it cherry picked to the v0.18 branch after merging to master

siku2 · 2021-01-23T22:17:57Z

@jstarry It's been a while but I think the issue still lies with the ergonomics of it all. I think I was unhappy with how complex the rules still are... I'll try to get this up to speed as soon as possible but at this point I certainly wouldn't mind if someone else got there before me :P

cecton · 2021-03-27T09:40:55Z

docs/concepts/html/components.md

-## Transformers
-
-Whenever you set a prop its value goes through a transformation step first.
-If the value already has the correct type, this step doesn't do anything.
-However, transformers can be useful to reduce code repetition.
-
-The following is a list of transformers you should know about:
-
- `&T` -> `T`
-
-  Clones the reference to get an owned value.
-
- `&str` -> `String`
-
-  Allows you to use string literals without adding `.to_owned()` at the end.
-
- `T` -> `Option<T>`
-
-  Wraps the value in `Some`.
-
-```rust
-struct Props {
-    unique_id: Option<usize>,
-    text: String,
-}
-
-struct Model;
-impl Component for Model {
-    type Properties = Props;
-
-    // ...
-}
-
-// transformers allow you to write this:
-html! { <Model unique_id=5 text="literals are fun" /> };
-// instead of:
-html! { <Model unique_id=Some(5) text="literals are fun".to_owned() /> };
-```
-


I removed the entire section here. I added a note about Into<Option<T>> later on.

cecton · 2021-03-27T09:41:16Z

docs/concepts/html/elements.md

-    <@{format!("h{}", level)} class="title">{ content }</@>
+    <@{format!("h{}", level)} class="title">{ text }</@>


Would be better if the tests were executed 😞

cecton · 2021-03-27T09:42:36Z

docs/concepts/html/elements.md

-Most HTML attributes can be marked as optional by placing a `?` in front of
-the `=` sign. This makes them accept the same type of value as otherwise, but
-wrapped in an `Option<T>`:
+Most HTML attributes can use optional values (`Some(x)` or `None`). This allows
+to omit the attribute if the attribute is marked as optional.


I tried to put things simply. But any advice are welcome 😅

cecton · 2021-03-27T09:42:58Z

docs/concepts/html/elements.md

+Please note that it is also valid to give only the value as properties behave
+like `Into<Option<T>>`:
+
+```rust
+let id = "foobar";
+
+html! {
+    <div id=id></div>
+}
+```
+


This is the remark I added about the optional attributes.

cecton · 2021-03-27T10:08:56Z

packages/yew/src/html/conversion.rs

+
+impl ImplicitClone for NodeRef {}
+impl<Comp: Component> ImplicitClone for Scope<Comp> {}
+// TODO there are still a few missing like AgentScope


I did the AgentScope... Can someone help me enumerate what else is missing? 😅

I can't think of anything else that is missing. In the worst case, the user will have to call clone if this trait isn't implemented. Let's just roll with it for now. We'll know what's missing once this code is merged and used.

ranile

It seems like the failing CI is because of new Clippy lints introduced in Rust 1.51. #1801 fixed those in master so I believe it can be rebased here too.

docs/concepts/html/elements.md

packages/yew-macro/src/derive_props/field.rs

ranile · 2021-04-25T07:42:18Z

packages/yew-macro/src/props/element.rs

+            .pop("class")
            .map(|prop| ClassesForm::from_expr(prop.value));
        let value = props.pop("value");
        let kind = props.pop("type");
-        let checked = props.pop_nonoptional("checked")?;
+        let checked = props.pop("checked");


Is there any reason why these 4 are special cased?

ranile · 2021-04-25T07:46:35Z

packages/yew/src/html/conversion.rs

+// TODO there are still a few missing like AgentScope
+
+/// A trait similar to `Into<T>` which allows conversion to a value of a `Properties` struct.
+pub trait IntoPropValue<T> {


Can we implement this for T and &T where T: ToString?

No... I think it's because of specialization

Co-authored-by: Muhammad Hamza <muhammadhamza1311@gmail.com>

…ests (yewstack#1801) * update rust version for macro test to 1.51 * enable const generic tests * run integration tests using MSRV * am blind * clippy, fmt * apply suggestion

Co-authored-by: Muhammad Hamza <muhammadhamza1311@gmail.com>

cecton · 2021-04-25T12:48:40Z

It seems like the failing CI is because of new Clippy lints introduced in Rust 1.51. #1801 fixed those in master so I believe it can be rebased here too.

Thanks a lot!! I cherry-picked 🙏

siku2 added 7 commits October 23, 2020 16:50

start

7d330aa

update examples

f80566e

optional attributes for elements

198567a

update macro tests

e48605e

update yew-stdweb todomvc example

0ed35ff

it's gonna be gone soon anyway *shrugs*

remove quote spanned

5b7ee05

remove transformer trait

f6571e1

siku2 added ergonomics macro Issues relating to our procedural or declarative macros labels Oct 29, 2020

siku2 added this to the v0.18 milestone Oct 29, 2020

siku2 added 2 commits October 29, 2020 18:06

first draft for unified prop handling

f8adf9b

update examples

815147b

siku2 mentioned this pull request Nov 5, 2020

Extract Classes to a separate macro #1601

Merged

3 tasks

update optional-attrs

ec14013

siku2 added 2 commits November 21, 2020 18:01

don't even remember to be honest

692000a

update optional-attrs branch

907f05a

siku2 added 4 commits November 25, 2020 17:48

Update optional-attrs

f2e6d99

remove implicit cloning from strings

0535bdc

update examples and docs

8de3926

update stdweb examples

14a237a

siku2 mentioned this pull request Dec 12, 2020

Listeners should take Into<Callback<Event>> instead of Callback<Event> #1657

Closed

3 tasks

jstarry changed the base branch from master to v0.18 January 24, 2021 00:58

cecton added 4 commits March 20, 2021 10:00

Clippy

b317330

Oops this is not public API, we can fix it

45d63a5

Update documentation

3ee805e

Revert probable temporary change

5f98c06

cecton reviewed Mar 27, 2021

View reviewed changes

cecton added 2 commits March 27, 2021 11:03

Solve some TODOs

8037e78

Clippy fix

dfe7ccb

cecton reviewed Mar 27, 2021

View reviewed changes

Fix some doc

e488e99

ranile reviewed Apr 25, 2021

View reviewed changes

cecton marked this pull request as ready for review April 25, 2021 08:09

cecton and others added 3 commits April 25, 2021 13:44

Update packages/yew-macro/src/derive_props/field.rs

af93a08

Co-authored-by: Muhammad Hamza <muhammadhamza1311@gmail.com>

Update Rust version for macro tests to 1.51 & enable const generics t…

9728825

…ests (yewstack#1801) * update rust version for macro test to 1.51 * enable const generic tests * run integration tests using MSRV * am blind * clippy, fmt * apply suggestion

Update docs/concepts/html/elements.md

390abdb

Co-authored-by: Muhammad Hamza <muhammadhamza1311@gmail.com>

cecton added 3 commits April 25, 2021 14:01

Clippy stuff

004ef9a

Update test stderr for rustc 1.51

78a3129

More fix for 1.51.0

ac09c97

cecton merged commit 99d5088 into yewstack:v0.18 Apr 30, 2021

siku2 deleted the optional-attrs branch May 5, 2021 20:48

ranile pushed a commit to ranile/yew that referenced this pull request Jun 5, 2021

Implicit optional attributes (yewstack#1637)

4eecb75

ranile pushed a commit to ranile/yew that referenced this pull request Jun 5, 2021

Implicit optional attributes (yewstack#1637)

43d5a61

siku2 added a commit that referenced this pull request Jun 5, 2021

Implicit optional attributes (#1637)

f17d370

mc1098 mentioned this pull request Jun 13, 2021

Remove warning about prop_or_else macro #1917

Merged

3 tasks

mc1098 mentioned this pull request Sep 26, 2021

Consuming Web Components is very unergonomic #1666

Open

5 tasks

voidpumpkin added the A-yew Area: The main yew crate label Nov 26, 2021

ranile mentioned this pull request Jul 28, 2022

Allow using Option<VNode> in html!() macro #2792

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implicit optional attributes #1637

Implicit optional attributes #1637

siku2 commented Oct 29, 2020 •

edited by cecton

Loading

siku2 commented Oct 30, 2020 •

edited

Loading

jstarry commented Nov 5, 2020

siku2 commented Nov 7, 2020 •

edited

Loading

jstarry commented Nov 15, 2020

jstarry commented Nov 22, 2020

siku2 commented Nov 22, 2020 •

edited

Loading

jstarry commented Jan 23, 2021

siku2 commented Jan 23, 2021 •

edited

Loading

cecton Mar 27, 2021

cecton Mar 27, 2021

cecton Mar 27, 2021

cecton Mar 27, 2021

cecton Mar 27, 2021

ranile Apr 25, 2021

cecton Apr 25, 2021

ranile left a comment

ranile Apr 25, 2021

cecton Apr 25, 2021

ranile Apr 25, 2021

cecton Apr 25, 2021

cecton commented Apr 25, 2021

		<@{format!("h{}", level)} class="title">{ content }</@>
		<@{format!("h{}", level)} class="title">{ text }</@>

Implicit optional attributes #1637

Implicit optional attributes #1637

Conversation

siku2 commented Oct 29, 2020 • edited by cecton Loading

Description

Goals

TODO

siku2 commented Oct 30, 2020 • edited Loading

1. Unifying attributes with props

2. Pesky transformations

jstarry commented Nov 5, 2020

siku2 commented Nov 7, 2020 • edited Loading

jstarry commented Nov 15, 2020

jstarry commented Nov 22, 2020

siku2 commented Nov 22, 2020 • edited Loading

jstarry commented Jan 23, 2021

siku2 commented Jan 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cecton commented Apr 25, 2021

siku2 commented Oct 29, 2020 •

edited by cecton

Loading

siku2 commented Oct 30, 2020 •

edited

Loading

siku2 commented Nov 7, 2020 •

edited

Loading

siku2 commented Nov 22, 2020 •

edited

Loading

siku2 commented Jan 23, 2021 •

edited

Loading