Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

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

wants to merge 1 commit into from

Conversation

JScearcy
Copy link
Collaborator

@JScearcy JScearcy commented Aug 18, 2024 •
edited
Loading

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
Copy link
Collaborator

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
Copy link
Collaborator Author

JScearcy commented Aug 18, 2024 •
edited
Loading

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 JScearcy closed this Aug 27, 2024
@JScearcy JScearcy reopened this Aug 27, 2024
@JScearcy
Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JScearcy @hayleigh-dot-dev