Defer doRender until after value of prevRender is taken

[dave]

Hey @slimsag one more bug... if h.children[i] == prev.children[i], then doRender will reset the prevRender of prevChild, and prevRender.Node will be nil.

This fixes it by deferring doRender until after we've taken the value of prevRender.

However, this worries me... What happens if the list order is mixed up and doRender clears the prevRender.Node of a component somewhere later in the prev children... Perhaps we need to read the prevRender values from all the children before doing any doRenders? I'll try to set up this situation to double check it's an issue.

[slimsag]

Good catch. Thank you for sending this. This just highlights, for me, how much I need to write tests for all standard functionality.

However, this worries me... What happens if the list order is mixed up and doRender clears the prevRender.Node of a component somewhere later in the prev children... Perhaps we need to read the prevRender values from all the children before doing any doRenders? I'll try to set up this situation to double check it's an issue.

It's possible I don't clearly understand the problem you're describing, but I think that currently this situation would not make a difference because of // TODO better list element reuse. i.e. today if list element order is mixed up we will always recreate elements from scratch anyway. Obviously this is still something to fix, but perhaps not as pressing / not a recent regression?

[slimsag]

(I'm going to merge because I absolutely think this fix is right, and want it in master ASAP. Let's continue to discuss here or in a new issue if you think it needs more visibility 😄 )

[dave]

Hey I think this was actually caused by a bug in my code. I was passing in a component to the view constructor and storing it in the view... So the constructor for the child component wasn't being called on the parent Render... A bit like this:

type PanelView struct {
    vecty.Core
    contents []vecty.MarkupOrComponentOrHTML
}
func NewPanel(contents ...vecty.MarkupOrComponentOrHTML) {
    return &PanelView{contents: contents}
}
func (v *PanelView) Render() *vecty.HTML {
    return elem.Div(contents...)
}

This is wrong. The constructor for the contents components must be called each time PanelView.Render is called. I can get round it by passing in a closure. It's not as neat, but it should fix the problem.

I'm pretty sure this is the only reason that h.children[i] == prev.children[i]... I haven't finished off the fix yet, but I'm confident. The last 24 hours I've been working from cafes and airports, so haven't been able to give it my full attention!

However, this PR won't break anything, so it should be safe to leave merged for now.

[slimsag]

Oh wow. I sat here for maybe ~20 mins trying to read over your last comment and understand the situation at play here. Couldn't make any sense of how this change could be wrong.

I see now that I severely misunderstood this change. I should've read more into h.children[i] == prev.children[i].. that should be an impossible case in all situations I think.

I'm going to think more about this, just to make sure I'm not doubly confused, but I think that if any new render is equal to the prev render, we should perhaps panic as that is quite the invalid state to be in. Hmm.. we should probably revert as well

[dave]

Yes, it should be impossible, so I think I agree with your panic idea... It certainly would have made my debugging quicker...

[slimsag]

@davelondon Very late with following up on this, but I've just reverted this change + added in the panic. Let me know if you have other thoughts on this / can confirm it looks correct!