Keyed children #147
Keyed children #147
Conversation
|
The implementation relies on the new The reason for requiring Components to implement the interface, rather than just relying on I considered alternative approaches to avoid requiring the interface, as I recall you raising that preference last time we looked at this, but the caveats were too severe for them to be viable. |
Codecov Report
@@ Coverage Diff @@
## master #147 +/- ##
==========================================
- Coverage 48.02% 42.46% -5.56%
==========================================
Files 4 4
Lines 456 551 +95
==========================================
+ Hits 219 234 +15
- Misses 212 290 +78
- Partials 25 27 +2
Continue to review full report at Codecov.
|
dom.go
Outdated
| @@ -69,6 +69,13 @@ type Unmounter interface { | |||
| Unmount() | |||
| } | |||
|
|
|||
| // Keyer is an optional interface that a Component can implement in order to | |||
| // uniquely identify the component amongst its siblings. If implemented, all | |||
| // sibling components and HTML must also be keyed. | |||
There was a problem hiding this comment.
As it stands:
If implemented, all sibling components and HTML must also be keyed.
Makes it sounds like "all sibling components" and "HTML" must be keyed, so maybe even children of children? I think that's not the intent, so better wording would be:
If implemented, all siblings, both component and HTML, must also be keyed.
dom.go
Outdated
| // uniquely identify the component amongst its siblings. If implemented, all | ||
| // sibling components and HTML must also be keyed. | ||
| type Keyer interface { | ||
| Key() string |
There was a problem hiding this comment.
Add docstring:
// Key returns the key value that uniquely identifies this component amongst its siblings.
There was a problem hiding this comment.
Why is a Keyer using a string key and not an interface{} key? I think that cases like this will be very common, and interface{} equality works just the same for us internally as string equality would (i.e. we don't need to convert to a string).
There was a problem hiding this comment.
It could be an interface{}, string was just the most sensible concrete type to me. I'm okay with changing it though.
There was a problem hiding this comment.
I'll have to try and document very clearly that the returned value's type must be comparable, maybe include a link to https://golang.org/ref/spec#Comparison_operators, or maybe specify that it must be a valid map key and link to https://golang.org/ref/spec#MapType, though that might be even more confusing.
There was a problem hiding this comment.
I am still confident that interface{} is a better choice than string
I think that saying:
// Key returns a value that will be compared against sibling key values for ordering purposes.
Is plenty enough. I don't think we need to even link to the spec about comparison operators.
If an interface{} value is not comparable to another interface{} value, nothing bad will happen (i.e. no panic) aside from the comparison becoming false / not working as the user may incorrectly expect. For example: https://play.golang.org/p/7uK4AQbER-
Anecdotally, although not exactly the same situation, the new sync.Map type also doesn't document requirements for keys: https://golang.org/pkg/sync/#Map.Load
There was a problem hiding this comment.
We don't strictly compare different key values, just look them up, and check for conflicts. That said, a value that is invalid as a map key will panic: https://play.golang.org/p/HjI8bscDdz.
And sync.Map is a rather different scenario - it's obvious that you must adhere to map semantics there, whereas here, the fact that the key is stored in a map internally is an implementation detail, and is not obvious to the user.
Maybe we can get away with just saying that the provided key's type must be comparable.
There was a problem hiding this comment.
Right, I see, I had forgotten that. How about just:
// Key returns a value that will be used for ordering purposes. The type must be valid as
// a map key, or a panic will occur.
Clear and to the point. Also, we should make sure to add something like:
_ = map[interface{}]struct{}{key: struct{}{}}
At any location where we receive a map key from the user of Vecty, as this would ensure they get a stack trace that shows exactly the location where they passed the invalid key. Presumably the compiler will optimize this out completely (but I didn't check, and it doesn't matter).
There was a problem hiding this comment.
Also, we should make sure to add something like:
I don't think we bother with this, components can implement Keyer however they like anyway, so would be of limited utility.
There was a problem hiding this comment.
Fine for now, I'll think about it more.
dom.go
Outdated
| } | ||
|
|
||
| // Node returns the underlying JavaScript Element or TextNode. | ||
| func (h *HTML) Node() *js.Object { return h.node.(wrappedObject).j } | ||
|
|
||
| // Key satisfies the Keyer interface. |
There was a problem hiding this comment.
Replace satisfies with implements
There was a problem hiding this comment.
In Go terminology, interfaces are satisfied not implemented. However, it's good to keep the documentation consistent for this code base.
There was a problem hiding this comment.
I'm happy to be proven wrong, but I've never seen <Method> satisfies the <Interface> interface. over <Method> implements the <Interface> interface. in a docstring.
https://gotools.org/net/http#transport.go-L337 seems to agree with me.
| @@ -81,6 +88,7 @@ func (p *ItemView) Render() *vecty.HTML { | |||
| "completed": p.Item.Completed, | |||
| "editing": p.editing, | |||
| }, | |||
| prop.ID(p.Item.Title), | |||
There was a problem hiding this comment.
What is this for ? I assume it's a quick fix for some bug? Or added here unintentionally?
There was a problem hiding this comment.
My bad, I left this in from my testing - just made it easier to identify items from devtools, will remove.
dom.go
Outdated
| if keyer, ok := nextChild.(Keyer); ok { | ||
| nextKey = keyer.Key() | ||
| if len(h.keyMap) > 0 && nextKey == "" { | ||
| panic("vecty: all siblings must have keys when using keyed elements") |
There was a problem hiding this comment.
I agree with the panic, but nextKey == "" places an unusual constraint that "" is not a valid key? As mentioned above, I'd prefer we switch to interface{} key types, in which case we can use nil here I suppose.
There was a problem hiding this comment.
Zero value needs to be invalid for whatever type is used, yes.
|
Let me know when you're ready for me to do another pass on this, I may have some time one night during the week. |
dom.go
Outdated
| func (l childList) remove(next *HTML, insertBeforeNode jsObject) { | ||
| l.html.node = next.node | ||
| l.html.insertBeforeNode = next.insertBeforeNode | ||
| l.html.removeChildren(l.html.children) |
There was a problem hiding this comment.
The documentation on this function and its parameters do not make sense to me.
If the goal is to remove the childLists's elements from the DOM node, then why does it also take a next parameter?
Why is there an unused insertBeforeNode parameter?
There was a problem hiding this comment.
I'll rename next (the target for the operation) and update the docs. The insertBeforeNode was used before this commit, but now is not, will remove.
dom.go
Outdated
| } | ||
| case List: | ||
| nextChild = childList{html: &HTML{children: v}} | ||
| h.children[i] = nextChild |
There was a problem hiding this comment.
h.children's data type is []ComponentOrHTML which does not include childList. It feels wrong to ignore this.
There was a problem hiding this comment.
ComponentOrHTML is exported, but this is an unexported type, what would you like to do?
There was a problem hiding this comment.
Could we change it to an interface{} type without requiring extensive* copying / conversion all over the place? (I am not sure).
Otherwise, maybe we just need a doc comment on the definition of children now:
// important: children is actually ComponentOrHTML OR childList here, from an internal perspective.
dom.go
Outdated
| // Massage concrete type if necessary. | ||
| switch v := nextChild.(type) { | ||
| case *HTML: | ||
| if v == nil { |
There was a problem hiding this comment.
Document.
// The next child's type is *HTML but it is nil. We want the same effective
// handling as if it was a nil ComponentOrHTML, so explicitly set nextChild to
// nil now.
dom.go
Outdated
| h.children[i] = nextChild | ||
| } | ||
| case List: | ||
| nextChild = childList{html: &HTML{children: v}} |
There was a problem hiding this comment.
Document. There is a reason why we are replacing List types with our own childList type, but I do not know what it is.
dom.go
Outdated
| } | ||
| if !keyed { | ||
| keyed = len(h.keyMap) > 0 | ||
| } |
There was a problem hiding this comment.
Does keyed need to be persistent outside the scope of the for loop?
i.e., why don't we just declare keyed := len(h.keyMap) > 0 before the first usage below here?
There was a problem hiding this comment.
By "I think this is fine" do you mean the current code, or what I suggested?
There was a problem hiding this comment.
I think the change is fine.
There was a problem hiding this comment.
This does need to exist outside this scope, since we use it to determine how to remove children. I've slightly changed how this works in the next push any way, since we need to track this across renders too now.
dom.go
Outdated
| prevChildList.remove(h, h.insertBeforeNode) | ||
| if h.insertBeforeNode != nil { | ||
| h.insertBeforeNode = prevChildList.html.insertBeforeNode | ||
| } |
There was a problem hiding this comment.
I don't understand what this if block is doing. Please document.
dom.go
Outdated
| keyMap map[string]ComponentOrHTML | ||
| nestedKeys bool | ||
| insertBeforeNode jsObject | ||
| lastRenderedChild *HTML |
There was a problem hiding this comment.
The fields you've added here are not self-explanatory like the ones prior to this change. key is self explanatory, but:
keyMapnestedKeysinsertBeforeNodelastRenderedChild
are not. Can you document each of them with an explanation of what they are / what state they carry?
dom.go
Outdated
| var ( | ||
| new = h.node != prev.node | ||
| nextKey string | ||
| stableKey bool |
There was a problem hiding this comment.
Why do we not define this variable closer to where it is used down below?
There was a problem hiding this comment.
If you'd prefer to split these up I can.
dom.go
Outdated
| new = h.node != prev.node | ||
| nextKey string | ||
| stableKey bool | ||
| insertBeforeKeyedNode jsObject |
dom.go
Outdated
| keyed = len(h.keyMap) > 0 | ||
| } | ||
|
|
||
| if (i >= len(prev.children) && !keyed) || new { |
There was a problem hiding this comment.
-
This block is purely responsible for handling creation of new child elements that the previous DOM node's children did not have, so we should document that now.
-
Why does this condition on
!keyed? Why would new children that are keyed not be handled here? I don't understand.
There was a problem hiding this comment.
- Ack
- Because position can not be used for anything when dealing with keyed children - the feature is designed to deal with children that may move position, and that means they may appear at a higher index on the current render, than the last render had. Once we know we're dealing with keyed children, the key is the only authoritative index.
| var prevChild ComponentOrHTML | ||
| if len(prev.children) > i { | ||
| prevChild = prev.children[i] | ||
| } |
There was a problem hiding this comment.
This change introduces a lot of uncertainty for me about whether or not at this point in the loop we're dealing with children that have a direct relationship to prev or not. It seems this is only the case when keyed == true, so I think it'd be more clear to have this prevChild declaration be inside the if below and then keep the old prevChild declaration that you removed but have it defined below this keyed if statement.
There was a problem hiding this comment.
The if check here is required, because if we're dealing with keyed children, the earlier condition may not have continued the children loop when we get here - we still want to assign this var in either case, it just needs this check now to avoid overflowing the prev.children array when keyed.
.. with the above said; I'm afraid of this change right now because I've spent a considerable amount of time trying to grok this but still do not fully understand the implementation. With 2-3 hours yesterday, and 2 hours just now, I'm out of time and probably won't have time to take another look until next weekend. I feel confident you've done your due diligence, and that this is a sound change, but this currently adds an additional level of complexity that I currently cannot reason about. I'm not saying we shouldn't merge it, and also not saying we should block it on testing. But in its current state I will need more time (maybe 1 - 3 weekends) to fully parse and internalize how this is all working together. I think the main thing I do not understand is how, after this change, the overall reconciliation flow is operating. Which conditions are handling new or existing children, keyed or not, etc. is all very overwhelming currently. I'll come back to this when I have some more spare time, hopefully next weekend. If you'd like to take a second pass on this in the meantime, feel free to of course. |
|
I can understand that, In the mean time, I may take another pass and at least address the notes here, see if there's any way to make things easier to parse. The silver lining here though, is that I believe this is the last outstanding major architectural change, based on the current issue list. |
|
We got a little side-tracked with details, but when I pushed this I also wanted to discuss some alternative implementation details. There's no need to understand the current implementation to discuss these. The implementation I've pushed here requires all siblings to be keyed, including those that are rendered as part of a The only options I could think of to allow a separate keyspace for
I'll find some time to post some examples of what I'm talking about, if this isn't clear enough. Please let me know your thoughts @slimsag, and if there are particular parts you would like clarified. |
Good point; thank you for flagging this. I agree that this does not sound ideal, one reason in particular is because a
|
type KeyedComponent struct {
vecty.Core
ID string `vecty:"prop"`
}
func (c *KeyedComponent) Key() interface{} {
return c.ID
}
func (c *KeyedComponent) Render() *vecty.HTML {
return elem.Div(vecty.Text(c.ID))
}
type MyComponent struct {
vecty.Core
data []string
}
func (c *MyComponent) Render() *vecty.HTML {
var contents []vecty.MarkupOrChild
for _, v := range c.data {
contents = append(contents, &KeyedComponent{ID: v})
}
contents = append(contents, vecty.List(some_footer_elements)) // <- this will panic with missing key error on the List when rendered
return elem.Div(contents...)
}So, the user has a bunch of keyed components (could just as easily be keyed elements), that are generated from some data store. This is the most common reason to use keys - dealing with arbitrary data that the developer doesn't control, and may change between renders. With option 3, the user is left with the requirement to add some arbitrary key they make up to the Of course, it could be argued that this is poorly written, and to avoid needing to make up a key for the footer func (c *MyComponent) Render() *vecty.HTML {
- var contents []vecty.MarkupOrChild
+ var contents vecty.List
for _, v := range c.data {
contents = append(contents, &KeyedComponent{ID: v})
}
- contents = append(contents, vecty.List(some_footer_elements)) // <- this will panic with missing key error on the List when rendered
- return elem.Div(contents...)
+ return elem.Div(
+ contents,
+ vecty.List(some_footer_elements),
+ )
}But in that case, option 1 works just as well, and the panic in the original implementation would be more explicit about what the problem is - |
|
I think I understand. I'll be brief since I don't have much time -- so weigh this comment with less weight than usual as there could be a critical piece I have not understood yet. My thinking was that we might add something like type KeyedComponent struct {
vecty.Core
ID string `vecty:"prop"`
}
func (c *KeyedComponent) Key() interface{} {
return c.ID
}
func (c *KeyedComponent) Render() *vecty.HTML {
return elem.Div(vecty.Text(c.ID))
}
type MyComponent struct {
vecty.Core
data []string
}
func (c *MyComponent) Render() *vecty.HTML {
var contents []vecty.MarkupOrChild
for _, v := range c.data {
contents = append(contents, &KeyedComponent{ID: v})
}
- contents = append(contents, vecty.List(some_footer_elements)) // <- this will panic with missing key error on the List when rendered
+ contents = append(contents, vecty.ListWithKey("listkey", some_footer_elements)) // <- this does not panic because vecty.ListWithKey was used instead of vecty.List
return elem.Div(contents...)
}The idea being that you assign a key for the entire list itself, rather than for individual elements of the list. In this case Maybe i've misunderstood something, though? |
I'm not entirely certain how to make it consistent, ie - what is the method signature of type KeyedList struct {
key interface{}
list LIst
}
func ListWithKey(key interface{}, List) KeyedList {}
My example may have been based on a lack of imagination, and being too close to the code, now that I think about it further. I had not considered the possibility that a user may want to generate a list of keyed |
Yes, I think that a signature like that would be ideal. Other semantics which probably aren't important for validating that this idea would work:
type keyedList struct {
list LIst
key interface{}
}
// ListKey wraps the given List with the given key for ordering purposes.
func ListKey(list List, key interface{}) MarkupOrChild { return keyedList{list, key} }I'm not super confident about the name, but it seems better than I think having What do you think of this approach overall? Good enough idea to take a try at implementing it? |
Not sure I'm in love with // WithKey wraps the List in a Keyer using the given key, for ordering purposes.
func (l List) WithKey(key interface{}) vecty.MarkupOrChild {}
I think I can see how this might fit together, can take a pass at it. |
|
I like that API/name approach a lot! That's beautiful :) Can't believe I didn't think of that myself! |
|
New pass addresses previous comments, and implements the keyed list with separate keyspace, and |
pdf
force-pushed
the
keyed_children
branch
2 times, most recently
from
5680da3
to
bfc1f63
Compare
|
Thanks @pdf ! I'll do my best to review this ASAP so we can land it in this weekend. |
|
I'm not sure it's much easier to understand now than in the last pass, so please fire any questions you have at me. I'm not sure how much more time I'll have to work on this over the weekend, but I'll certainly try to clarify anything you need me to. |
|
I have a little time today if you have thoughts/queries @slimsag. |
|
Hi @pdf ! Sorry for the delayed response to your above messages! I've just spent the last ~6 hours working through your PR case by case and feel I have a very solid understanding of how everything works now. :) I've created a separate PR to your branch at pdf#2 which has very individual commits + good commit messages with my suggestions. Once you've had a chance to look over them and feel good about the suggestions we can squash merge to keep a clean git history. This change is very good! I like it quite a lot! 😃 |
|
No problem, glad you could make sense of it now. I'll take a look over your proposed changes now. |
This reverts commit 0cbd104.
This change fixes a regression introduced by PR hexops#134. The basic issue is that when `nil` is encountered, it is treated correctly as a nil render, but when `(*HTML)(nil)` is encountered, it is not. I discovered this while porting Go-Package-Store (which uses Vecty) over to the new API. The following code effectively demonstrates the issue: ```Go package main import ( "github.com/gopherjs/vecty" "github.com/gopherjs/vecty/elem" ) func main() { vecty.RenderBody(&PageView{}) } type PageView struct { vecty.Core } func (p *PageView) Render() *vecty.HTML { return elem.Body( // this works: //nil, // this does not: p.returnsNil(), ) } func (p *PageView) returnsNil() *vecty.HTML { return nil } ```
dom.go
Outdated
| switch v := prevChild.(type) { | ||
| case keyedList: | ||
| pendingMounts = l.html.reconcileChildren(v.html) | ||
| case *HTML, Component, nil: |
|
Looks like you've just merged my changes + made the change #147 (comment) If there's nothing else changed (I didn't spot anything), then I'm happy to merge this as soon as you feel good to :) |
dom.go
Outdated
| } | ||
| // WithKey wraps the List in a Keyer using the given key. | ||
| func (l List) WithKey(key interface{}) ComponentOrHTML { | ||
| return keyedList{key: key, html: &HTML{children: l}} |
There was a problem hiding this comment.
My only concern with this is that MarkupOrChild/ComponentOrHTML may end up with this keyedList in them, which makes the documentation inaccurate, amongst other things.
There was a problem hiding this comment.
Yes that is a good point actually...
There was a problem hiding this comment.
We could publish a type KeyedList interface{} value + update the docs of MarkupOrChild/ComponentOrHTML?
Then nothing else has to change.
There was a problem hiding this comment.
Actually, just make keyedList public with no exported fields / methods
There was a problem hiding this comment.
That was my inclination, too.
|
I shuffled the documentation for |
|
Ready to merge once you are. I will update |
|
|
|
🎉 |
Initial pass at keyed children. This one was pretty tough due to having to handle keys and insertions for nested
Listchildren.Working through this did show up some bugs in the existing implementation of
Listhandling, primarily: the*HTMLniltype coercion from #138 was not applying toListchildren, so I've moved that into*HTML.reconcileChildren(), as I need to handleLists there anyway.Fixes #25