vecty icon indicating copy to clipboard operation
vecty copied to clipboard

Applying markup with Text() panics often

Open bzub opened this issue 7 years ago • 6 comments

Using an Applyer that isn't compatable with a Text Node doesn't appear to be handled currently.

Simple example:

package main

import (
	"github.com/gopherjs/vecty"
	"github.com/gopherjs/vecty/elem"
)

type pageView struct {
	vecty.Core
}

func main() {
	vecty.RenderBody(&pageView{})
}

func (c *pageView) Render() vecty.ComponentOrHTML {
	return elem.Body(
		vecty.Text("hello",
			vecty.Markup(
				vecty.Attribute("test", true),
			),
		),
	)
}

Result:

TypeError: obj[$externalize(...)] is undefined

Code: https://github.com/gopherjs/vecty/blob/1d629507357c2e861fbc2b7cf1553c1cb0789399/dom.go#L1251

bzub avatar Apr 21 '18 16:04 bzub

Agreed, we should be producing a clean panic here and documenting this or refusing to accept vecty.Attribute here via the type-system instead.

vecty.Text is just a DOM TextNode, essentially, so this is not allowed. The function takes vecty.MarkupOrChild today but in reality you can't give it children (Component, *HTML, List, KeyedList, or nil) I think. You can only provide it with vecty.Markup and even then, I think only vecty.Key is actually valid (and even that may not be, I'm not sure). Not sure how we missed this.

WDYT @pdf ?

emidoots avatar Apr 22 '18 02:04 emidoots

I think I'd be fine with just changing the signature to vecty.Text(text string). Though vecty.Key might be valid here, I think it's fine to require users to wrap the text in a span or something if they want efficient re-ordering.

pdf avatar Apr 22 '18 03:04 pdf

Yeah, or we could always offer an vecty.KeyedText if it is really needed in the future.

👍 to just changing signature to vecty.Text(text string)

emidoots avatar Apr 22 '18 03:04 emidoots

Sounds good to me as well, to change the signature of Text().

There's still the issue of what to do when Apply(h) is called on a *HTML that's a text node. I make my own types that implement Applyer quite often. There's no way to tell in that situation if a *HTML is a text node (without being hacky). Perhaps we could add a (*HTML).IsText() method?

bzub avatar Apr 22 '18 20:04 bzub

@bzub Could you elaborate / provide examples of Applyers that you make yourself?

emidoots avatar Apr 23 '18 00:04 emidoots

I'll have a concrete example that I'm happy with in a few days, hopefully. So I'll elaborate with a simple example for now.

I've created types that satisfy both Component as well as Applyer, with the Component's outer-most-node's markup applied entirely by its corresponding Applyer + user supplied markup. This is to support web libraries that rely heavily on classes/attributes/properties for CSS/JS functionality. The users of my components can either use the components normally as a Component or they can use them as a "smart" Applyer in their own elements/components. The Applyer is smart because it may contain somewhat complex logic based on the values within the component's struct.

Going beyond this, in my experience the web library components I'm trying to implement in Vecty often have child elements that each need special markup applied as well. So my idea was to make each of those building blocks as types that satisfy Component and Applyer as well. That way it's as simple as possible to think about customizing individual pieces of a complex component, while still ensuring each peice gets the markup expected by the web library being used.

Here's an HTML example.

Here's a basic render example. The Applyer portion is missing but could be anything, you can see the component itself is passed to the Markup() function:

func (c *ApplyerComponent) Render() vecty.ComponentOrHTML {
	return elem.Div(
		vecty.Markup(
			c,
			c.UserSuppliedMarkup
		)
		// Children
		&ChildComponent{Label: "Child Component 1"},
		&ChildComponent{Label: "Child Component 2"},
	)
}

bzub avatar Apr 23 '18 04:04 bzub