Add testing for 'idiomatic way to pass child component lists into an element's constructor'

[pdf]

Can you tell me why https://github.com/gopherjs/vecty/blob/34ae77073fd923c6ab2f89c234a8ecf76f463cb0/dom.go#L175 exists?

Would it not be safe to just continue here?

[dmitshur]

The commit message of ee9d6c9886074534c4ad3aadbc3c2df27247b284 says:

panic if nextChildRender == prevChildRender (very invalid state)

See #70

[pdf]

Sorry, should have used blame to start, however I've read the issue and the related code, and I still may be confused.

The upshot seems to be that for every render, nested components must be reinstantiated. I think I can see why that is as the code stands, but it makes natural patterns produce this panic, which is confusing.

The obvious pattern is mentioned in #70 - passing and storing components to be nested. This can be worked around as Dave did by passing a closure that returns new instances of the components, but this doesn't seem ideal.

I think we can support at least Components by just calling Rerender if prevChild == nextChild, ie:

diff --git a/dom.go b/dom.go
index 7ee1c9c..12fa374 100644
--- a/dom.go
+++ b/dom.go
@@ -158,8 +158,8 @@ func (h *HTML) restoreHTML(prev *HTML) {

        // TODO better list element reuse
        for i, nextChild := range h.children {
-               nextChildRender := doRender(nextChild)
                if i >= len(prev.children) {
+                       nextChildRender := doRender(nextChild)
                        if doRestore(nil, nextChild, nil, nextChildRender) {
                                continue
                        }
@@ -167,6 +167,11 @@ func (h *HTML) restoreHTML(prev *HTML) {
                        continue
                }
                prevChild := prev.children[i]
+               if c, isComponent := nextChild.(Component); isComponent && prevChild == nextChild {
+                       Rerender(c)
+                       continue
+               }
+               nextChildRender := doRender(nextChild)
                prevChildRender, ok := prevChild.(*HTML)
                if !ok {
                        prevChildRender = prevChild.(Component).Context().prevRender

Not certain on how to handle HTML. Thoughts?

[slimsag]

Removing this panic is NOT a valid thing to do. While you may find a way to make this work technically in the code -- it's not a good thing to be doing in general: a component Render should always propagate downwards into child components, rendering the entire tree of elements from scratch.

The motivation for this goes back to the very core of what React & Vecty do: you give us a new render of the DOM, and we handle things behind the scenes. Storing a previous render and returning it subsequently in a secondary render is a lot like manipulating the browser DOM natively -- it's just not a good practice.

I understand the desire to use a pattern like that mentioned in https://github.com/gopherjs/vecty/pull/70#issuecomment-260214972 but we'll need to find a way to do it nicely without compromising on the core idea that Render really does return a new virtual DOM tree.

[pdf]

I believe the diff above does what you're describing, but works only for Components, not HTML. It leaves the panic in place.

[pdf]

Would you be willing to accept a PR for the above patch in the mean time? It solves enough of the issue to work for me (I only need to retain persistent vecty.Components, not *vecty.HTML).

[gernest]

gentle ping @pdf @slimsag @shurcooL I came across this , I'm trying to port material design lite to vecty. I'm still puzzled what does render variable mean.

If you don't mind can you enlighten me abit here.

Here is my component, it is a tab panel

type Panel struct {
    vecty.Core
    IsActive bool
    Name     string
    ID       string
    Children vecty.MarkupOrComponentOrHTML
}

func (p *Panel) Render() *vecty.HTML {
    c := make(vecty.ClassMap)
    c["mdl-tabs__panel "] = true
    if p.IsActive {
        c["is-active"] = true
    }
    return elem.Div(
        c,
        p.Children,
        prop.ID(p.ID),
    )
}

Rerendering this component results into the panic. And I need to rerender this to reflect tab selection, I have tried several setps that seemed to not work. Now I have a feeling I missed something and that my understanding on how vecty work is limited.

The Children field is important as user defined content of the tab.

Any ideas will be highly appreciated

Thanks

[pdf]

@gernest the render variable is internal to vecty. You are right to suspect the Children field to be responsible for your woes - you could try pulling in my patch from #94 to see if it resolves your problem, and you can watch #94 for merge status, however @slimsag said that he would like to add some tests first, so it might be a while.

PS - MDL is deprecated in favour of material-components-web.

[gernest]

@pdf thanks. I think I will try to avoid re rendering the component altogether, I will do direct dom manipulation for the same effect.

PS - MDL is deprecated in favour of material-components-web.

I heard about that, but I have to start somewhere. If having a full mdl port is possible, then migrating it to material-components-web. should be possible as well.

[slimsag]

This is most likely fixed, as @pdf said in #124:

I think #78 should be close to solved with this merge, combined with the rework of render/renderComponent/reconcile, but I've not tested sufficiently to call it conclusively.

I think this is correct, but let's keep this issue around for adding proper testing of this functionality. I also wonder how this will influence or need to be changed based on whatever we do for #115

[slimsag]

Updated title to reflect what needs to be done here, per my last message.