Open Boscop opened 4 years ago
Yeah not super ideal, the hope is that devs notice this part: PropsBuilderStep_missing_required_prop_allow_zoom
.
If anyone comes up with a better approach, I'd be very happy!
So I've thought about this, and have created a possible alternative to the current system. It would work by creating a macro for each property, that is then used by the html!
macro.
A simplified example looks like this:
struct Foo;
struct Testing {
f: Option<i32>,
b: Foo,
}
macro_rules! CreateTesting {
(@match_b b: $expr:expr , $($rest:tt)*) => {{
$expr
}};
(@match_b $first:ident : $expr:expr , $($rest:tt)*) => {
CreateTesting! { @match_b $($rest)* }
};
(@match_b) => {
compile_error!("Field 'b' is required");
};
(@match_f f: $expr:expr , $($rest:tt)*) => {
$expr
};
(@match_f $first:ident : $expr:expr , $($rest:tt)*) => {
CreateTesting! { @match_f $($rest)* }
};
(@match_f) => {
Default::default()
};
($($fields:tt)*) => {{
let f = CreateTesting! { @match_f $($fields)* };
let b = CreateTesting! { @match_b $($fields)* };
Testing {
f, b
}
}};
}
fn main() {
let t : Testing = CreateTesting! { f: None, b: Foo, };
}
This can be a bit odd in non-2018 editions, but in 2018 edition crates, macros can be imported and don't pollute the global name space.
It works by recursing through all the fields, and trying to match the 'correct' one. If it can't find it, it will output either the default (which doesn't have to be Default::default
at the end) or it will output a specific compiler_error
which will tell the user exactly what was missing. This will also tell them all the missing props from the get-go, so it has nice UX already. If it finds the field it just uses the expression.
If this sounds promising I'd go and implement a proof of concept rewrite @jstarry
Edit: I've looked deeper into this, and it's sadly not that easy to add as I hoped it would be. Since macros are hygienic, but proc-macros are not this creates very odd situations. Additionally, you can't associate a macro to a struct, so one needs to have more workarounds in total. Maybe a more ergonomic way to implement this comes to my mind later.
This looks promising! The answer is always more macros đ
I've looked deeper into this, and it's sadly not that easy to add as I hoped it would be. Since macros are hygienic, but proc-macros are not this creates very odd situations. Additionally, you can't associate a macro to a struct, so one needs to have more workarounds in total. Maybe a more ergonomic way to implement this comes to my mind later.
Can you elaborate a bit more on the hygiene issues and what you mean by "can't associate a macro to a struct"?
Thereâs multiple issues:
Iâm trying to think of ways to circumvent these issues. But macros are annoying to debug ^^â
All in all, this is highlighting a hole in Rusts features, which is partially default structs. Because in that case it would be trivial to implement.
Thanks for the context, makes sense. Might be worth hacking it out because macros would give us more flexibility.
All in all, this is highlighting a hole in Rusts features, which is partially default structs. Because in that case it would be trivial to implement.
I came across the "Init struct pattern" on r/rust
recently and I think it should work actually.
If we call YakShaverInit
-> OptionalProps
...
impl Default for OptionalProps
is trivial using the #[prop_or...]
attributesOptionalProps
can then be created using passed props and filled with ..Default::default()
OptionalProps::init
method can be generated from the passed required props and the props from OptionalProps
Am I missing anything? I think the compiler error messages will be better too because Rust would just complain that a certain field is not set (i.e. missing required prop) which is more intuitive.
EDIT: There may be some trickiness creating OptionalProps
because it would need to be associated with the Properties
trait
That was my initial try, but the issue is creating the actual final version. The html! Macro itself doesnât know which fields are optional or not. So it wonât know what goes into OptionalProps or not. And you will also need to create the actual Props struct, but that can only be done with the struct constructor, which requires all fields front the get go. And you can only interpolate structs from the same type.
One thing that would work is Mark the optional fields as âoverridesâ or so. By for example prepending with an â@â, this way one could make it work. But I tried to make it work without it.
Ah, that's right. Bummer. I guess the state builder approach lives another day.
What do you think of a syntax that looks like this:
html! {
<my::Model req_prop=123 @optional_prop=123 />
}
It would solve the big issue of "which" field is missing. The only downside I see is that going from required to optional is a breaking change, whereas right now it is not. To me that doesn't sound like a big price to pay for nicer errors.
I'm open to it, but yeah I would have the same concern over the pain of switching properties from required to optional and vice versa. We would need nice error messages for that đ
marking as blocked because there's no known alternative approach that makes this better without a syntax change to html!
This is the workaround I use, It feels a bit weird though...
#[derive(Clone, PartialEq)]
pub struct GridPaginationBarProps {
pub page: i32,
pub page_size: i32,
pub total_rows: usize
}
#[derive(Properties, Clone, PartialEq)]
pub struct Props {
pub props: GridPaginationBarProps
}
#[function_component(GridPaginationBar)]
pub fn grid_pagination_bar(i: &Props) -> Html {}
but then caller has to pass a struct into a single property
<GridPaginationBar props={GridPaginationBarProps{}}/>
fixed
<div class="yew-data-grid-footer-container">
<GridPaginationBar props={GridPaginationBarProps{ page: 1, page_size: 100, total_rows: rows_total}}/>
</div>
Note the error has changed with yew 0.20 and is hopefully a bit more readable now. It should say something along the lines of
error[E0277]: the trait bound `AssertAllProps: HasProp<_GridPaginationBarProperties::page, _>` is not satisfied
--> tests/html_macro/component-fail.rs:115:14
|
115 | html! { <GridPaginationBar /> };
| ^^^^^^^^^^^^^^^^^ the trait `HasProp<_GridPaginationBarProperties::page, _>` is not implemented for `AssertAllProps`
|
While I still think the mentioned names should be explained somewhere in the docs (can't find AssertAllProps
or HasProp
at the moment), at least the name of the missing property is now clearly visible. The work-around mentioned by @ta32 does not support default properties, which are automatically filled in by the macro otherwise. Another possible syntax to write this, if you do not care about default properties, would be using a base expression
#[function_component]
pub fn GridPaginationBar(i: &GridPaginationBarProps) -> Html {}
html! {
<GridPaginationBar ..GridPaginationBarProps{ page: 1, page_size: 100, total_rows: rows_total}/>
}
(I'm using Yew 0.13. https://github.com/yewstack/yew/pull/975, https://github.com/yewstack/yew/issues/928) When the prop
allow_zoom
is missing from these props:I get this error:
Ideally, the error should not mention other props that ARE passed, such as
clicked_pos
. It should only mention the missing prop, but not encoded in code, but in clear text, like