turbo: Duplicate content, Turbo stream added an already existing item

I think the jumping around is a feature not a bug. This would allow you to reorder things. If you need full control over the order of a list, you’ll need to render the entire container instead. But as a basic default, I’d like to see option C. We remove any duplicates, then do as we’re instructing.

In addition to that, we could introduce two new directives: update-or-append and update-or-prepend. Those would essentially do option B, but give you the control of whether to append or prepend on those that don’t match an existing ID.

Don’t append or prepend children with the same ID as existing ones

This doesn’t seem like a good idea to me, as the content of the ID might have changed and the updated content would not be added.

Remove target children with conflicting ID before adding template

This may cause page content to jump around. A developer might want to append or prepend an updated item at the beginning or end of a list, however considering this is mostly a race condition, I can’t imagine the content to be that stale and not already at the beginning or ending of a list.

Replace children with conflicting ID instead of append/prepend

I prefer this option over the other options.

I believe a cleaner version to my original code might be something like:

append() {
  this.replaceDuplicateChildren()
  this.targetElement?.append(this.templateContent)
}

replaceDuplicateChildren() {
  this.duplicateChildren.forEach((elementChild, templateChild) => {
    elementChild.replaceWith(templateChild);
    templateChild.remove();
  })
}

get duplicateChildren() {
  return [...this.templateContent.children].map(templateChild => (
    [this.targetElement?.getElementById(templateChild.id), templateChild]
  )).filter((elementChild, templateChild) => elementChild);
}

OK, I’m prepping a fix for this, but this regardless will break the tie between the action name (append/prepend) and the matching DOM functions so there are decisions to be made.

Here’s a contrived example to highlight the possibilities:

Assuming the following Target:

<ul id="vehicles">
  <li id="vehicle_1">Old car</li>
  <li id="vehicle_2">New car</li>
</ul>

Stream element:

<turbo-stream action="append" target="vehicles">
  <template>
      <li id="vehicle_1">Old car with updates</li>
      <li id="vehicle_3">Newer car</li>
  </template>
</turbo-stream>

With the current code, the result is:

<ul id="vehicles">
  <li id="vehicle_1">Old car</li>
  <li id="vehicle_2">New car</li>
  <li id="vehicle_1">Old car with updates</li>
  <li id="vehicle_3">Newer car</li>
</ul>

The duplicated id makes the html invalid (although it works), and append works like the DOM append function.

Would you change it to be: Option A: do not append template children with id matching target children id

<ul id="vehicles">
  <li id="vehicle_1">Old car</li>
  <li id="vehicle_2">New car</li>
  <li id="vehicle_3">Newer car</li>
</ul>

Option B: replace target children with the same id by template children, append the rest of the template (without the prior children)

<ul id="vehicles">
  <li id="vehicle_1">Old car with updates</li>
  <li id="vehicle_2">New car</li>
  <li id="vehicle_3">Newer car</li>
</ul>

Option C: remove target children with the same id as template children id, append template like in https://github.com/hotwired/turbo/issues/132#issuecomment-767552026

<ul id="vehicles">
  <li id="vehicle_2">New car</li>
  <li id="vehicle_1">Old car with updates</li>
  <li id="vehicle_3">Newer car</li>
</ul>

In the specific case we are trying to fix (duplicate append call with the same template containing one child), the 3 options will give the same result, but for more complicated scenarios we need to pick a behaviour.

We could also introduce another action type (maybe safe_append) if this breaks too much expectations of behaviour for append.

Any preference before I code this ? Sound like a few people in this thread have stated C), although this example may change that.

Definitely want to deal with this. One suggestion that @sstephenson had presented was that Turbo would find all the id’s in a turbo stream update, then remove all those elements from the DOM before adding what’s in the action.