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

Fix key parse bug for components #1338

Merged
merged 1 commit into from
Jun 21, 2020
Merged

Fix key parse bug for components #1338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
156 changes: 54 additions & 102 deletions yew-macro/src/html_tree/html_component.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ impl ToTokens for HtmlComponent {
children,
} = self;

let validate_props = if let Props::List(list_props) = props {
let check_props = list_props.props.iter().map(|HtmlProp { label, .. }| {
let validate_props = if let PropType::List(list_props) = &props.prop_type {
let check_props = list_props.iter().map(|HtmlProp { label, .. }| {
quote! { props.#label; }
});

Expand Down Expand Up @@ -116,9 +116,9 @@ impl ToTokens for HtmlComponent {
quote! {}
};

let init_props = match props {
Props::List(list_props) => {
let set_props = list_props.props.iter().map(|HtmlProp { label, value }| {
let init_props = match &props.prop_type {
PropType::List(list_props) => {
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
Expand All @@ -133,24 +133,23 @@ impl ToTokens for HtmlComponent {
.build()
}
}
Props::With(with_props) => {
let props = &with_props.props;
PropType::With(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}
Expand Down Expand Up @@ -336,70 +335,39 @@ impl ToTokens for HtmlComponentClose {
}
}

enum Props {
List(Box<ListProps>),
With(Box<WithProps>),
enum PropType {
List(Vec<HtmlProp>),
With(Ident),
None,
}

struct ListProps {
props: Vec<HtmlProp>,
struct Props {
node_ref: Option<Expr>,
key: Option<Expr>,
prop_type: PropType,
}

struct WithProps {
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)."
}
}
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).";

impl Parse for Props {
fn parse(input: ParseStream) -> ParseResult<Self> {
let mut props = Props::None;
let mut node_ref: Option<Expr> = None;
let mut key: Option<Expr> = None;

let mut props = Props {
node_ref: None,
key: None,
prop_type: PropType::None,
};
while let Some((token, _)) = input.cursor().ident() {
if token == "with" {
match props {
Props::None => Ok(()),
Props::With(_) => Err(input.error("too many `with` tokens used")),
Props::List(_) => {
Err(syn::Error::new_spanned(&token, Props::collision_message()))
}
match props.prop_type {
PropType::None => Ok(()),
PropType::With(_) => Err(input.error("too many `with` tokens used")),
PropType::List(_) => Err(syn::Error::new_spanned(&token, COLLISION_MSG)),
}?;

input.parse::<Ident>()?;
props = Props::With(Box::new(WithProps {
props: input.parse::<Ident>().map_err(|_| {
syn::Error::new_spanned(&token, "`with` must be followed by an identifier")
})?,
node_ref: None,
key: None,
}));
props.prop_type = PropType::With(input.parse::<Ident>().map_err(|_| {
syn::Error::new_spanned(&token, "`with` must be followed by an identifier")
})?);

// Handle optional comma
let _ = input.parse::<Token![,]>();
Expand All @@ -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;
}

Expand All @@ -438,50 +406,34 @@ impl Parse for Props {
return Err(syn::Error::new_spanned(&prop.label, "expected identifier"));
}

match props {
ref mut props @ Props::None => {
*props = Props::List(Box::new(ListProps {
props: vec![prop],
node_ref: None,
key: None,
}));
}
Props::With(_) => {
return Err(syn::Error::new_spanned(&token, Props::collision_message()))
match props.prop_type {
ref mut prop_type @ PropType::None => {
*prop_type = PropType::List(vec![prop]);
}
Props::List(ref mut list) => {
list.props.push(prop);
PropType::With(_) => return Err(syn::Error::new_spanned(&token, COLLISION_MSG)),
PropType::List(ref mut list) => {
list.push(prop);
}
};
}

match props {
Props::None => {}
Props::With(ref mut p) => {
p.node_ref = node_ref;
p.key = key
}
Props::List(ref mut p) => {
p.node_ref = node_ref;
p.key = key;

// sort alphabetically
p.props.sort_by(|a, b| {
if a.label == b.label {
Ordering::Equal
} 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()
}
});
}
};
if let PropType::List(list) = &mut props.prop_type {
// sort alphabetically
list.sort_by(|a, b| {
if a.label == b.label {
Ordering::Equal
} 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)
}
Expand Down
47 changes: 42 additions & 5 deletions yew/src/virtual_dom/vcomp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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,
Expand All @@ -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("");

Expand Down