lustre-labs / lustre

An Elm-inspired framework for building HTML templates, single page applications, and server-rendered components in Gleam!
https://hexdocs.pm/lustre
MIT License
727 stars 52 forks source link

ensure nested sub-trees are morphed #115

Closed nerdyworm closed 2 months ago

nerdyworm commented 2 months ago

Good evening.

I ran into this bug where if you happen to nest more than two element.map calls, the third never gets rendered.

Example of how you can reproduce this bug :

fn view(model: Model) -> Element(Msg) {
  html.div([], [
    html.text("parent"),
    html.div([], [
      html.text("child"),
      html.div([event.on_click(Waaa)], [html.text("baby")])
        |> element.map(BabyMsg),
    ])
      |> element.map(ChildMsg),
  ])
}

The above example, the baby node is never rendered, but if you remove the element.map it works fine.

The fix seems to be just pushing the next subtree onto the stack and everything is happy again.

I'm not entirely sure if this is the correct fix for this situation, but it seems to resolve all the issues I'm having with it.

Let me know you need anything else.

Thanks!

hayleigh-dot-dev commented 2 months ago

Hmm yeah your description makes sense eg I can see how the current code wouldn't correctly handle directly nested Map nodes. What I don't understand is why the snippet doesn't work 🤔.

In the project root there's a test-apps/ folder. Do you think you could make a minimal lustre project in there with this scenario too.

nerdyworm commented 2 months ago

Added the example app.

Running the example with an older version (I also tried f7eadafb263a83179719b240415e34b7b1a5fa84 last night before opening this pr) image

Running the example with my patch: image

What I observed when debugging this is that the third nested map is simply discarded because it doesn't match any of the if else statements in the morph function.

This also happens if there wrapper elements around the element.map call.

hayleigh-dot-dev commented 2 months ago

This also happens if there wrapper elements around the element.map call.

Yeah this is the bit that surprises me. Hey anyway it works, thank you so much!