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.

Sign up for GitHub

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

Jump to bottom

Remove special handing of struct fields of type Option in derive(Properties) #3398

Merged
merged 6 commits into from
Sep 23, 2023
Merged

Remove special handing of struct fields of type Option in derive(Properties) #3398

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8cc02a8
removed PropAttr::Option
its-the-shrimp Sep 9, 2023
247a207
fixed formatting
its-the-shrimp Sep 9, 2023
7997d85
removed an irrelevant test
its-the-shrimp Sep 9, 2023
471f259
added a test for converting value into Some(value) in the html! macro
its-the-shrimp Sep 9, 2023
1771ce8
added more tests for derive(Properties)
its-the-shrimp Sep 12, 2023
4de7160
added more tests (again)
its-the-shrimp Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 4 additions & 94 deletions packages/yew-macro/src/derive_props/field.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@ use proc_macro2::{Ident, Span};
use quote::{format_ident, quote, quote_spanned};
use syn::parse::Result;
use syn::spanned::Spanned;
use syn::{
parse_quote, Attribute, Error, Expr, Field, GenericParam, Generics, Path, Type, TypePath,
Visibility,
};
use syn::{parse_quote, Attribute, Error, Expr, Field, GenericParam, Generics, Type, Visibility};

use super::should_preserve_attr;
use crate::derive_props::generics::push_type_param;
Expand All @@ -17,7 +14,6 @@ use crate::derive_props::generics::push_type_param;
#[derive(PartialEq, Eq)]
enum PropAttr {
Required { wrapped_name: Ident },
Option,
PropOr(Expr),
PropOrElse(Expr),
PropOrDefault,
Expand Down Expand Up @@ -82,11 +78,6 @@ impl PropField {
#name: ::std::option::Option::unwrap(this.wrapped.#wrapped_name),
}
}
PropAttr::Option => {
quote! {
#name: this.wrapped.#name,
}
}
PropAttr::PropOr(value) => {
quote_spanned! {value.span()=>
#name: ::std::option::Option::unwrap_or(this.wrapped.#name, #value),
Expand Down Expand Up @@ -115,19 +106,9 @@ impl PropField {
let ty = &self.ty;
let extra_attrs = &self.extra_attrs;
let wrapped_name = self.wrapped_name();
match &self.attr {
PropAttr::Option => {
quote! {
#( #extra_attrs )*
#wrapped_name: #ty,
}
}
_ => {
quote! {
#( #extra_attrs )*
#wrapped_name: ::std::option::Option<#ty>,
}
}
quote! {
#( #extra_attrs )*
#wrapped_name: ::std::option::Option<#ty>,
}
}

Expand Down Expand Up @@ -164,19 +145,6 @@ impl PropField {
}
}
}
PropAttr::Option => {
quote! {
#[doc(hidden)]
#vis fn #name<#token_ty>(
&mut self,
token: #token_ty,
value: impl ::yew::html::IntoPropValue<#ty>,
) -> #token_ty {
self.wrapped.#name = value.into_prop_value();
token
}
}
}
_ => {
quote! {
#[doc(hidden)]
Expand Down Expand Up @@ -216,12 +184,6 @@ impl PropField {
} else {
unreachable!()
}
} else if matches!(
&named_field.ty,
Type::Path(TypePath { path, .. })
if is_path_an_option(path)
) {
Ok(PropAttr::Option)
} else {
let ident = named_field.ident.as_ref().unwrap();
let wrapped_name = format_ident!("{}_wrapper", ident, span = Span::mixed_site());
Expand Down Expand Up @@ -294,36 +256,6 @@ impl<'a> PropFieldCheck<'a> {
}
}

fn is_path_segments_an_option(path_segments: impl Iterator<Item = String>) -> bool {
fn is_option_path_seg(seg_index: usize, path: &str) -> u8 {
match (seg_index, path) {
(0, "core") => 0b001,
(0, "std") => 0b001,
(0, "Option") => 0b111,
(1, "option") => 0b010,
(2, "Option") => 0b100,
_ => 0,
}
}

path_segments
.enumerate()
.fold(0, |flags, (i, ps)| flags | is_option_path_seg(i, &ps))
== 0b111
}

/// Returns true when the [`Path`] seems like an [`Option`] type.
///
/// This function considers the following paths as Options:
/// - core::option::Option
/// - std::option::Option
/// - Option::*
///
/// Users can define their own [`Option`] type and this will return true - this is unavoidable.
fn is_path_an_option(path: &Path) -> bool {
is_path_segments_an_option(path.segments.iter().take(3).map(|ps| ps.ident.to_string()))
}

impl TryFrom<Field> for PropField {
type Error = Error;

Expand Down Expand Up @@ -369,25 +301,3 @@ impl PartialEq for PropField {
self.name == other.name
}
}

#[cfg(test)]
mod tests {
use crate::derive_props::field::is_path_segments_an_option;

#[test]
fn all_std_and_core_option_path_seg_return_true() {
assert!(is_path_segments_an_option(
vec!["core".to_owned(), "option".to_owned(), "Option".to_owned()].into_iter()
));
assert!(is_path_segments_an_option(
vec!["std".to_owned(), "option".to_owned(), "Option".to_owned()].into_iter()
));
assert!(is_path_segments_an_option(
vec!["Option".to_owned()].into_iter()
));
// why OR instead of XOR
assert!(is_path_segments_an_option(
vec!["Option".to_owned(), "Vec".to_owned(), "Option".to_owned()].into_iter()
));
}
}
30 changes: 18 additions & 12 deletions packages/yew-macro/tests/derive_props/pass.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,18 +220,6 @@ mod t11 {
}
}

mod t12 {
#[derive(::std::clone::Clone, ::yew::Properties, ::std::cmp::PartialEq)]
pub struct Props<T: ::std::clone::Clone + ::std::cmp::PartialEq> {
value: ::std::option::Option<T>,
}

fn optional_prop_generics_should_work() {
::yew::props! { Props::<::std::primitive::bool> { } };
::yew::props! { Props::<::std::primitive::bool> { value: true } };
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this test? We should not be removing tests. I would like to have this change not come at the cost of functionality. Please add the test back

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test (or half of it) would go into fail.rs then, as it produces the problem this PR addresses.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I should note that tests being removed all the time is a normal procedure for many developers. Tests can be removed if they do not reflect current reality (i.e. do not pass, in this case because behavior is opposite to what previously was tested for), are covered by other tests (which have been added (will be added per your suggestions) in this PR by the way), or simply poorly written or considered more of a burden for development than the sought for assistance they are supposed to give.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I should note that tests being removed all the time is a normal procedure for many developers. Tests can be removed if they do not reflect current reality (i.e. do not pass, in this case because behavior is opposite to what previously was tested for), are covered by other tests (which have been added (will be added per your suggestions) in this PR by the way), or simply poorly written or considered more of a burden for development than the sought for assistance they are supposed to give.

Absolutely, but in this case, the test can be updated to be passed and covers a very important functionality: generic props. We should have a passing test for that feature

#[deny(non_snake_case, dead_code)]
mod t13 {
#[derive(::std::cmp::PartialEq, ::yew::Properties)]
Expand All @@ -249,7 +237,25 @@ mod raw_field_names {
r#true: u32,
r#pointless_raw_name: u32,
}
}

mod value_into_some_value_in_props {
#[derive(::std::cmp::PartialEq, ::yew::Properties)]
struct Props {
field1: ::std::option::Option<usize>,
#[prop_or_default]
field2: ::std::option::Option<usize>
}

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

#[::yew::function_component]
fn Main() -> ::yew::html::Html {
::yew::html! { <Inner field1=3 field2=5/> }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a case where only the required field is passed? Also, please add an identical test but failing where it doesn't pass the required prop.

}
}

fn main() {}
1 change: 1 addition & 0 deletions packages/yew/src/suspense/component.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ mod feat_csr_ssr {
#[derive(Properties, PartialEq, Debug, Clone)]
pub(crate) struct BaseSuspenseProps {
pub children: Html,
#[prop_or(None)]
pub fallback: Option<Html>,
}

Expand Down
Loading