:bug: After iterating, clean up fragment elements that will be unused #160

JScearcy commented 3 months ago

Summary

When using a top level fragment such as:

fn view(model: Model) -> Element(Msg) {
  element.fragment([
    // html.div([], [
    ui.button([event.on_click(AddRow)], [element.text("Add")]),
    html.table([], [
      html.thead([], [
        html.tr([], [
          html.td([], [element.text("id")]),
          html.td([], [element.text("name")]),
        ]),
      ]),
    ]),
    element.fragment(list.map(model.rows, render_row)),
  ])
}

The reflected DOM will include some number of rows. On re-render, if the number of rows is less than the previous instance today the vdom will not clean up the hanging elements after re-rendering the rows that should exist. After iterating through a top-level fragment, all previous siblings have been accounted for in this new iteration, so in order to clean up superfluous elements, remove all until prev is not truthy. This resolves the issue of handing elements.

TODO: Add images

	Before	After

hayleigh-dot-dev commented 3 months ago

Would this cause problems with not-top-level fragments removing elements only for them to be re-made again?

all previous siblings have been accounted for in this new iteration, so in order to clean up superfluous elements, remove all until prev is not truthy.

This could lead to lustre removing parts of the DOM it's not supposed to own. Consider the following HTML:

<body>
  <div id="app" />
  <aside id="some-third-party-chat-widget" />
</body>

If we return a top-level fragment the resulting DOM will look like:

<body>
  <button>...</button>
  <table>...</table>
  <aside id="some-third-party-chat-widget" />
</body>

This is because lustre replaces the target element not mounts into it. If we start nuking any remaining children we're going to delete that <aside> and our users will probably have a bad time.

I see two ways forward, of varying amounts of work:

Render into the root node rather than onto it. This means if we have some <div id="app"> as our root node, then all the lustre application elements are inside this element.
Render comment nodes around the beginning and end of a fragment. The vdom patching would use these special comment nodes to fence fragments, then it knows when a fragment ends and when it finishes and can safely remove any remaining elements up to that end.

What do you think?

JScearcy commented 3 months ago

Would this cause problems with not-top-level fragments removing elements only for them to be re-made again?

No I don't think so - this case is only possible for a top level fragment, otherwise it's handled as a child of an actual Element so I think in all other cases we have the safety of an Element wrapping and cleaning up dangling children - in the example I had to make sure the rows were siblings with the table, otherwise the rows properly matched the model

This is because lustre replaces the target element not mounts into it. If we start nuking any remaining children we're going to delete that \<aside> and our users will probably have a bad time.

Yeah you're right, that could easily happen, I only thought through the full document ownership so that'll be a problem otherwise

I see two ways forward, of varying amounts of work:

I think the latter may be more work but seems preferable, rather than always needing some wrapping component. I'll circle back on this (first for top-level fragments, then port to all)

JScearcy commented 2 months ago

Ooops, I was commenting and closed it. Fencing the top level fragment is implemented, now cleaning up handing elements

hayleigh-dot-dev commented 3 weeks ago

Closing this as we've moved to a native approach to fragments instead of requiring special runtime support, thanks for the effort anyway 💕

lustre-labs / lustre

:bug: After iterating, clean up fragment elements that will be unused #160 #174

Summary