Does the Render type have the parameter order wrong?

[TastyPi]

I'm not sure if I'm just misunderstanding something or if they are actually the wrong way round, but it took me a while to figure out what the issue was so this at least might help someone figure it out. The Render type is defined as (newElement: E, currentElement: E) => void, but it seems like it should be (currentElement: E, newElement: E) => void. Let me explain.

For background: We use Front's live chat widget, and their library adds an iframe to the body than can never be reloaded. This means we need to keep the same body element as well. The original workaround we tried used morphdom, but then we ran into this issue of Stimulus controllers not reloading. So we decided to implement a custom renderer that keeps the body and iframe but copies everything else across.

This is the first attempt:

  event.detail.render = (newElement, currentElement) => {
    // Remove all the old elements except the Front iframe
    Array.from(currentElement.children).forEach((element) => {
      if (element.id !== FRONT_ELEMENT_ID) {
        element.remove()
      }
    })

    // Move the new elements across
    Array.from(newElement.children).forEach((element) => currentElement.appendChild(element))
  }

This doesn't work, it looks like it just removes all the children without copying the new ones across. I assumed the newElement was the newly rendered body and the currentElement was the body currently in the DOM. Turns out they're the other way round, and swapping the parameter order made it start working - it looked like it was just removing elements because the elements were being moved the wrong way.

The naming of these parameters is very confusing to me, how could the existing DOM be considered the newElement? Hence this issue, are the parameters named the wrong way round or am I misunderstanding the meaning of the terms new and current?

[TastyPi]

@seanpdoyle who introduced the Render type in #431

[TastyPi]

It looks like it should be defined (currentElement, newElement)

turbo/src/core/drive/page_renderer.ts

Line 161 in 0f3632a

this.renderElement(this.currentElement, this.newElement)

[seanpdoyle]

@TastyPi thank you for opening this issue.

Yes, the order of the arguments in the Render signature are incorrect, and were introduced in that incorrect order:

https://github.com/hotwired/turbo/pull/431/files#diff-d554b20c605a72c68b2d429ff9915cca4afa8e118026f56ede93382d130382a7R11

Luckily, the default implementations and the tests that exercise that feature are in the correct order.

The signature's argument order was inspired by morphdom's, with the latter argument serving as overrides for the prior argument.

Fortunately (and unfortunately!), custom rendering isn't yet covered by Turbo's documentation website, so we have an opportunity to document it thoroughly and correctly.

In the meantime, I'll open a pull request to swap the order of the arguments for TypeScript's sake.

[seanpdoyle]

I've opened #814 to resolve this.

[TastyPi]

Some documentation would be great, I've just found that my custom rendering logic breaks back/forward navigation and I have no idea why.