Writing extensible elements is harder than it should be

[slimsag]

The scenario: I want to create something like the elem subpackage but with more sane default values than what the browser provides. For example, buttons would be styled more nicely than the browser defaults, etc. And of course, I want the caller to be able to override these 'sane defaults' they should they want to.

Basically, I want something with the same exact signature as elem.Button, just with some default vecty.Markup. For example, it might have a default black border style, and the caller could overwrite it by calling it like this:

Button(
    vecty.Markup(
        vecty.Style("border": "99px solid gold"),
    ),
)

---

This is possible today, but the code isn't that pretty or straightforward:

func Button(markup ...vecty.MarkupOrChild) *vecty.HTML {
    return elem.Button(
        append([]vecty.MarkupOrChild{
            vecty.Markup(
                vecty.Style("border": "1px solid black"),
            ),
        }, markup...)
    )
}

I wrote this in my first attempt, but it doesn't compile / work:

func Button(content ...vecty.MarkupOrChild) *vecty.HTML {
    return elem.Button(
        vecty.Markup(
            vecty.Style("border": "1px solid black"),
        ),
        content...,
    )
}

This is because vecty.Markup is MarkupList and content is []vecty.MarkupOrChild, but elem.Button is a variadic ...vecty.MarkupOrChild and variadic arguments in Go only accept parameters of the same type.

I'm not saying that we even can do anything to make the above easier to write -- but this issue is about asking the question: can we?

Related to #149

[pdf]

Is this a little better?

func Button(content ...vecty.MarkupOrChild) *vecty.HTML {
    return elem.Button(
        vecty.Markup(
            vecty.Style("border": "1px solid black"),
        ).(vecty.MarkupOrChild),
        content...,
    )
}

EDIT: Actually, that won't work either - the problem is that you can't mix variadic expansion with distinct arguments. I don't believe there's a way around this - it's a language constraint.

[slimsag]

Oh interesting, I did not think of that. Indeed, that's a bit better!

---

I am also wondering what drawbacks there would be to:

func Tag(tag string, children ...vecty.ComponentOrCHild) *vecty.HTML
...
func (h *HTML) WithMarkup(markup ...Applyer) *HTML
...
Tag("button", children...).WithMarkup(markup...)

A custom Button signature then becomes:

func Button(children ...vecty.ComponentOrChild) *vecty.HTML {
    return elem.Button(children...).WithMarkup(
        vecty.Style("border": "1px solid black"),
        )
}

[pdf]

Yeah, I was actually thinking about this in relation to #78, because if we allow *HTML references, the current API provides no way to modify them. Moving to a chaining API would allow this. The question would be whether or not we force/allow children to be declared in this way too. Illustration:

type MyComponent struct {
    vecty.Core
    elem *vecty.HTML
}

func (c *MyComponent) Mount() {
    // do something with c.elem reference
}

func (c *MyComponent) Render() vecty.ComponentOrHTML {
    if c.elem == nil {
        c.elem = elem.Span(vecty.Text("initialText")).WithMarkup(vecty.ClassMap{"initial": true})
    } else {
        c.elem.WithMarkup(vecty.ClassMap{"initial": false})
    }
    return c.elem
}

So, this provides a means to modify markup, but not children. We could potentially expose .Children(...vecty.ComponentOrHTML) on *HTML, but still allow the current syntax for declaring children as args to the elem func, if this is a use-case we'd like to support.

[slimsag]

Interesting ideas. I think that in general it is better to strive to be functional in nature and avoid mutability where possible, because more state == harder to reason about (generally speaking).

So my initial impression would be that we should either try to enforce that as being impossible, or more likely, just suggest people don't do it. If we expose .Children(...) on *HTML I think that we may open a new can of worms: Can I get the current children to modify their markup, instead of creating new ones? Can I get the *HTML tag name, event handlers, etc?

[pdf]

I don't think we can entirely avoid the need for JS lib interop, which is really what the above is about. If we have .WithMarkup() and allow *HTML refs, we need to handle this anyway (it's as simple as keeping a copy of the previous *HTML render, similar to how components are handled, I think).

[pdf]

Can I get the current children to modify their markup, instead of creating new ones?

Not unless you retain your own reference, they'll get reconciled as normal either way though.

Can I get the *HTML tag name, event handlers, etc?

I think not.

[slimsag]

I don't think we can entirely avoid the need for JS lib interop

I completely agree with this. in fact, I don't think we should try to avoid JS lib interop at all -- I think we should embrace it and strive to offer better JS lib interop than what we have today. I don't know what this would entail yet, though, so I haven't created any specific/concrete issues around it yet.

which is really what the above is about.

This part of your message I am really confused about. Are you saying that WithChildren would somehow help JS lib interoperability? Could you elaborate on this (I genuinely do not see how it would help with JS interop, since currently you can always just store a reference to an element and return that as a child without needing WithChildren).

If we have .WithMarkup() and allow HTML refs, we need to handle this anyway (it's as simple as keeping a copy of the previous HTML render, similar to how components are handled, I think).

I don't think WithMarkup would need to keep a copy of any previous render around. It would just copy it's own *HTML, and overwrite any markup by applying each of them in order and return it as a pointer. I think it would be as simple as:

func (h *HTML) WithMarkup(markup ...Applyer) *HTML {
    cpy := *h
    for _, m := range markup {
        m.Apply(&cpy)
    }
    return &cpy
}

[slimsag]

I guess what you are saying may be that, currently it is not possible to get a reference to the DOM node for the element that your component returns from Render.

i.e. you can get a reference if it's a child that your component returns, but not if it's the main element your component returns.

In that case, I see what your argument for WithChildren is about now

[pdf]

Are you saying that WithChildren would somehow help JS lib interoperability?

Without WithChildren, you can't update the children of an element that you need to retain a reference for, after the initial render.

I don't think WithMarkup would need to keep a copy of any previous render around.

I'll need to have a think about this some more to determine whether this works for all cases.

[slimsag]

Ok, so for now I think we will absolutely have a WithChildren and WithMarkup specifically for JS interop ala #78 -- just wanted to state that & say that we can move that discussion over there.

For this specific issue, though, I think WithMarkup is not a good idea (see my comments in #172).

---

As I mentioned last in #172, I'm reconsidering what nil means to us and I think I may have found an obvious solution that would work -- and would make what we did in #134 look quite silly.

I recall back specifically to your message here https://github.com/gopherjs/vecty/pull/134/files#r135373959 which basically talked about how if we had markup and children sitting side-by-side with each other in a call to e.g. elem.Div that we don't have a way to tell if nil is a nil child or nil markup.

func (c *MyComponent) Render() *vecty.HTML {
        return elem.Div(
                c.OptionalMarkup(), // breaks persistence
                &OtherComponent{},
        )
}

Specifically because when c.OptionalMarkup() != nil, we know it is markup, but when it is nil we don't know if it is markup or a child.

The issue here is that when we see nil we implicitly treat it as a nil child, and so if it was markup previously it would now become a child and shift every element by one -- which messes up both keyed and unkeyed children and causes all of them past that nil to be recreated.

My current line of thinking is: We store the previous render, why don't we look at it to determine whether or not that nil was markup or a child? Based on the prior render we can determine whether or not it consumes a child slot.

The benefits of this are obvious: A much cleaner API as we had before, and it would fix this issue because we could merge ComponentOrHTML and MarkupOrChild.

I thought extensively throughout today and yesterday about the drawbacks, and what I came up with was that:

1. It is semi-tricky to implement. But I found this to not be true because I think we can actually just have a pre-emptive step in which we resolve nil in these lists into a child-slot-consuming-nil or not, and then proceed with the same algorithm we have today effectively. e.g. something that takes the prev render and next render and inserts empty MarkupList when there is a nil in the same slot as the previous render's markup.
2. It might not work as well for lists whose size is changing. I haven't completely thought this part through yet.

To explain in terms of the problematic example snippet above:

return elem.Div(
    c.OptionalMarkup(),
    elem.Header1(`header`),
)

The first render observed may be:

firstRender := elem.Div(
    c.OptionalMarkup(), // != nil
    elem.Header1(`header`),
)

And the second render:

secondRender := elem.Div(
    nil, // we see in the same position of last render that this was the markup type, so we know it doesn't consume a child slot.
    elem.Header1(`header`),
)

For cases where there are no keys, I think this is exactly the same behavior we have today. i.e. if the number of elements here changes size, then things become unordered and all bets are basically off (you can expect many elements to be recreated).

Now for cases where there are keys, the problem is a bit trickier to think about (at least for me, it is). I'm still thinking about it.

[pdf]

My current line of thinking is: We store the previous render, why don't we look at it to determine whether or not that nil was markup or a child? Based on the prior render we can determine whether or not it consumes a child slot.

If slot is nil for first render, you have an indeterminate state.

Ok, so for now I think we will absolutely have a WithChildren and WithMarkup specifically for JS interop ala #78 -- just wanted to state that & say that we can move that discussion over there.

Something to consider: if only the chaining API was available, each method would only accept a specific type, and there are no untyped nil considerations to worry about any more.

[slimsag]

If slot is nil for first render, you have an indeterminate state.

I don't think it would be indeterminate; it would depend on what we now have. To give a concrete example of what I am proposing here, we would store the arguments passed to Tag directly in *HTML and prior to operating on that list we would run it through a function. That function would determine whether or not nil is child-consuming or not based on the previous render. The exact logic would be like this:

Prev render slot	New render slot	Slot type	description
nil	non-nil markup	not-child	we know it's not a child because we now have markup
nil	non-nil component	child	we know it's a child because we now have a child
non-nil markup	nil	not-child	we know it's not a child because we previously had markup
non-nil component	nil	child	we know it's a child because we previously had a child
nil	nil	unknown	we don't know, but it also doesn't matter (we ignore the slot)
non-nil markup	non-nil markup	not-child	we know it's not a child because it's our only option
non-nil component	non-nil component	child	we know it's a child because it's our only option

[slimsag]

A more simple thought: we could do as you originally proposed and outright ban / panic on nil.. don't know why I didn't think about zero-value struct values being a good option:

// Used to be named Applyer. Not a pointer type, so it can't be `nil` ever. `Markup{}` is the zero value.
type Markup struct {
    apply func(h HTML)
}

// Not a pointer type, so it can't be `nil` ever. `HTML{}` is the zero value.
type HTML struct {
    *html // same as *HTML before this change
}

[pdf]

I'm afraid I'm really short on bandwidth at the moment, I won't have time to give this the thought it needs for at least a couple of weeks.

[pdf]

If you could explain how this helps with the original problem described in this issue, that would be helpful though.

[slimsag]

Totally understand being short on bandwidth. Starting tomorrow I also will not have any time for OSS /Vecty until around the second week of December. So no rush at all. :)

---

If you could explain how this helps with the original problem described in this issue, that would be helpful though.

Absolutely. So, per the issue description here, writing this is incredibly clunky and I'd like for it to be more simple:

func Button(markup ...vecty.MarkupOrChild) *vecty.HTML {
    return elem.Button(
        append([]vecty.MarkupOrChild{
            vecty.Markup(
                vecty.Style("border": "1px solid black"),
            ),
        }, markup...)
    )
}

If we were to do what I last proposed here (https://github.com/gopherjs/vecty/issues/171#issuecomment-343781568), which I now believe to be the best option, then I could just write:

func Button(markup ...vecty.MarkupOrChild) vecty.HTML {
    return elem.Button(
        vecty.Style("border": "1px solid black"),
        markup...,
    )
}

Which I believe is the most ideal API overall.

---

The side effects of this are that:

1. The Button API signature now returns vecty.HTML instead of *vecty.HTML, and passing in nil is now illegal / causes a panic.
2. Passing a zero value vecty.HTML{} or vecty.Markup{} is now the equivalent to nil, except it's actually typed so we know what it is.

The other nice benefits of this which I like are:

1. We can get rid of vecty.Markup which I have seen some users question/complain about ("what does this do, why can't I specify vecty.Style without vecty.Markup, etc).
2. We could get rid of vecty.MarkupIf and go back to a single vecty.If -- if we were willing to use reflection to return an empty vecty.HTML{} or vecty.Markup{} depending on the input type.
3. We can get rid of vecty.ComponentOrHTML and just expose vecty.MarkupOrChild publically.

[pdf]

You can't mix discrete arguments and variadic expansion:

elem.Button(vecty.Style(), markup...)

will fail to compile with too many arguments, no?

[slimsag]

You're 100% right. This will not solve this issue (but we may want to do what I said for the other benefits regardless).

---

I have been an idiot here and didn't actually verify that the solution I was thinking of would solve the real issue at hand -- and I now see your original reply edit:

EDIT: Actually, that won't work either - the problem is that you can't mix variadic expansion with distinct arguments. I don't believe there's a way around this - it's a language constraint.

which is 100% right.

So the only way to solve this issue would be to adopt something like the WithMarkup API. I'll think about this more.

(But I would still like to eventually hear your thoughts about my above proposal with respect to the other benefits I mentioned).

[slimsag]

func Button(markup ...vecty.MarkupOrChild) vecty.HTML {
    return elem.Button(
        vecty.Style("border": "1px solid black"),
        markup,
    )
}

Without variadic expansion, I wonder?

[pdf]

I'm afraid my bandwidth is still really low right now, I will try to find some time to onboard this and #178 (pretty excited about this) in the near future though.

[slimsag]

Again, no worries! I'm once again offline for about a week (or maybe four..) :) I just had some spare cycles to do all this today.

[divan]

I'm solving this problem for now by simply abandoning variadic parameters. In most of my cases, I just need to wrap some element or add classes.

func Button(content vecty.MarkupOrChild) *vecty.HTML {
    return elem.Button(
        vecty.Markup(
            vecty.Style("border": "1px solid black"),
        ),
        content,
    )
}

...

Button(
    vecty.Div(/*..can group elements here if I need to...*/),
)

[dave]

FWIW, this is how I did re-usable components (to create modal popups) in https://play.jsgo.io/ - Modal, and AddFileModal, LoadPackageModal etc.

It didn't exactly feel fluent, but worked OK.

[goozp]

Any new ideas about this disccussion? My current solution is to wrap another layer on the outside by elem.Div(), which is actually weird.

I saw the implementation of Modal above, and it seems to be wrapped in a div too finally.

I just used Vecty recently, and I feel that this question is very important, especially if you want to do some UI encapsulation.

[slimsag]

Just to be clear, you can do this today without wrapping in a div or anything. It is mentioned in the issue description:

func Button(markup ...vecty.MarkupOrChild) *vecty.HTML {
    return elem.Button(
        append([]vecty.MarkupOrChild{
            vecty.Markup(
                vecty.Style("border": "1px solid black"),
            ),
        }, markup...)
    )
}

The above creates a function which produces a <button> element with some markup applied - and the returned element is a button not a button wrapped in a div.

Is it pretty? No. Do I want Vecty to support better here? Definitely - it will take some time and investment to get there though :)