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

Add html_nested! macro to support nested iterable children access #843

Merged
merged 11 commits into from
Jan 6, 2020
Merged

Add html_nested! macro to support nested iterable children access #843

merged 11 commits into from
Jan 6, 2020

Conversation

trivigy
Copy link
Contributor

@trivigy trivigy commented Jan 4, 2020

@trivigy
Copy link
Contributor Author

trivigy commented Jan 5, 2020

@jstarry This work is complete.

if true {
html! { <Container int=1 /> }
} else {
html! {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks

@jstarry jstarry changed the title Re-introduces changes from #820 along with #835 and fix #839 Add html_nested! macro to support nested iterable children access Jan 6, 2020
@trivigy
Copy link
Contributor Author

trivigy commented Jan 6, 2020

@jstarry are you sure that you want to remove the html! rewriting? The reason why I did that was the following:

If a developer is trying to write code like this:

fn view(&self) -> Html {
    return html! {
        <Table>
            <TableHead>
                <TableRow>
                    <TableCell>{ "ID" }</TableCell>
                    <TableCell>{ "First Name" }</TableCell>
                    <TableCell>{ "Last Name" }</TableCell>
                    <TableCell>{ "Email" }</TableCell>
                    <TableCell>{ "Gender" }</TableCell>
                    <TableCell>{ "IP Address" }</TableCell>
                </TableRow>
            </TableHead>
            <TableBody>
                {
                    self.props
                        .data_source
                        .into_iter()
                        .map(|row: &SampleRecord| {
                            html_nested! {
                                <TableRow>
                                    <TableCell>{ &row.id }</TableCell>
                                    <TableCell>{ &row.first_name }</TableCell>
                                    <TableCell>{ &row.last_name }</TableCell>
                                    <TableCell>{ &row.email }</TableCell>
                                    <TableCell>{ &row.gender }</TableCell>
                                    <TableCell>{ &row.ip_address }</TableCell>
                                </TableRow>
                            }
                        })
                        .collect::<Vec<VChild<TableRow>>>()
                }
            </TableBody>
        </Table>
    };
}

This forces the developer to remember that html_nested needs to be used. Makes yew harder to use.

@trivigy
Copy link
Contributor Author

trivigy commented Jan 6, 2020

Otherwise maybe we could call it something like: html_vnode_unwrap or html_unwrap.

@jstarry
Copy link
Member

jstarry commented Jan 6, 2020

I don't think we can determine implicitly when html! should be transformed to html_nested! so it should be explicit. I'm open to other names but so far I think html_nested! is the best option

@trivigy
Copy link
Contributor Author

trivigy commented Jan 6, 2020

Just tested this and it is breaking my code now. Getting:

error[E0277]: `std::vec::Vec<yew::virtual_dom::vcomp::VComp>` doesn't implement `std::fmt::Display`
  --> src/components/table_body.rs:80:21
   |
80 | /                     self.props
81 | |                         .children
82 | |                         .iter()
83 | |                         .filter(|c| match &c.props {
...  |
92 | |                         .map(|c| c.into())
93 | |                         .collect::<Vec<VComp>>()
   | |________________________________________________^ `std::vec::Vec<yew::virtual_dom::vcomp::VComp>` cannot be formatted with the default formatter
   |
   = help: the trait `std::fmt::Display` is not implemented for `std::vec::Vec<yew::virtual_dom::vcomp::VComp>`
   = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
   = note: required because of the requirements on the impl of `std::string::ToString` for `std::vec::Vec<yew::virtual_dom::vcomp::VComp>`
   = note: required because of the requirements on the impl of `std::convert::From<std::vec::Vec<yew::virtual_dom::vcomp::VComp>>` for `yew::virtual_dom::vnode::VNode`
   = note: required by `std::convert::From::from`

For:

return html! {
    <>
        {
            self.props
                .children
                .iter()
                .filter(|c| match &c.props {
                    Variants::Row(_) => true,
                })
                .map(|mut c| {
                    if let Variants::Row(ref mut props) = c.props {
                        props.context = Some(TableContext::Body);
                    }
                    c
                })
                .map(|c| c.into())
                .collect::<Vec<VComp>>()
        }
    </>
};

Here is the expansion:

return {
    enum ProcMacroHack {
        Nested = ( "< >\n{\n    self . props . children . iter () . filter\n    (| c | match & c . props { Variants :: Row (_) => true, }) . map\n    (| mut c |\n     {\n         if let Variants :: Row (ref mut props) = c . props\n         { props . context = Some (TableContext :: Body) ; } c\n     }) . map (| c | c . into ()) . collect :: < Vec < VComp >> ()\n} < / >" , 0 ) . 1 , }
    ::yew::virtual_dom::VNode::VList(
        ::yew::virtual_dom::vlist::VList::new_with_children({
            let mut v = ::std::vec::Vec::new();
            v.extend(::yew::utils::NodeSeq::from(
                ::yew::virtual_dom::VNode::from({
                    self.props
                        .children
                        .iter()
                        .filter(|c| match &c.props {
                            Variants::Row(_) => true,
                        })
                        .map(|mut c| {
                            if let Variants::Row(ref mut props) = c.props {
                                props.context = Some(TableContext::Body);
                            }
                            c
                        })
                        .map(|c| c.into())
                        .collect::<Vec<VComp>>()
                }),
            ));
            v
        }),
    )
};

The expansion before the wip commit did:

return {
    enum ProcMacroHack {
        Nested = ( "< >\n{\n    self . props . children . iter () . filter\n    (| c | match & c . props { Variants :: Row (_) => true, }) . map\n    (| mut c |\n     {\n         if let Variants :: Row (ref mut props) = c . props\n         { props . context = Some (TableContext :: Body) ; } c\n     }) . map (| c | c . into ()) . collect :: < Vec < VComp >> ()\n} < / >" , 0 ) . 1 , }
    ::yew::virtual_dom::VNode::from(::yew::virtual_dom::VNode::VList(
        ::yew::virtual_dom::vlist::VList::new_with_children({
            let mut v = ::std::vec::Vec::new();
            let comps = ({
                self.props
                    .children
                    .iter()
                    .filter(|c| match &c.props {
                        Variants::Row(_) => true,
                    })
                    .map(|mut c| {
                        if let Variants::Row(ref mut props) = c.props {
                            props.context = Some(TableContext::Body);
                        }
                        c
                    })
                    .map(|c| c.into())
                    .collect::<Vec<VComp>>()
            },);
            ::yew::utils::NodeSeq::from(comps.0)
                .into_iter()
                .for_each(|x| v.push(x.into()));
            v
        }),
    ))
};

The reason this works and the other doesn't is because a tuple can hold multiple different types whereas a vector cannot. And the type coercion was happening when the tuple was iterated over and unloaded. This way it is impossible.

@jstarry
Copy link
Member

jstarry commented Jan 6, 2020

@trivigy the reason your example code breaks is because I added back ::yew::virtual_dom::VNode::from(..)

Why do you have .collect::<Vec<VComp>>()?

@trivigy
Copy link
Contributor Author

trivigy commented Jan 6, 2020

@jstarry Consider the following example:

There is collection into three different types. All need to be handled in such way so that the underlying table can have access to their props.

return html! {
    <Table>
        {
            self.props
                .children
                .iter()
                .filter(|c| match c.props {
                    Variants::Head(_) => true,
                    _ => false,
                })
                .map(|c| c.into())
                .collect::<Vec<VChild<TableHead>>>()
        }
        {
            self.props
                .children
                .iter()
                .filter(|c| match c.props {
                    Variants::Body(_) => true,
                    _ => false,
                })
                .map(|c| c.into())
                .collect::<Vec<VChild<TableBody>>>()
        }
        {
            self.props
                .children
                .iter()
                .filter(|c| match c.props {
                    Variants::Foot(_) => true,
                    _ => false,
                })
                .map(|c| c.into())
                .collect::<Vec<VChild<TableFoot>>>()
        }
    </Table>
};

Statically this would work absolutely fine.

return html! {
    <Table>
        <TableHead></TableHead>
        <TableHead></TableHead>
        <TableBody></TableBody>
        <TableBody></TableBody>
        <TableFoot></TableFoot>
        <TableFoot></TableFoot>
    </Table>
};

@jstarry
Copy link
Member

jstarry commented Jan 6, 2020

@trivigy what's the error?

@trivigy
Copy link
Contributor Author

trivigy commented Jan 6, 2020

Why do you have .collect::<Vec>()?

Because for right now I have this:

return html! {
    <>
        {
            self.props
                .children
                .iter()
                .filter(|c| match c.props {
                    Variants::Head(_) => true,
                    _ => false,
                })
                .map(|c| c.into())
                .collect::<Vec<VComp>>()
        }
        {
            self.props
                .children
                .iter()
                .filter(|c| match c.props {
                    Variants::Body(_) => true,
                    _ => false,
                })
                .map(|c| c.into())
                .collect::<Vec<VComp>>()
        }
        {
            self.props
                .children
                .iter()
                .filter(|c| match c.props {
                    Variants::Foot(_) => true,
                    _ => false,
                })
                .map(|c| c.into())
                .collect::<Vec<VComp>>()
        }
    </>
};

With this:

impl Into<VComp> for Variant {
    fn into(self) -> VComp {
        match self.props {
            Variants::Head(props) => VComp::new::<table_head::TableHead>(props, NodeRef::default()),
            Variants::Body(props) => VComp::new::<table_body::TableBody>(props, NodeRef::default()),
            Variants::Foot(props) => VComp::new::<table_foot::TableFoot>(props, NodeRef::default()),
            Variants::Row(props) => VComp::new::<table_row::TableRow>(props, NodeRef::default()),
        }
    }
}

Because I could not implement:

impl Into<VChild<TableHead>> for Variant {
    fn into(self) -> VChild<TableHead> {
        match self.props {
            Variants::Head(props) => VChild::<table_head::TableHead>::new(props, NodeRef::default()),
        }
    }
}

The whole vdom is quite a mess and I could not figure out how to make it do what I was slowly actually headed towards.

@jstarry jstarry merged commit f18eca1 into yewstack:master Jan 6, 2020
llebout pushed a commit to llebout/yew that referenced this pull request Jan 20, 2020
…ewstack#843)

* Revert "Revert "Improve nested html! expansion by unwrapping VNodes (yewstack#820)" (yewstack#842)"

This reverts commit 70862a4

* Add minor fix to VList to conform with yewstack#820

* Add unittest for the regression bug yewstack#839

* Resolve yewstack#839 regression issue introduced by yewstack#820

* wip

* Revert "Resolve yewstack#839 regression issue introduced by yewstack#820"

This reverts commit 34dc935.

* Use explicit html_nested

* Remove NodeSeq from prelude

* Clean up tests

* Add list and tag tests

* Revert vtag children change

Co-authored-by: Justin Starry <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants