-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Unify and change children logic in the html macro #1275
Conversation
@jstarry Yep, that's already on the TODO list 👍 |
Ah whoops, indeed it is! Cool, sounds good 👍 |
@jstarry I just added a section to the PR description where I try to explain the purpose (and desired result) using some examples. |
Also I hope you don't mind that I removed the checklist from the description. I know it has its purpose but for me it's mostly just in the way... |
@jstarry there's one more case I would like to cover before I'm content with this PR: // self is a component with a `children` prop.
html! {
<List>
{ self.props.children }
</List>
} Currently this behaves like everything else prior to the changes. That is, the children are rendered into a single So here's what I propose:
If you're willing to accept a breaking change I think we should no longer allow |
Willing to accept breaking changes, I trust your judgment! |
I forgot that the children need to be cloned anyway... I could also change the So to recap, these are the three options as I see them:
Right now I'm going with the first approach because it's consistent with all other props which also need to be cloned explicitly. Please let me know if you have a particular preference, @jstarry. |
I guess Travis just doesn't want to cooperate today... |
Oh there's one thing I forgot to ask. |
There is one more difference, |
Why are we stuck with this case? |
Yes, due to messiness! I'd be happy if that restriction was removed 👍 |
By the way, your changes look awesome! Great clean up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to you if you'd like to address the html_nested! {}
restriction in this PR or in a new one. I think this is great as is
You're right, that is an important detail. This still holds true for the new
I'm not sure I understand what you mean by this but if you're wondering why one would ever need to do this in the first place then look no further than the
I think it makes sense to include this here. It already introduces breaking changes and restructures the whole tree organisation. |
I was thinking of this html! { self.props.children.clone() }
Sounds good! |
That would be a lot better, but I can't make it work. ::yew::virtual_dom::VNode::from({ self.children.clone() }) For this to work we need to implement The only other way I can think of to make this work would be to treat expressions ( We can wrap it in a fragment though: html! { <>{ self.props.children.clone() }</> } I quite like this syntax because it forces the user to consider that they're dealing with a list of children. The generated code is also pretty much equivalent to I think we should go with this one. |
I thought about this a bit more and I might have misinterpreted it, but html_nested! {
<span>{ 1 }</span>
<span>{ 2 }</span>
}; Generates the error: error: unexpected token
|
| <span>{ 2 }</span>
| ^ It just generates a much more cryptic error message than The question is... should this work? And if so, what should it return? Perhaps we can come up with a better way to solve the problem For now I just changed it so that |
Good question, maybe it shouldn't. I'm not sure what it would return 😆
Perhaps, not a pressing issue IMO
Sounds good! |
self.to_vec().into_iter() | ||
pub fn iter<'a>(&'a self) -> impl Iterator<Item = T> + 'a { | ||
// clone each child lazily. | ||
// This way `self.iter().next()` only has to clone a single node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement 👍
pub struct HtmlRootNested(HtmlTreeNested); | ||
impl Parse for HtmlRootNested { | ||
/// Same as HtmlRoot but always returns a VNode. | ||
pub struct HtmlRootVNode(HtmlRoot); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This naming is a great improvement
)" This reverts commit d792e37.
Fixes #1262
What and why?
The easiest way to explain the purpose of these changes is to look at some examples.
Example 1
Let's assume we want to create a
List
component that wraps the contents in a<ul>
tag and also wraps each child in a<li>
so that users of this component don't even need to think about it.The code for such a component looks like this:
Now let's try to use this component.
If we only use 'literal' html it works as expected:
HTML output
But of course in a lot of cases (if not most) the list's content will be dynamically generated. Something like this:
This doesn't generate the same HTML because the expression block is treated as a single child instead of being 'unfolded' into multiple.
HTML output
Currently the only way around this issue is to pass the children as a property or to use an undocumented feature that allows us to use a vector in expressions below components.
The primary goal of this PR was to make this particular case work as expected.
Example 2
It is now possible to use a vector in an expression block.
This should be seen as a better alternative to using
.collect::<Html>()
. It has more semantic meaning as an actual list of children and is less verbose in a lot of situations.This is mostly a side effect of the changes but I'm mentioning it because I would like to remove the recommendation for
.collect::<Html>()
. There may still be a use case for it but it explicitly creates a single vnode thereby circumventing the work done in this PR.There are certainly times where the 'unwrapping' isn't desired. In those cases I would advocate for the use of fragments:
Technical details
Refactor of the different Html tree types
Right now there are 4 tree types:
HtmlTree
,HtmlRoot
,HtmlRootNested
, andHtmlTreeNested
.I noticed the following problems:
HtmlTreeNested
is justHtmlTree
but it doesn't wrap the code in::yew::virtual_dom::VNode::from
.HtmlRootNested
is justHtmlTreeNested
. That's it. There's no difference.HtmlRoot
is justHtmlTree
but the parse function is extended to handleHtmlIterable
andHtmlNode
.HtmlTree
contains the two variants mentioned above but it will never parse them (onlyHtmlRoot
can create anHtmlTree
with those variants).This PR changes it so that there's only
HtmlTree
andHtmlRoot
.HtmlTree
is basicallyHtmlTreeNested
in that it no longer wraps the code in::yew::virtual_dom::VNode::from
.HtmlTree
also no longer contains theHtmlIterable
andHtmlNode
, both were moved toHtmlRoot
.Speaking of which,
HtmlRoot
behaves exactly like it did before but it now does a lot of the work thatHtmlTree
did itself.Unified node children behaviour.
There are 3 node types that support children:
HtmlTag
,HtmlList
, andHtmlComponent
.All of them currently have their own implementation of the whole children thing and all of them are slightly different.
HtmlList
andHtmlComponent
are very close. They both useNodeSeq
for more flexibility, which is why they already support passing children in a vector. The difference is thatHtmlComponent
collectsHtmlTreeNested
nodes whileHtmlList
usesHtmlTree
(note that I'm referring to the old types here, not the refactored ones).HtmlTag
on the other hand is totally different because it doesn't useNodeSeq
. Instead it collectsHtmlTree
and just puts all of them in a vector. Unlike the other two methods, which accept any expression that implementsInto<VNode>
, here expressions have to resolve toVNode
directly.This PR makes it so all three behave exactly the same. It introduces the new type
HtmlChildrenTree
which does all of the work.ChildrenRenderer
no longer implementsRenderable
(breaking change)The reason for this is the same as for why I think we should discourage
.collect::<Html>()
- it explicitly creates a single node.There is one case where this results in a bit more verbosity: when the user actually wants to create a single node. I can only think of one use case for this though and that is for components that want to return their children unchanged. The
ContextProvider
is one such example.The code for doing this just looks like this now: