dom icon indicating copy to clipboard operation
dom copied to clipboard

Suggestion: `Node.insertAfter(newNode, referenceNode)`

Open dead-claudia opened this issue 3 years ago β€’ 16 comments

In frameworks, it's far more common to want to insert after the current node than before the next node, and it's significantly easier to implement. Problem is, for performance reasons, some of us employ various hacks like this (example call) to avoid having to pay the cost of a C++ round trip just to get the next sibling of a given DOM node.

I also see a gaping hole in this matrix for child-relative operations:

Operation Node method ChildNode method jQuery method
Insert before node.insertBefore(child, ref) child.before(...nodes) $(parent).before(child), $(child).insertBefore(parent)
Insert after child.after(...nodes) $(parent).after(child), $(child).insertAfter(parent)
Remove node.removeChild(child) child.remove() $(child).remove()
Replace node.replaceChild(child, prev) child.replaceWith(...nodes) $(child).replaceWith(newChild), $(newChild).replaceAll(child)

I propose we fill that with the following method:

partial interface Node {
    [CEReactions] Node insertAfter(Node node, Node? child);
};

The semantics are pretty obvious:

  • If child is null, this prepends node as the first child.
  • If child is not null, this inserts node after child similar to how insertBefore inserts it before child.

With this primitive, a tree walk could then just do this (high-level pseudocode, omits most the irrelevant crap) on create:

global parent = null
global lastUpdated = null

render(root, tree):
    local prev = {parent, lastUpdated}
    parent = root
    lastUpdated = null
    updateTree(treeForRoot)
    {parent, lastUpdated} = prev

createElement(virtualNode):
    local elem = document.createElement(virtualNode.tagName)
    local prev = parent
    setAttributes(elem, virtualNode.attributes)
    parent = elem // Save parent node to build to
    for child of virtualNode.children:
        createChild(child)
    parent = prev
    parent.insertAfter(elem, lastUpdated)
    lastUpdated = elem
    return

We currently can't do that without a lot of branching and/or multiple C++ round trips - that was our mechanism prior to our current system, actually, and it proved to be an eventual bottleneck. And of course, this isn't unique to virtual DOM - it could apply to most frameworks overall. In general, it means us framework writers can get away with a lot less bookkeeping when incrementally building the DOM, so I would really love this.

As for why I'm not using child.after? We need the parent node because we won't have the child node until we visit one that already exists, and when we're creating a node after a fragment, we won't know it's the first child until we visit the entire fragment, and when we do, we might very well not have a child there to work from. And it's a lot more awkward to branch between parent.prepend(newChild) and child.after(newChild), and I suspect it'd also provide a performance cliff due to new CPU cache misses and a more complicated branch profile. Ideally, that conditional belongs in C++ land - it's already there for insertBefore. That and also I've already found C++ transitions to be toxic for performance just from personal experience elsewhere.

dead-claudia avatar Jun 09 '21 07:06 dead-claudia

Are there benchmarks for this? @smaug---- @rniwa thoughts?

annevk avatar Jun 14 '21 07:06 annevk

It seems okay to add this assuming there is no web compatibility issue. It looks like there is at least one (semi?) popular library which implements insertAfter so someone needs to assess the web compatibility risk.

rniwa avatar Jun 14 '21 07:06 rniwa

If child is null, this prepends node as the first child.

with insertBefore, if child is null the element is appended (like appendChild would do) ... I find this proposed behavior here a bit controversial ... the before "appends with null, the after "prepend" with null as first child ... am I the only one not fully getting the use case for prepending as first child with a method called after ?

WebReflection avatar Jun 14 '21 07:06 WebReflection

Are there benchmarks for this? @smaug---- @rniwa thoughts?

@annevk I don't have any benchmarks on the ready as this was years ago (though little's changed since). The main thing is C++ showing up high on profiles, and of course I want to avoid that. And .insertAfter is a bit simpler natively than in userland, or I'd use it:

function insertAfter(parent, child, refNode) {
    parent.insertBefore(child, refNode != null ? refNode.nextSibling : parent.firstChild)
}

It seems okay to add this assuming there is no web compatibility issue. It looks like there is at least one (semi?) popular library which implements insertAfter so someone needs to assess the web compatibility risk.

@rniwa Not sure it's that significant of a risk when they also overwrite Element#insertBefore in exactly the same way.

dead-claudia avatar Jun 14 '21 07:06 dead-claudia

@isiahmeadows thanks for clarifying, then maybe the proposal text should be updated ... but to be sure we're on the same page:

<ul id="list">
  <li>first task</li>
  <!-- some comment -->
</ul>

Assuming a new item such as:

const task = document.createElement('li');
task.textContent = 'last task';

list.insertAfter(task, null)

would produce:

<ul id="list">
  <li>first task</li>
  <!-- some comment -->
  <li>last task</li>
</ul>

while ...

list.insertAfter(task, list.lastElementChild)

would produce:

<ul id="list">
  <li>first task</li>
  <li>last task</li>
  <!-- some comment -->
</ul>

did I get it right?

WebReflection avatar Jun 14 '21 07:06 WebReflection

If child is null, this prepends node as the first child.

with insertBefore, if child is null the element is appended (like appendChild would do) ... I find this proposed behavior here a bit controversial ... the before "appends with null, the after "prepend" with null as first child ... am I the only one not fully getting the use case for prepending as first child with a method called after ?

@WebReflection It's based on the semantics of elem.insertBefore

  • elem.insertBefore(node, null) appends because in essence, it's just seeking forward from the start to the position within the element's children right before the reference node (without skipping past it) and inserting there. Absent a reference node, it just seeks to the end (where it can't seek further), thus appending.
  • elem.insertAfter(node, null) I'm proposing to prepend because of the same reason, just in the opposite direction and starting from the opposite end. It seeks back from the start to the position within the element's children right after the reference node (as in, it likewise doesn't skip past it) and inserting the node there. Absent a reference node, it just seeks to the start (where it can't seek further), thus prepending.

Of course, it's not spec'd like this (nobody with any sense is going to implement it that way), but that's the general idea.

Separately, in virtual DOM frameworks, in the face of fragments, we literally don't know what the next sibling will be, and so it's faster for us to recompute it each time from our internal model.

dead-claudia avatar Jun 14 '21 07:06 dead-claudia

@webreflection Had to recast my comment because I misinterpreted your response. Please see my updated comment. πŸ€¦β€β™€οΈ

dead-claudia avatar Jun 14 '21 07:06 dead-claudia

Are there benchmarks for this? @smaug---- @rniwa thoughts?

@annevk As for the cost of C++, consider this diff. We got serious gains from two things:

  1. Avoiding a polymorphic property lookup by performing a polymorphic type check on the value first (counterintuitive, but it made a noticeable difference).
  2. Avoiding a long series of unnecessary calls to C++ land and reducing it to a dictionary property update.

And that's for a reasonably simple pair of algorithms. (The date of that diff should give you an idea how long ago I last ran those benchmarks, but I've watched the situation on JS perf, and little has changed in this area - I can tell you that for certain.)

dead-claudia avatar Jun 14 '21 07:06 dead-claudia

@isiahmeadows so ... considering this previous scenario:

list.insertAfter(task, null)

would produce:

<ul id="list">
  <li>last task</li>
  <li>first task</li>
  <!-- some comment -->
</ul>

I understand the specular API idea behind, but I find it a bit counter-intuitive ... anyway, not opposing, or anything, just wantet to understand the rationale or use case. I don't use vDOM, but I do use DOM diffing, so these methods might be handy for my use cases too.

Polyfill

I guess it would boil down to this?

Element.prototype.insertAfter = function insertAfter(node, reference) {
  return this.insertBefore(node, reference ? reference.nextSibling : this.firstChild);
};

WebReflection avatar Jun 14 '21 07:06 WebReflection

@WebReflection That is correct. The idea is that you're inserting a node, but specifically after nothing. And if nothing's supposed to be before it, then you're essentially prepending it. And so yes, that's the intent.

  • elem.insertAfter(node, null) β†’ elem.prepend(node)
  • elem.insertAfter(node, refNode) β†’ refNode.after(node)
  • elem.insertAfter(node, elem.lastChild) β†’ elem.append(node)

dead-claudia avatar Jun 14 '21 07:06 dead-claudia

@isiahmeadows thanks. If you can confirm the polyfill reflects your proposal, I might publish it already under @ungap + I don't see it problematic or much slower than what insertBefore could be already. I'll try to reach out Dmitry too for thoughts around breaking changes.

WebReflection avatar Jun 14 '21 08:06 WebReflection

2\. Avoiding a long series of unnecessary calls to C++ land and reducing it to a dictionary property update.

So this is only about the case when one wants to prepend the very first node. Otherwise using child.after() gives exactly the same performance.

But I guess insertAfter should be fine.

smaug---- avatar Jun 14 '21 12:06 smaug----

@smaug---- That's correct, though the code I was referencing was for event listener patching (similar, but slightly different). I was just showing that as an example of the costs we've incurred from JS ↔ native overhead, and a big part of the driving reason for why I want a merged operation for this (why it's not just a nice-to-have).

dead-claudia avatar Jun 14 '21 20:06 dead-claudia

FWIW, one semipopular library doesn’t overwrite any native methods. Element in the code is custom object, not a native HTMLElement or similar. No problems with this method introduction. πŸ‘

DmitryBaranovskiy avatar Jun 15 '21 03:06 DmitryBaranovskiy

Thanks everyone for weighing in. It seems like the next steps here would be a PR to change the DOM Standard as well as corresponding tests in the web-platform-tests project.

annevk avatar Jun 16 '21 07:06 annevk

FWIWI I've published @ungap/insert-after polyfill already, in case it's needed for tests or something.

WebReflection avatar Jun 16 '21 11:06 WebReflection