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.

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

Moving an element from one parent to another can cause re-mounting #163

Open
raquo opened this issue Aug 5, 2024 · 3 comments
Open

Moving an element from one parent to another can cause re-mounting #163

raquo opened this issue Aug 5, 2024 · 3 comments
Labels
bug needs design The solution is not clear, or I am not very happy with it

Comments

@raquo
Copy link
Owner

raquo commented Aug 5, 2024

Background

In Laminar you can move an element from one parent to another, e.g. by dynamically including / excluding it from a pair of child <-- and children <-- contents. The design intent behind this feature is for the element to retain its mounted status, so that its subscriptions remain active.

(Note: the problem described here does not apply when moving elements within a single children <-- block).

Problem

However, in some cases, the element is unmounted from its current location before it's mounted to the new location, and so the element's subscriptions are deactivated and re-activated. That includes running onUnmount / onMount hooks. This is undersirable, both for predictability and performance reasons, since the main reason to move elements like this is to either retain complex element state, or to get max performance.

Reproduction by @felher (discord, scribble):

      val onTop = Var(true)
      val numMounts = Var(0)
      val elem = div("element", onMountCallback(_ => numMounts.update(_ + 1)))

      div(
        child.text <-- numMounts.signal.map(_.toString),
        button("toggle", onClick --> (_ => onTop.update(!_))),
        div(
          "top: ",
          child <-- onTop.signal.map({
            case true => elem
            case false => "disabled"
          })
        ),
        div(
          "bottom: ",
          child <-- onTop.signal.map({
            case true => "disabled"
            case false => elem
          })
        )
      )

In this reproduction, when onTop.signal emits, the first child <-- effect is triggered, unmounting elem. This happens before the second child <-- effect begins executing.

Solution?

I'm not yet sure what the solution should be.

I guess one approach would be to delay the deactivation of the element's subscriptions until the end of the current transaction. But this means that the element's subscriptions will remain active even after the element has been removed from the DOM, even if very briefly. Any subscriptions bound to the element being unmounted will survive for a bit longer, and will still be active after the element has been removed from the DOM.

  • This would be a breaking change, because if any of those subscriptions would have fired in the same transaction as the event that unmounted that element, previously, they wouldn't fire, and now, they would. I think.
  • I'm not 100% sure, but I think that currently such subscriptions don't have a chance to observe the element while it's unmounted from the DOM. If that's true, then implementing such a fix would start allowing events / effects in the same transaction to access the element when it's already removed from the real DOM tree, and this could break some of them, e.g. if they try to access the element's dimensions, that are only accessible while the element is mounted.

So I'm not sure if that would be a good solution. Needs some thinking, and deeper investigation.

Perhaps this could be opt-in somehow, like an element could be marked as "movable" to opt-in to the delayed unmounting logic? Well, maybe that could be the way to gradually roll this out, to reduce disruption. Although that option might be hard for users to discover.

Workaround

In some cases you may be able to use Airstream's delaySync operator to make sure that the event fired by the "sender" of the element is fired prior to the event fired by the "receiver". In this reproduction, both of those are onTop.signal, which is ok, but the solution won't work because the element can be moved in both directions (well, it will work for only one of those directions).

@raquo raquo added bug needs design The solution is not clear, or I am not very happy with it labels Aug 5, 2024
@raquo
Copy link
Owner Author

raquo commented Jan 2, 2025

Note – see another somewhat related issue with drag and drop on discord.

Basically, if you use a third party JS lib like Sortable, it will break laminar if you drag an element from one dynamic list into another dynamic list. The latter will fail when it tries to look up the externally inserted item in prevNodesMap, during children reconciliation.

So, TLDR – it seems that both of these issues can be solved by having a single global WeakMap[ReactiveElement, dom.Element], with only opt-in elements written into it to keep performance reasonable. And the way you could opt-in elements would be something like children.movable <--. Plus some element-level API that does the same, for custom logic. Then we could look up the element in this map when it's not found in prevElementsMap, and we could also give a more specific error message that would instruct the user on how to fix this.

And this same .movable thing would also enable a solution to this re-mounting issue, e.g. by delaying deactivation of the element somehow?

As an alternative to WeakMap, we could also add a custom .laminarRef property to each JS DOM element, but I would rather avoid that. There could be issues with GC, and I'm not sure if changing the shape of DOM objects would affect JS engine performance somehow. Both would probably be fine, but figuring that out is annoying, and the WeakMap approach seems very reasonable overall.

@nghuuphuoc
Copy link

nghuuphuoc commented Jan 7, 2025

+1 for this (I'm the one reported the issue on Discord).
I've been trying a few workarounds without success.

@raquo
Copy link
Owner Author

raquo commented Jan 22, 2025

Possibly one more reason to opt-in elements into moving: accidentally moving an element can be a bug caused by (incorrect) reuse of elements, as in #184

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs design The solution is not clear, or I am not very happy with it
Projects
None yet
Development

No branches or pull requests

2 participants