-
-
Notifications
You must be signed in to change notification settings - Fork 29
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 Lazy function #156
Add Lazy function #156
Conversation
`Lazy` provides lazy evaluation of node rendering. Useful together with `If`. Fixes #152
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #156 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 9 9
Lines 505 519 +14
=========================================
+ Hits 505 519 +14 ☔ View full report in Codecov by Sentry. |
Hi, thanks for asking for feedback. I like the idea. It's simple (both usage and the implementation), it works. I looked in detail and can't find anything to improve, implementation-wise. Perhaps docs need to be expanded (with a caution to prefer this when using If). If it weren't a breaking change, I'd even suggest to allow only lazy functions as the second argument to If, but maybe too much of a hassle for medium+ projects using the lib at this point. |
@egeozcan thank you for the quick feedback! Good idea with the docs, I'll add that to I considered a breaking change, but since I'd hate to break existing consumers and The only thing I'm not entirely happy with is the variadic
I'll leave this open for a while to see if anyone has feedback. |
@markuswustenberg you are very much welcome and thank you for such attention to details. About the variadic arguments & panic hack... Again I witness things getting complicated caused by go allowing neither overloads nor default values. Many choose the option you chose, and while I personally would go for 2 functions (Lazy for elements and LazyAttribute for attributes), it comes down to personal preference. No strong opinions. And I agree with removing Stringer. It feels out of place. |
@egeozcan thank you for your response. Yeah, I considered two functions as well, but I think I'll go with my current one even though it's a bit more "magic" (in a bad way). Maybe I'll think of something better later. Thank you again for your input! |
I wrote this today: func Iff(condition bool, node func() g.Node) g.Node {
if condition {
return node()
}
return nil
} Usage Iff(props.subtitle != nil, func() g.Node {
return Span(g.Text(*props.subtitle))
}), I thought but g.If(u != nil, g.Lazy(func() g.Node {
return g.El("span", g.Text(u.Name))
})), update: and it was even a previous exploration here: #99 |
@hastebrot yeah, your Iff is maybe how I should have implemented it in the first place. But in the end, I guess both have their usage, and I'm hoping I've found a sweet spot with Lazy. 😊 |
// Used with If, it enables lazy evaluation of the node to return, depending on the condition passed to If. | ||
// It defaults to being an ElementType, but can be set to AttributeType by passing a NodeType. | ||
// Even though nodeType is variadic, only the first NodeType is used. | ||
func Lazy(cb func() Node, nodeType ...NodeType) 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.
Two thoughts:
- This seems to be complicated by implementing the optional nodeTypeDescriber interface, but I've been reading through the code and it seems like the only use of nodeTypeDescriber is to omit the output if the type is wrong here
Line 110 in 1f0bb2a
if p, ok := c.(nodeTypeDescriber); !ok || p.Type() == ElementType { - If NodeType is important, and there are only two node types, maybe create a LazyAttr function and a LazyEl function?
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.
@vikstrous2 thank you for the review! Sorry for the delay in replying, the winter and its various sicknesses has not been kind to me and my family this year… 🤒
Yes, nodeTypeDescriber
is only used for checking whether a node should be rendered as an element or attribute. I think it's pretty useful in e.g. form checkboxes and the checked
attribute, and it feels wrong to leave out support for attributes, even though it would simplify the API quite a bit here.
LazyAttr
would also solve this, arguably more elegantly, but I would really like there to not be two functions for this already slightly edge-case functionality.
Hmm. I'm seriously considering dropping this whole Lazy
stuff, it doesn't quite feel right.
See #172 as a solution instead. |
Lazy
provides lazy evaluation of node rendering. Useful together withIf
.Fixes #152