Fix key parse bug for components (#1338)

yew-macro/src/html_tree/html_component.rs

@@ -87,8 +87,8 @@ impl ToTokens for HtmlComponent {
             children,
         } = self;
 
-        let validate_props = if let Props::List(list_props) = props {
+        let validate_props = if let PropType::List(list_props) = &props.prop_type {
-            let check_props = list_props.props.iter().map(|HtmlProp { label, .. }| {
+            let check_props = list_props.iter().map(|HtmlProp { label, .. }| {
                 quote! { props.#label; }
             });
 
@@ -116,9 +116,9 @@ impl ToTokens for HtmlComponent {
             quote! {}
         };
 
-        let init_props = match props {
+        let init_props = match &props.prop_type {
-            Props::List(list_props) => {
+            PropType::List(list_props) => {
-                let set_props = list_props.props.iter().map(|HtmlProp { label, value }| {
+                let set_props = list_props.iter().map(|HtmlProp { label, value }| {
                     quote_spanned! { value.span()=> .#label(
                         <::yew::virtual_dom::VComp as ::yew::virtual_dom::Transformer<_, _>>::transform(
                             #value
@@ -133,24 +133,23 @@ impl ToTokens for HtmlComponent {
                         .build()
                 }
             }
-            Props::With(with_props) => {
+            PropType::With(props) => {
-                let props = &with_props.props;
                 quote! { #props }
             }
-            Props::None => quote! {
+            PropType::None => quote! {
                 <<#ty as ::yew::html::Component>::Properties as ::yew::html::Properties>::builder()
                     #set_children
                     .build()
             },
         };
 
-        let node_ref = if let Some(node_ref) = props.node_ref() {
+        let node_ref = if let Some(node_ref) = &props.node_ref {
             quote_spanned! { node_ref.span()=> #node_ref }
         } else {
             quote! { ::yew::html::NodeRef::default() }
         };
 
-        let key = if let Some(key) = props.key() {
+        let key = if let Some(key) = &props.key {
             quote_spanned! { key.span()=> Some(#key) }
         } else {
             quote! {None}
@@ -336,70 +335,39 @@ impl ToTokens for HtmlComponentClose {
     }
 }
 
-enum Props {
+enum PropType {
-    List(Box<ListProps>),
+    List(Vec<HtmlProp>),
-    With(Box<WithProps>),
+    With(Ident),
     None,
 }
 
-struct ListProps {
+struct Props {
-    props: Vec<HtmlProp>,
     node_ref: Option<Expr>,
     key: Option<Expr>,
+    prop_type: PropType,
 }
 
-struct WithProps {
+const COLLISION_MSG: &str = "Using the `with props` syntax in combination with named props is not allowed (note: this does not apply to the `ref` prop).";
-    props: Ident,
-    node_ref: Option<Expr>,
-    key: Option<Expr>,
-}
-
-impl Props {
-    fn node_ref(&self) -> Option<&Expr> {
-        match self {
-            Props::List(list_props) => list_props.node_ref.as_ref(),
-            Props::With(with_props) => with_props.node_ref.as_ref(),
-            Props::None => None,
-        }
-    }
-
-    fn key(&self) -> Option<&Expr> {
-        match self {
-            Props::List(list_props) => list_props.key.as_ref(),
-            Props::With(with_props) => with_props.key.as_ref(),
-            Props::None => None,
-        }
-    }
-
-    fn collision_message() -> &'static str {
-        "Using the `with props` syntax in combination with named props is not allowed (note: this does not apply to the `ref` prop)."
-    }
-}
 
 impl Parse for Props {
     fn parse(input: ParseStream) -> ParseResult<Self> {
-        let mut props = Props::None;
+        let mut props = Props {
-        let mut node_ref: Option<Expr> = None;
+            node_ref: None,
-        let mut key: Option<Expr> = None;
+            key: None,
-
+            prop_type: PropType::None,
+        };
         while let Some((token, _)) = input.cursor().ident() {
             if token == "with" {
-                match props {
+                match props.prop_type {
-                    Props::None => Ok(()),
+                    PropType::None => Ok(()),
-                    Props::With(_) => Err(input.error("too many `with` tokens used")),
+                    PropType::With(_) => Err(input.error("too many `with` tokens used")),
-                    Props::List(_) => {
+                    PropType::List(_) => Err(syn::Error::new_spanned(&token, COLLISION_MSG)),
-                        Err(syn::Error::new_spanned(&token, Props::collision_message()))
-                    }
                 }?;
 
                 input.parse::<Ident>()?;
-                props = Props::With(Box::new(WithProps {
+                props.prop_type = PropType::With(input.parse::<Ident>().map_err(|_| {
-                    props: input.parse::<Ident>().map_err(|_| {
+                    syn::Error::new_spanned(&token, "`with` must be followed by an identifier")
-                        syn::Error::new_spanned(&token, "`with` must be followed by an identifier")
+                })?);
-                    })?,
-                    node_ref: None,
-                    key: None,
-                }));
 
                 // Handle optional comma
                 let _ = input.parse::<Token![,]>();
@@ -412,21 +380,21 @@ impl Parse for Props {
 
             let prop = input.parse::<HtmlProp>()?;
             if prop.label.to_string() == "ref" {
-                match node_ref {
+                match props.node_ref {
                     None => Ok(()),
                     Some(_) => Err(syn::Error::new_spanned(&prop.label, "too many refs set")),
                 }?;
 
-                node_ref = Some(prop.value);
+                props.node_ref = Some(prop.value);
                 continue;
             }
             if prop.label.to_string() == "key" {
-                match key {
+                match props.key {
                     None => Ok(()),
                     Some(_) => Err(syn::Error::new_spanned(&prop.label, "too many keys set")),
                 }?;
 
-                key = Some(prop.value);
+                props.key = Some(prop.value);
                 continue;
             }
 
@@ -438,50 +406,34 @@ impl Parse for Props {
                 return Err(syn::Error::new_spanned(&prop.label, "expected identifier"));
             }
 
-            match props {
+            match props.prop_type {
-                ref mut props @ Props::None => {
+                ref mut prop_type @ PropType::None => {
-                    *props = Props::List(Box::new(ListProps {
+                    *prop_type = PropType::List(vec![prop]);
-                        props: vec![prop],
-                        node_ref: None,
-                        key: None,
-                    }));
                 }
-                Props::With(_) => {
+                PropType::With(_) => return Err(syn::Error::new_spanned(&token, COLLISION_MSG)),
-                    return Err(syn::Error::new_spanned(&token, Props::collision_message()))
+                PropType::List(ref mut list) => {
-                }
+                    list.push(prop);
-                Props::List(ref mut list) => {
-                    list.props.push(prop);
                 }
             };
         }
 
-        match props {
+        if let PropType::List(list) = &mut props.prop_type {
-            Props::None => {}
+            // sort alphabetically
-            Props::With(ref mut p) => {
+            list.sort_by(|a, b| {
-                p.node_ref = node_ref;
+                if a.label == b.label {
-                p.key = key
+                    Ordering::Equal
-            }
+                } else if a.label.to_string() == "children" {
-            Props::List(ref mut p) => {
+                    Ordering::Greater
-                p.node_ref = node_ref;
+                } else if b.label.to_string() == "children" {
-                p.key = key;
+                    Ordering::Less
-
+                } else {
-                // sort alphabetically
+                    a.label
-                p.props.sort_by(|a, b| {
+                        .to_string()
-                    if a.label == b.label {
+                        .partial_cmp(&b.label.to_string())
-                        Ordering::Equal
+                        .unwrap()
-                    } else if a.label.to_string() == "children" {
+                }
-                        Ordering::Greater
+            });
-                    } else if b.label.to_string() == "children" {
+        }
-                        Ordering::Less
-                    } else {
-                        a.label
-                            .to_string()
-                            .partial_cmp(&b.label.to_string())
-                            .unwrap()
-                    }
-                });
-            }
-        };
 
         Ok(props)
     }

yew/src/virtual_dom/vcomp.rs

@@ -279,7 +279,7 @@ impl<COMP: Component> fmt::Debug for VChild<COMP> {
 
 #[cfg(test)]
 mod tests {
-    use super::VChild;
+    use super::*;
     use crate::macros::Properties;
     use crate::{html, Children, Component, ComponentLink, Html, NodeRef, ShouldRender};
     #[cfg(feature = "wasm_test")]
@@ -347,6 +347,47 @@ mod tests {
         };
     }
 
+    #[test]
+    fn set_component_key() {
+        let test_key = "test".to_string();
+        let check_key = |vnode: VNode| {
+            assert_eq!(vnode.key().as_ref(), Some(&test_key));
+        };
+
+        let props = Props {
+            field_1: 1,
+            field_2: 1,
+        };
+        let props_2 = props.clone();
+
+        check_key(html! { <Comp key=test_key.clone() /> });
+        check_key(html! { <Comp key=test_key.clone() field_1=1 /> });
+        check_key(html! { <Comp field_1=1 key=test_key.clone() /> });
+        check_key(html! { <Comp with props key=test_key.clone() /> });
+        check_key(html! { <Comp key=test_key.clone() with props_2 /> });
+    }
+
+    #[test]
+    fn set_component_node_ref() {
+        let test_node: Node = document().create_text_node("test").into();
+        let test_node_ref = NodeRef::new(test_node);
+        let check_node_ref = |vnode: VNode| {
+            assert_eq!(vnode.first_node(), test_node_ref.get().unwrap());
+        };
+
+        let props = Props {
+            field_1: 1,
+            field_2: 1,
+        };
+        let props_2 = props.clone();
+
+        check_node_ref(html! { <Comp ref=test_node_ref.clone() /> });
+        check_node_ref(html! { <Comp ref=test_node_ref.clone() field_1=1 /> });
+        check_node_ref(html! { <Comp field_1=1 ref=test_node_ref.clone() /> });
+        check_node_ref(html! { <Comp with props ref=test_node_ref.clone() /> });
+        check_node_ref(html! { <Comp ref=test_node_ref.clone() with props_2 /> });
+    }
+
     #[test]
     fn vchild_partialeq() {
         let vchild1: VChild<Comp> = VChild::new(
@@ -412,8 +453,6 @@ mod tests {
 
     #[cfg(feature = "web_sys")]
     fn setup_parent() -> (AnyScope, Element) {
-        use crate::utils::document;
-
         let scope = AnyScope {
             type_id: std::any::TypeId::of::<()>(),
             parent: None,
@@ -428,8 +467,6 @@ mod tests {
 
     #[cfg(feature = "web_sys")]
     fn get_html(mut node: Html, scope: &AnyScope, parent: &Element) -> String {
-        use super::VDiff;
-
         // clear parent
         parent.set_inner_html("");