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

Property fields of type Option<T> are not mandatory #3395

Closed
1 of 3 tasks
horacimacias opened this issue Sep 6, 2023 · 15 comments · Fixed by #3398
Closed
1 of 3 tasks

Property fields of type Option<T> are not mandatory #3395

horacimacias opened this issue Sep 6, 2023 · 15 comments · Fixed by #3398
Labels
A-yew Area: The main yew crate proposal

Comments

@horacimacias
Copy link

Problem
I want to use Option as a property field.
I want to enforce users of my component provide me with that property always, which can be None or Some(something) but must be populated.
According to https://yew.rs/docs/concepts/function-components/properties#derive-macro-field-attributes:

When deriving Properties all fields are required by default.

Steps To Reproduce
Steps to reproduce the behavior:

  1. Write the following code:
#[derive(Properties, PartialEq)]
pub struct Props {
    pub mandatory: Option<String>,
}

#[function_component]
fn HelloWorld(_props: &Props) -> Html {
    html! {}
}

// Then supply the prop
#[function_component]
fn App() -> Html {
    html! {<HelloWorld/>}
}

Expected behavior
Since all fields are mandatory and I'm not passing the mandatory field of property, I expect this code to not compile.
I suggest either the behaviour is changed so Option is also mandatory (as stated in the docs). People not needing this can always use #[prop_or_default]. Otherwise at least update the docs to reflect this special case for Option fields

Environment:

  • Yew version: 0.20
  • Rust version: latest stable

Questionnaire

  • I'm interested in fixing this myself but don't know where to start
  • I would like to fix and I have a solution
  • I don't have time to fix this right now, but maybe later
@horacimacias
Copy link
Author

...if this is confirmed as a bug and somebody points me in the right direction, I'd be happy to contribute with a PR

@futursolo
Copy link
Member

futursolo commented Sep 7, 2023

I feel the current behaviour is more desirable than requiring user to write a prop when the prop is Option<T>.

Currently a prop: Option<T> accepts the following for a prop:

  1. (nothing) -> prop: None

  2. prop={val} -> prop: Some(val)

  3. prop={Some(val)} -> prop: Some(val)

  4. prop={None} -> prop: None

Requiring a prop to present only takes away 1 but not 2, that does not sound right to me.
If we take away both 1 and 2, then it becomes quite inconvenient to force users to write Some(val).

It is not possible to take away both 1 and 2 based on whether #[prop_or_default] is present as 2 is provided via impl IntoPropValue<Option<T>> for T blanket implementation.

I think this is also consistent with the behaviour of HTML Elements, where omitted attributes are treated as null if you call Element.getAttribute("prop").

However, I do agree that the documentation should be adjusted to reflect this behaviour.

@futursolo futursolo added A-yew Area: The main yew crate proposal and removed bug labels Sep 7, 2023
@horacimacias
Copy link
Author

horacimacias commented Sep 7, 2023

thanks.
I'm not sure I understand what you mean by "Requiring a prop to present only takes away 1 but not 2".
Option 1 is not providing the property and (unless prop_or_default is used) according to the documentation all fields are required. So, not taking away 2 is not an issue. Taking away 1 but not 2 is absolutely fine.

Regarding forcing users, whoever develops de component is responsible for making it convenient or not to the component users. There is already a way to "use default if not provided"; prop_or_default.

If I search for code containing #[prop_or_default] with fields being Option, I get several results: https://github.com/search?q=repo%3Ayewstack%2Fyew%20%23%5Bprop_or_default%5D&type=code

(I also found occurrences in my code)
So, my point is, it looks like this behavior of needing properties even if they are Option was already there at some point, otherwise why would we have annotated so many Option fields with #[prop_or_default] when it's not needed?

all in all, I'd suggest we treat all types of fields the same as there already is a documented (and used) way of "using default if not provided".

@horacimacias
Copy link
Author

... and after re-reading your message again I think I see what you meant about option 2 doing the "val -> Some(val)" conversion behind the scenes.
I have no strong opinion there on whether that is good (convenient) or bad (treating a certain type different to others), as the main driver for me is to have a way to have mandatory properties which happen to be of type Option

@horacimacias
Copy link
Author

in case this is of any value, I was curious to find out why was I using #[prop_or_default] in for Options so I tested with yew 0.20, 0.19, 0.18 and 0.17.

On 0.17 this was failing at compile time:

error[E0599]: no method named `build` found for struct `app::PropsBuilder<app::PropsBuilderStep_missing_required_prop_mandatory>` in the current scope
  --> src/app.rs:49:9
   |
3  |   #[derive(Properties, PartialEq, Clone)]
   |            ---------- method `build` not found for this struct
...
49 | /         html! {
50 | |             <Child />
51 | |         }
   | |         ^
   | |_________|
   |           method not found in `PropsBuilder<PropsBuilderStep_missing_required_prop_mandatory>`
   |
   = note: the method was found for
           - `app::PropsBuilder<app::PropsBuilderStep_build>`
   = note: this error originates in the macro `proc_macro_call_0` which comes from the expansion of the macro `html` (in Nightly builds, run with -Z macro-backtrace for more info)

@kirillsemyonkin
Copy link
Contributor

@futursolo I noticed a proposal tag change, I personally do in fact consider it a bug, or at least the fact that it leads to bugs for properties that are supposed to always get an Option value, yet Yew currently allows you to simply forget to pass it.

Requiring a prop to present only takes away 1 but not 2, that does not sound right to me.

It sounds right to us though. This is simply about not providing the prop at all. If an equivalent syntactic prop={}, prop={ () } or prop={ ! } could be made instead for your odd behavior equating "no value typed out in code means none variant", this could be a middle-ground solution. I think not requiring Some is something that Rust should also do, by doing "either T or None::<T>".

I think this is also consistent with the behaviour of HTML Elements, where omitted attributes are treated as null

This is not a good argument. That is JS, this is Rust. null is not Option::None, there are no nulls in Rust. You do not call value_opt.do_something(), you have to explicitly check the value's presence (e.g. value_opt.unwrap().do_something()), and this is one of great things of having a stronger type system compared to whatever JS has. Also, props are not attributes, we consider attributes optional strings of elements, but we consider props actual in-code values of strong types.

@futursolo
Copy link
Member

@futursolo I noticed a proposal tag change, I personally do in fact consider it a bug, or at least the fact that it leads to bugs for properties that are supposed to always get an Option value, yet Yew currently allows you to simply forget to pass it.

I removed the bug label because this behaviour is intentionally introduced, although the documentation didn't reflect this special behaviour for Option<_>. This is not an indication of the current behaviour is better than the one purposed.

This issue is proposing a behaviour different than what this macro is currently trying to do.

a middle-ground solution

I do not think these solutions are better than writing prop={None}, if you have to explicit say something.
They are not much shorter or easier to understand than assigning None.

That is JS, this is Rust.

This behaviour can also be observed in Rust.
web_sys::Element::get_attribute also returns None when the attribute does not present.

You do not call value_opt.do_something(), you have to explicitly check the value's presence (e.g. value_opt.unwrap().do_something()), and this is one of great things of having a stronger type system compared to whatever JS has.

Since there is no compile stage in ordinary JavaScript, you cannot complain about JavaScript does not give you compile time errors. JavaScript is horrible in many ways, but this is not one of them.

instead for your odd behavior equating "no value typed out in code means none variant",

I think this behaviour is consistent with HTML Elements, if you want an attribute to be set, then it is present, if you want it to be null-ish, then do not have it present. You never pass an explicit value to an attribute to represent a null / None / nil-ish value.

If I have to say, I would argue explicit passing None here is the odder behaviour of the 2.

@horacimacias
Copy link
Author

horacimacias commented Sep 7, 2023

If/when I develop a component who's property field is Option<something> and the property is not fundamental to my component (only I as the developer of the component can tell if this option is required or not), I can happily annotate with prop_or_default and my users can pass None or not pass at all (but again, the component developer, not the framework, made that clear and documented choice).

If/when I develop a component who's property field is Option<something> and the property is fundamental to my component, this (bug/feature) is the only way I can force my users to pass that property (even if they pass None, which is just as valid as any other value). (I can also define my own Option-like enum which is less than ideal)

I think we're really mixing passing None with not passing something, and this may be ok in some scenarios but I don't think the framework should be opinionated here (and it already provides prop_or_default in case I want to bypass the 'all fields are mandatory' rule).
If there was a #[mandatory_prop] I could use here I'd use that, as again the component developer can choose what semantics to give to the property fields.

all in all, it does look like a special/arbitrary case when the possibility of transform behavior (prop_or_default) is already there.

The "magic" converting something={value} to something={Some(value)} is different in my opinion; if the property is Option<something> and I pass something, without this "magic" things would not even compile. So, the framework is making something compile which would otherwise not (but it is not introducing a property the developer using the component didn't write).
The "magic" converting not passing a property to passing a something={None}, is assuming the developer intended to pass None, which may or may not be the case.

@kirillsemyonkin
Copy link
Contributor

kirillsemyonkin commented Sep 7, 2023

if you have to explicit say something

You have to. Missing #[prop_or...] means there is no "or", there has to be a prop. You have to type out the name, at least in some way, to signal that you acknowledged that the prop exists. The change was meant to be a nice fix for Some, not a bug for None.

when the attribute does not present

consistent with HTML Elements, if you want an attribute

Attributes, attributes, attributes. I said that props are not attributes. An equivalent for attributes would be a #[prop_or_default], since the agreed-upon behavior for attributes missing is None; this is not the case for the usual properly-typed props. Proper types are the major distinguishing feature of props in a Rust framework.

there is no compile stage in ordinary JavaScript

I don't think I mentioned compile-time? I said it was about types, and types are more-or-less checked in JS at runtime. I did in fact use Option in JS for my projects, and the proposal for JS pattern matching does mention using an Option equivalent.

@ranile
Copy link
Member

ranile commented Sep 8, 2023

I agree this is a surprising behavior. I don't remember why this change was made but if you can maintain all the current functionality and also make passing None a requirement, unless specified otherwise, I'd be happy to accept such PRs.

@futursolo what do you think of that? I don't see how implicitly assigning None is a benefit when it can be explicit or the component author can opt into making it implicit. I think making the fact that None is passed to a property explicit and clear to the user is a good thing.

@horacimacias
Copy link
Author

thanks. I'll give it a try; if anybody is already doing this or has some hints on where to look at please let me know to avoid reinventing the wheel.

@kirillsemyonkin
Copy link
Contributor

@horacimacias

Required { wrapped_name: Ident },
Option,
i think related to this, they have a separate thing for Option

@its-the-shrimp
Copy link
Contributor

its-the-shrimp commented Sep 9, 2023

As far as I remember, Yew doesn't guarantee backwards compatibility until version 1.0, so we could just get rid of the special case, or we can add a #[required] attribute to the derive(Properties) macro, which would override the special treatment of the Option type when applied to a field of type Option, which approach would be better? I personally think it's better to take advantage of the lack of backwards compatibility guarantees while we can rather than complicate the codebase, and if others agree, I'll get to implementing it right away.

@kirillsemyonkin
Copy link
Contributor

@schvv31n I'm certain that adding #[required] would not only make you forget to use the prop whenever the attribute is missing, you would actually also forget to write #[required] in your component's code.

The whole Rust language is built upon making strict limitations to ensure bug-free experience, and only then opening it up bit by bit once the unrestricting logic can be figured out - see Non-Lexical Lifetimes. This #[required] situation would be akin to mut vs const, there it is wanted that users opt-in to mutability with the mut, rather than opting-out of it with a const. In our case the None path was opened because of a somewhat hasty attempt to unrestrict Some, which violates this philosophy of minimal opening that I observed. I consider this a bug (an undocumented change - I can consider this situation undefined behavior of sorts), thus no backwards compatibility should be preserved.

If you get rid of special case, try ensuring that Some::<T> still can be pulled from T. I'm not even certain I ever used it, but it does seem a reasonable QoL feature.

@horacimacias
Copy link
Author

while #[required] might help my specific use case (property fields of type Option be mandatory), I think it's adding a corner case of a corner case. Unless there is a technical reason for this, I'd stick to the existing prop_or_* annotations.

Comparing:

  1. all property fields are mandatory, except if prop_or_* is used.
  2. all property fields are mandatory, except if prop_or_* is used, except if field is Option which is then NOT mandatory and can be made so by using #[required]

I'd say option 1 is clear, elegant, concise. Good. option 2 sounds like a clumsy workarround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate proposal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants