-
-
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
Add a macro for building properties outside of html! #1599
Conversation
They add a lot of cognitive overhead and don't provide much benefit in this case.
79 | html! { <Child:: /> }; | ||
| ^^^^^^^^^^^ |
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.
At the time of writing this is the only expected error span regression.
Instead of boring you with why this happens (TL;DR: blame syn
) let me instead tell you about the silver lining:
This type of span error is already present right now, it can happen in props / attributes when there's an unexpected end of input (and also some other weird cases). When it happens the error span unfortunately points at the entire call site giving little to no helpful indication of the actual error location.
I used some hacks in this PR to make the span point at the offending tag instead.
It's currently impossible to get the exact location but pointing at the correct tag already helps a ton. Especially in large html!
trees.
There is still the "double children error thing" I want to address and I will probably uncover a few more things when reviewing the changes BUT at the very least the original premise of the PR is implemented now. |
Visit the preview URL for this PR (updated for commit 620e503): https://yew-rs--pr1599-props-s3b9y5s9.web.app (expires Tue, 27 Oct 2020 22:21:31 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
@jstarry do you have some time to take a quick look a this? There are quite a few changes in this PR (listed in the description) and I want to make sure you don't have see a problem with any of them. Also, I know it might not look like it, but I'm trying to work toward v0.18. I just don't really want to start #1550 before this is merged because of the many changes here 😅. |
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.
Awesome clean up! I couldn't help but dive into a full review 😉 your code is always pleasant to read haha.
I'm cool with all the breaking changes, feels like we're nearing a 1.0 release :)
let props = yew::props!(LinkProps | ||
href="/" | ||
text=Rc::from("imagine this text being really long") | ||
size=64 | ||
); |
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.
I'm wondering if it would make sense to mirror the struct syntax instead. I think the prop=value
syntax in particular is a little odd outside of macro. What about this:
let props = yew::props!(LinkProps | |
href="/" | |
text=Rc::from("imagine this text being really long") | |
size=64 | |
); | |
let props = yew::props!(LinkProps { | |
href: "/", | |
text: Rc::from("imagine this text being really long"), | |
size: 64, | |
}); |
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.
I see where you're coming from. When I wrote the documentation and had to split this across multiple lines for readability my first thought was "huh, that looks odd".
Here are a few points in favour of the current syntax that come to mind:
- because it's the same syntax you can move code from
html!
toyew::props!
or vice versa and it still works. - it's more obvious that the same rules apply, especially with things like transformers. I feel like the fake-Rust syntax can easily lead to wrong assumptions.
- the
yew::props!
macro uses the exact same parser as the props in thehtml!
macro. This makes it much easier for the two to stay in sync. It should also make it easier for a potential Yew IDE extension. - if we want to add special syntax in the future (like
?=
) it would be very hard to reproduce this in theyew::props!
macro without breaking the fake-Rust syntax.
html! {
<Foo
title_props=yew::props!(Title::Properties text="hello world")
body_props=yew::props!(Body::Properties
class="color-red"
onhover=self.link.callback(Msg::HoverBody)
)
onclick=self.link.callback(Msg::Click)
/>
}
// vs
html! {
<Foo
title_props=yew::props!(Title::Properties { text: "hello world" })
body_props=yew::props!(Body::Properties {
class: "color-red",
onhover: self.link.callback(Msg::HoverBody)
})
onclick=self.link.callback(Msg::Click)
/>
}
Personally, I think the first one is less distracting.
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.
because it's the same syntax you can move code from html! to yew::props! or vice versa and it still works.
Good point!
it's more obvious that the same rules apply, especially with things like transformers. I feel like the fake-Rust syntax can easily lead to wrong assumptions.
Yeah fair point about the transformers but, to be honest, I think transformer behaviour is not usually that obvious anyways.
the yew::props! macro uses the exact same parser as the props in the html! macro. This makes it much easier for the two to stay in sync. It should also make it easier for a potential Yew IDE extension.
They are actually already different in regards with "with props" and special props so maybe it actually makes sense for them to diverge a bit. The nice thing is that the tokenizer code wouldn't have to change.
if we want to add special syntax in the future (like ?=) it would be very hard to reproduce this in the yew::props! macro without breaking the fake-Rust syntax.
This is probably the most compelling reason to me to keep the "label = value" syntax. Is it pretty likely we'll need to add that special syntax?
Some points in favour of struct syntax
- I imagine that rustfmt will help make props look nice in the struct form
- You can move code easily between using yew::props! and not using the macro
- when yew::props! is used separately from the JSX syntax it looks quite strange
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 is probably the most compelling reason to me to keep the "label = value" syntax. Is it pretty likely we'll need to add that special syntax?
I'm not sure, I had two-way bindings in mind, but something like that only makes sense in the context of a component so it wouldn't work in yew::props!
anyway.
For now I don't think there's any reason to worry about special syntax.
I imagine that rustfmt will help make props look nice in the struct form
I'm fairly certain that doesn't work. As far as I can tell Rustfmt is entirely disabled in proc macros by default (Because of rust-lang/rustfmt#3434).
You can move code easily between using yew::props! and not using the macro
Moving away from the macro still requires the user to add the fields with default values and manually add the transformations performed by Transformer
. It's certainly true the other way around though.
when yew::props! is used separately from the JSX syntax it looks quite strange
Absolutely agree, especially when split across multiple lines.
I came up with some reasons in favour of the struct-like syntax of my own:
- You can use the field init shorthand (
a
instead ofa: a
) - In the future we could support the base expression syntax (
Props { text: "a", ..other_props }
)
Either syntax has its fair share of problems but I'm totally fine with going with the struct syntax.
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.
I'm fairly certain that doesn't work. As far as I can tell Rustfmt is entirely disabled in proc macros by default (Because of rust-lang/rustfmt#3434).
Got it, makes sense.
Moving away from the macro still requires the user to add the fields with default values and manually add the transformations performed by Transformer. It's certainly true the other way around though.
Ah yeah, I meant the other way around 👍
You can use the field init shorthand (a instead of a: a)
Good point!
Either syntax has its fair share of problems but I'm totally fine with going with the struct syntax.
Certainly true. Sounds like struct syntax has the edge, shall we go with that then?
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.
Oh! You already made the change! Looks good!
Oh could you rename the test files from html_tag -> html_element too? |
Description
Adds a new macro (
yew::props!
) which can be used to create properties outside of thehtml!
macro.The motivation for this is that creating props manually can be very verbose, especially if there are a lot of optional props (more specifically, any props with default values).
This also makes it much more ergonomic to pass props around or nest them. Hopefully this helps with #1533 a bit.
Here's a peek at what it looks like:
This PR also contains some misc. clean up of the macro code such as removing a lot of import renames which add a lot of unnecessary cognitive overhead to the already complex subject of macros.
I also ended up unifying most of the parsing logic for tags and props / attributes. Before this PR each node type (list, component, "tag" (element)) had its own parsing logic whereas now it's abstracted and all of them can use the same logic.
This should help maintain coherence.
Also removes the long deprecated component syntax
<Comp: a, b, c />
.Closes: #1604
Changes
<.../>
syntax. Both components, elements, and fragments are tags.with props
now supports expressions instead of just idents making it much more ergonomic to use.with props
now handles children (as opposed to silently ignoring them). The new behaviour is such that the children given inhtml!
overwrite the ones already inprops
. This could also emit an error but I felt like this makes more sense.yew::props!
macroProperties
derive macro to theProperties
trait. Before it was possible to import them separately (needless to say that this can be very confusing). Now, if you importProperties
from yew you always get both. (This is a slightly breaking change)Other Things?
I wanted to change the
with props
syntax so that thewith x
expression has to follow the special props (i.e. it always comes last). This would've simplified parsing and added some consistency. For the sake of backward compatibility I didn't go with it (but I have to wonder if there's even a single app that placesref
/key
afterwith
).Turns out keywords (reserved idents) work in the
props!
macro but theProperties
derive macro doesn't support them.