Open progrium opened 6 years ago
That's certainly a fair question @progrium - take a browse through #78 and all the associated issues. Short answer is: some time ago, allowing this caused for some subtle and non-obvious breakages, however that's suspected to have bene fixed, but not adequately tested to be certain.
Huh. Quite a while ago. I guess you were running into it as well, what are you doing in the meantime?
My primary case was around reusing components rather than HTML, but we did a whole lot of work after I lodged that, so the renderer has changed significantly since then. Components are now reusable, but that panic persists for HTML, mainly because we've not had enough time to adequately test that it is safe to remove it now (I know I've personally had little time in the past year or so to contribute, and I suspect it has been similarly difficult for slimsag). As I mentioned in https://github.com/gopherjs/vecty/pull/124#issuecomment-312419500 I think it may be safe to remove now though.
I mean, I can write tests. I just need to know what I'm looking for. I don't understand the failure cases it's there for. Honestly, if we can't articulate it, I'd say remove it until we can. And then fix/write tests for that.
The original description is in #70 but it's not the easiest conversation to parse at this remove.
Dredging my memory of about a year ago, I think what we're concerned with here is that if an instance of *HTML
persists between renders somehow, that we correctly render all modifications to that instance and its children in all standard render scenarios. This would include all rendered properties/attributes/children/event-handlers/etc, and scenarios like: being at the root of a component render; nested under the root at x,y,z levels of nesting, of both Components and HTML; as a vecty.List
member; when reordered via keys; when replacing one element with another; etc. I unfortunately don't have an exhaustive list off the top of my head though, and I'd have to spend some time reacquainting myself with the code to be certain exactly what's required.
If there are bugs that would be exercised by hitting this condition without triggering the panic, I suspect they will exhibit as elements not updating their rendered output as expected during rerender.
If my recollections are incorrect, hopefully slimsag will chime in to correct me.
One workaround I found for this exact check is to accept a function that returns vecty.ComponentOrHTML
instead of just the component itself. Then, in the Render
function of the component, call that function to produce new children.
Example:
type Box struct {
vecty.Core
Children func() vecty.ComponentOrHTML `vecty:"slot"`
}
func (b *Box) Render() vecty.ComponentOrHTML {
return elem.Div(
vecty.Markup(
vecty.Style("position", "relative"),
vecty.Style("background-color", "#ffc0c0"),
vecty.Style("width", "100%"),
vecty.Style("height", "100%"),
),
elem.Div(
vecty.Text("Hello world"),
b.Children(),
),
)
}
As far as I have seen, this persists the state of the children component, so new structs can safely be constructed upon each rerender.
See https://github.com/Gelio/go-js-diagram/blob/02918eb210f204a8b5db3fea7f8d28792581480c/pkg/components/box.go#L19 for a more real-life example (a draggable box component)
I've been adding slots to my template system for Vecty so you can do this:
used like this:
where Example might have a template like this:
The result would be:
And this works fine without really any changes to Vecty which is great, HOWEVER when I do rerenders of Example I run into this error: https://github.com/gopherjs/vecty/blob/master/dom.go#L508-L510
And I've tried doing shallow copies of Children elements and I'm sure I'm just not knowing how it all works well enough to do this right, but the only way I can get it to work so far is to remove that check. And everything seems to work fine. So I'm wondering why it's there? This might also help me figure out how to do this the right way.