Open domenic opened 6 years ago
Not having the top-level TemplatePart
interface seems fine.
I do like the idea of being able to iterate over all the parts and just assign value
however. In fact, I wanted to make value
setter do the same right thing based on whether it's true
/ false
instead of having a separate booleanValue
. I didn't do it simply because there wasn't a way to express it in WebIDL. However, you can't really assign a string to InnerTemplatePart
anyway, this might be already a moot point. In that world, it's probably better to have an explicit add/remove
methods on AttributeTemplatePart
instead.
We're not tied to just having perviousSibling
or nextSibling
. It's just that we do need some API to know where those template parts belong, and those two properties felt the simplest one we could add.
My original thought was that we didn't want to use live NodeList
but in this case, the liveness is cheap (e.g. mutating DOM tree doesn't have to invalidate this list) so we can just use [SameObject]
instead. Using FrozenArray<Node>
or sequence<Node>
would be fine as well. We're not tied to the specifics of this API.
Sure, I think just having textContent
, replaceWith
, and replaceWithHTML
would be good. We don't want to use innerHTML
since it gets confusing with the semantics of outerHTML
(the semantics of replaceWithHTML is closer to outerHTML although the template part is never really in a DOM tree).
I have a few minor issues with the template parts APIs:
Shared
value
getter/setter outweigh the confusion.Attr
s have values, so it seems OK there, but for NodeTemplatePart, it seems rather arbitrary that you picked textContent semantics. I'd prefer to only have it for AttributeTemplatePart, and have more specific setters for NodeTemplatePart (see below).TemplatePart
base class at all; just a mixin might suffice. I don't feel strongly, but I'd like to hear more about what you think inheritance gives here.Attribute parts
booleanValue
is not good name. It's a HTML convention that presence/absence is treated as "boolean". A quick fix would be to change this toattributePresent
or similar. But, see next point.foo={{bar}}
should generate one type of template part, wherebar
is always interpreted as a boolean and controls presence/absence, whereasfoo="{{bar}}"
generates a different type of template part wherebar
is interpreted as a string? This latter idea seems pretty nice to me.Node parts
ContainerNode
is not a thing. I think it should just beNode
.previousSibling
/nextSibling
but not all the other traversal and manipulation methods. Maybe it's OK, but right now I feel the choice is a bit arbitrary, and would appreciate convincing that this is the correct subset ofNode
to include.replacementNodes
being[NewObject]
is very bad. It should be a method, if it's going to return a newNodeList
every time. I'm not sure why it has to return a new object every time though? Can't it only return a new object when they change?replacementNodes
should probably return aFrozenArray<Node>
instead of aNodeList
. Or, if it becomes a method, just asequence<Node>
.value
setter for textContent behavior,replace()
forreplaceWith()
behavior, andreplaceHTML()
forinnerHTML
behavior) is strange to me. I'd prefer to use the conventional names (textContent
,replaceWith()
, andinnerHTML
). I'm also not convinced these are exactly the right choices: I think justreplaceWith()
would suffice, althoughinnerHTML
is convenient (since otherwise you need to do the template dance to create a suitableDocumentFragment
to pass toreplaceWith()
).