nested user components may not refresh as expected

[nathanhack]

Storing a elem.Italic() in a struct as a vecty.ComponentOrHTML seems to interfere with the diffing algorithm for that struct.

https://play.jsgo.io/3e44bb6e46a5c866c79da52fe30219a72c119507

What you'll see is two fontawesome sync-alt that when you move your mouse over them, they change color. The first one is broken (weirdly it changes once but no more). The last one works as I would have expected. The difference between them is that the last one has the elem.Italic() directly stored as a vecty.ComponentOrHTML instead of in a struct.

This occured while using

github.com/gopherjs/vecty v0.0.0-20191227222824-6a0a25ee5a96

[slimsag]

I took a very brief look at this, the problem is with the way we keep a single persistent component around.

For example, if you have a component Foo that each time Render is called produced a new &Bar we guarantee that we only keep one *Bar around even though there are two (the old and the new) that way you can trust that private fields on Bar can be mutated to maintain state.

The problem with this code is that (*Div).Render() returns components of the same type *FontTemp and so Vecty's internals say "Understood, we'll only keep the last one and will just copy the prop values over from the new one to the old one" and then suddenly both of your component instances:

        &Div{
            Normal:   &FontTemp{On: false},
            Hovering: &FontTemp{On: true},
        },

Have an On value of true, and thus all subsequent rerenders (even though they are correctly rerendering) are rerendering with the unexpected value of On: true.

How to address this (or whether this is expected behavior) is something I haven't pondered yet.

[slimsag]

It's also worth noting that if you use vecty.If this does work as you'd expect because Vecty is able to differentiate between the two FontTemp instances in this case: https://play.jsgo.io/620688b80910cddfebaad8c500273dce4e2cf765

[nathanhack]

I'm still wrapping my head around what you've said. BTW thanks for looking into this!

Ok, I get why the old component pointer is the one you keep, and that the compare checks the types and says, "we'll just copy the prop values over from the new one to the old one".

How/why does the same thing not happen with elem.Div() or any of the other elem.*?
I was looking in dom.go but I realize I'm REALLY not sure where this check is happening.

That said, I feel like this isn't necessarily a bug. It's undesirable but not a bug. It also means I have other code that I'm not sure how it's working correctly as I thought it was doing this same thing.

Based on what I think I understand, you're getting performance gains by not calling the render() function and diffing the nodes; and I don't see how it could be changed without causing a performance penalty.

[slimsag]

How/why does the same thing not happen with elem.Div() or any of the other elem.*? I was looking in dom.go but I realize I'm REALLY not sure where this check is happening.

It is primarily happening here: https://github.com/gopherjs/vecty/blob/master/dom.go#L999-L1009

The behavior is the same regardless of which elem.Foo type you're using. If you are seeing otherwise that would be extremely surprising.

That said, I feel like this isn't necessarily a bug. It's undesirable but not a bug. It also means I have other code that I'm not sure how it's working correctly as I thought it was doing this same thing.

If you can share more cases like this that would be helpful.

Based on what I think I understand, you're getting performance gains by not calling the render() function and diffing the nodes; and I don't see how it could be changed without causing a performance penalty.

No, this has nothing to do with performance, actually. Here is a complete explanation:

func (*PageView) Render() vecty.ComponentOrHTML {
  return &SubView{}
}

type Subview struct {
    vecty.Core
    i int
}

func (s *Subview) Render() vecty.ComponentOrHTML {
    s.i++
    return vecty.Text(fmt.Sprint(s.i))
}

If we were to continuously ask vecty.Rerender(pageView) then looking at the code above you would expect i to continuously increase with each render. But, actually, we have the following:

• After the first render, a *Subview with i == 1
• After the second render, an entirely new *Subview instance with i == 1
• After the third render, yet an entirely new *Subview instance with i == 1

What Vecty can do is:

• Copy over fields using reflection, but this wouldn't solve the issue because i is not exported.
• Copy over the entire struct from the prior render to the new render, but this means that if e.g. a method of Subview spawns a goroutine to increment i it would then be operating on the wrong instance. -- also confusing!
• Copy over fields using reflection from the new render into the old render.

We chose the third option above, since it means we only keep around one instance of the component at a time and things generally just work as you'd expect!

However, we have since learned through this issue that there are still cases where this can be pretty confusing behavior (and I largely agree).

I'm going to keep this issue open to think about whether we can improve this some more, and if not at least find a way to clearly document this behavior.

[daniil388]

Hello. Think I have the same problem but source code of the first message is not available.

My example with global variables: https://gist.github.com/daniil388/f3689983006466572659f8aa12633d33

In console log printed correct global list each Render method. In browser I see the previous list but shorter.

What is wrong with this code? except global variables :)

[slimsag]

@daniil388 almost certainly the problem with your code here is the usage of those global variables. You must create component instances in the Render method, generally speaking, because Vecty expects functional programming basically. Most likely we just don’t panic on your code as we should