Unmount not called for nested children on render (since preact X)

[timon-witt]

I encountered a behaviour, that I think can be considered a bug, unless it was intentionally introduced with preact X.

This is a common way to force unmount of components in a specific container:

render(<div />, container, container.firstElementChild)

Using this on the following HTML with preact 8, results in componentWillUnmount of the component being properly called. (Note that the component can be as deeply nested inside the container as wished)

<div id="container">
  <div>
    <MyComponent />
  </div>
</div>

With preact X however, the component is not unmounted, which is really bad for my understanding. There now is no way, that I know of, to achieve emptying a container and properly unmouting all components inside. This is specifically bad for single page applications, like mine, that do this frequently.

If the component is not nested, but a direct child of the container, it even works with preact X.

<div id="container">
  <MyComponent />
</div>

Here I have two exactly similar codepens showing this behaviour, only different in the preact version. One runs with 8.5.2 and one with 10.0.5.

preact 8: https://codepen.io/timonwitt/pen/NWPqwxO
preact X: https://codepen.io/timonwitt/pen/BayNwzW

[timon-witt]

I made a pull request with unit tests that check this behaviour. One shows that it is successful as long as there is no nesting. The other one shows the failure if there is nesting.

[JoviDeCroock]

I've commented on why this is correct behaviour

[timon-witt]

@JoviDeCroock So you are saying, that there is no way to clear a site (or any other container) of all preact components, if it is not entirely rendered with preact?

Maybe let me try to explain my particular problem: The project I'm working on uses many smaller preact components, that are spread all over the site. There is no preact root or a single large preact tree. And since it is a SPA, it relies on clearing the DOM, and all preact components inside of it, on page change. Because of that, this bug is hindering us to upgrade to preact X.

Imo, as long as preact's use case is not limited to rendering the whole page with it, there must be a way to properly unmount your components, like in preact 8.

[JoviDeCroock]

Why can't you do render(null, renderRoot)? ReplaceNode is more or less meant to cover cases like these

renderRoot in your case is the first thing rendered by Preact so that conforms to your first test

[timon-witt]

Because render(null, renderRoot) doesn't work either.
In the example I only included ReplaceNode because it was necessary in preact 8.

[JoviDeCroock]

Let me rephrase, why would there be an arbitrary div between your root and what you are hydrating into? I don't see cases where this can happen.

Every widget you render can be a root, there is no limit in how much roots a certain application can have

[apfelbox]

The problem (as far as I understand) is that the behavior of the third parameter changed in preact. You can't just render somewhere and it works™️, but instead you need to look very careful on the other children of your parent.

That is because preact tries pretty hard to hydrate as smart as possible, instead of just letting the user decide via the third parameter on (if and) where to hydrate.

Something like this worked in preact 8, but breaks in preact X:

<div>
    <div>hello</div>
    <script id="test"></script>
</div>

const MyComp = () => <div>MyComp</div>;

let script = document.getElementById("test");
render(MyComp, script.parentElement);
script.parentElement.removeChild(script);

in preact 8 it just rendered at the end of the parent <div>, in preact X due to auto-hydration, it tries to hydrate on the first element, as it is the same element.

I can imagine that this might be the same problem here?

[JoviDeCroock]

Your example does not seem related to replaceNode @apfelbox but the explanation is correct, we don't account for diverging HTMLTags because this can lead to unintended behaviour.

The problem you are showcasing is us creating a root and is not related to the thirth parameter.

The problem at hand is an arbitrary distance between your replacement and the root

[timon-witt]

Let me rephrase, why would there be an arbitrary div between your root and what you are hydrating into? I don't see cases where this can happen.

The div in the showcase is symbolic for every imaginable element or multiple elements... I don't want to hydrate anything, but just remove and clear all components, while unmounting them.

I am not sure where the problem in my explanation lies. Let me give it one more try. Here I have some HTML for you, that visualizes what I wrote before:

<body>
  <header></header>
  <div id="main-content">
    <h1>A headline</h1>
    <SomeComponent />
    <section>
      <AnotherComponent /> <!-- deep nested -->
      <div>
        <AndOneMoreComponent /> <!-- deep nested -->
      </div>
    </section>
  </div>
  <footer></footer>
</body>

Note the #main-content div. It contains many elements, of which some are preact components. In preact 8 there was a way, to clear the #main-content container (remove all children) and properly unmount all components inside. This feature is still important, as it is hardly possible to prevent memory leaks etc., if components are just not unmounted.

Let me know if this is understandable now.

[JoviDeCroock]

It is understandable but that paradigm has moved to multiple roots, as the article I've linked you to about partial hydration, because from what I understand in your application you want parts of your app to be hydrated.

The approach of working with roots (in our case this is a symbolic Fragment) is very needed for the granular control and DOM-correctness we can offer with X, it's a bit of a mindset shift but in essence becomes in my opinion a lot more intuitive than being able to do in this case render(x, body, replaceMyNestedComponent) now you would rather do render(SomeComponent, main) and a bit further render(AnotherComponent, section) and so on.

Another approach would be to hydrate completely and assert granular control through JSX instead.

I do wonder if your first render would be with replaceNode as well if we could make a functionality to bypass this, will update my answer later when I've looked into this.

Link to article: https://medium.com/@luke_schmuke/how-we-achieved-the-best-web-performance-with-partial-hydration-20fab9c808d5

[kadet1090]

I don't know if i understand this issue correctly but we have same problem in our project. In preact 8 we were able to "unrender" whole tree at once just by doing render(null, element.parentElement, element) - all components that were children of element were unmounted correctly. And now in case mentioned in comment #2168 (comment) no componentWillUnmount call would be made.

This is important because in some cases we don't know components (and even if there are any) that are part of some element and therefore we cannot properly dispose them, creating memory leaks and other strange behaviours. This is especially important for projects that have mix of both JSX code and plain old DOM, because of gradual rewriting. I can later provide you with some working example of such behaviour.

[JoviDeCroock]

I took the time to whip up some supported scenario's for working with roots as they are in Preact X https://codesandbox.io/s/peaceful-lumiere-n9fpz

[kadet1090]

So is there any valid method to umount all descendant (event deeply nested) components of some element if we don't know them ahead of time?

[JoviDeCroock]

@kadet1090 that's covered in "Case 1" even if you work with multiple roots (Case 3)

[kadet1090]

Well not really.

In our case the scenario is that we have some old code that manages DOM by itself and components that can be either preact or plain dom. We have some helper function that does element removal, and it worked fine in preact 8, i.e. it removed element from dom and unmounted all descendant components.

function remove(element) {
    // I suppose in preact X it should be render(null, element);
    render(null, element.parentElement, element); 
    element.remove();
}

So let's say that we have situation from comment mentioned before:

<body>
  <header></header>
  <div id="main-content">
    <h1>A headline</h1>
    <SomeComponent />
    <section>
      <AnotherComponent /> <!-- deep nested -->
      <div>
        <AndOneMoreComponent /> <!-- deep nested -->
      </div>
    </section>
  </div>
  <footer></footer>
</body>

And we want to remove #main-content by calling remove(document.getElementById('main-content')). In preact 8 it would unmount <SomeComponent />, <AnotherComponent /> and <AndOneMoreComponent /> and in preact X does not. As you can notice in context of remove function we don't really know anything about existing roots end therefore cannot call render(null, root) for each of them, so case 3 does not apply.

[JoviDeCroock]

We have fully transitioned to managing through a vdom-structure instead of holding references on dom-nodes. This gives us the advantage of supporting so much more stuff than we used to do in Preact 8 (all the added features), I think this is worth the trade-off. Preact manages the nodes it has spawned and nothing outside of it (which seems reasonable).

Adding vanilla-js code or another framework should be managed by that as far as I can see, this isn't supported in Preact X.

[kadet1090]

I understand that this is not typical scenario and it's not worth to change a lot of code just for few of us. So maybe - is there any way to check if dom element is preact root? It'd allow us to write some code that traverses the tree and calls render(null, child) on every nested component if needed.

[marvinhagemeister]

To elaborate a bit on @JoviDeCroock 's comment:

In Preact 8.x we did compare our virtual tree against the DOM tree directly. To support components we did set a _component property on each node that was rendered by a Component. Because of that the concept of render roots didn't really matter at all for the 8.x release line. We could just pick any node in the tree and call render(vnode, domNode) on it, and everything would work just fine. That's the scenario we have here in this issue.

In Preact X we don't diff against the DOM anymore, but against a second virtual copy of the tree. The DOM has moved to being just a side-effect of our render function and doesn't hold any tree data anymore. Except for root nodes which can be checked for like this:

const container = document.getElementById("app");
render(<div />, container);

// Roots will have a _children property (mangled name is __k,
// see the 'mangle.json' in this repo
function isRenderRoot(domNode) {
  return domNode.__k !== undefined
}

console.log(isRenderRoot(container));
// Logs: true

I doubt that we'll have a more elegant solution to convert existing nodes of a Preact tree into a new root soon. It's an interesting use case but like you said, it's somewhat niche. Hope this helps 🙂

[kadet1090]

Maybe we can add symbol for that not to rely on mangled name __k? It'd be easier and cleaner to make transition from DOM to preact.

[kadet1090]

Working solution

// you can use flatMap instead of map(...).reduce(...) combo
export function getAllRoots(element: Element): Element[] {
    return Array.from(element.childNodes)
        .filter(element => element instanceof Element)
        .map((element: HTMLElement) => element['__k'] !== undefined ? [ element ] : getAllRoots(element))
        .reduce((acc, arr) => [...acc, ...arr], []);
}

export function remove(element: Element) {
    getAllRoots(element).forEach(root => render(null, root));
    element.remove();
}

[marvinhagemeister]

Changing the handling of the way we treat roots has come up a few times in the past weeks. The main interest is to set markers for lazy hydration. Because of that we haven't made any part surrounding roots public. My guess is that it will remain private (and thus mangled) until we settle on something we can commit ourselves to make public and take on maintenance. But that may be months away.

For now your best bet is to rely on the private mangle property. Due to our build pipeline you can be sure that the mangled name won't change across Preact versions, so there is at least a little bit of guarantee there.

[developit]

Yup, @kadet1090 that's a reasonable solution. In addition to @marvinhagemeister's point about the symbol, Preact only publishes the mangled names (__k), so there should be no reason to support the unmangled ones.

[timon-witt]

Thank you @marvinhagemeister and @kadet1090 for keeping on the discussion and finding a reasonable solution.

[ForsakenHarmony]

For readability you can store __k in a variable with a more intuitive name

[marvinhagemeister]

Closing, because it's something we don't support officially. A gradable workaround has been suggested, so there doesn't seem to be further action needed on our part 👍

[timon-witt]

For everyone searching for a working solution, here is how we finally implemented it on our side. Checking for __k property on every single dom element is not really practicable from a performance perspective.

Basically have an enhanced wrapping render function, that marks preact root elements with an attribute. These can then be identified fast and unmounted.

/**
 * Preact containers, that are rendered with the render function below, receive this attribute.
 * It is used to identify preact containers afterwards.
 */
const preactContainerAttr = 'data-preact-container';

/**
 * Enhanced preact.render function.
 */
export const render: typeof preact.render = (vnode, parent, replaceNode): void => {
  if (parent instanceof Element) {
    parent.setAttribute(preactContainerAttr, 'true');
  }
  preact.render(vnode, parent, replaceNode);
};

/**
 * Unmounts all preact components inside the given element and its children.
 * @param parent Pass the parent here, where the component is rendered into.
 *               Don't pass a component itself.
 */
export const unmount = (parent: Element): void => {
  let parentsToUnmount = [];

  // Check the passed element itself.
  if (parent.getAttribute(preactContainerAttr) === 'true') {
    parentsToUnmount.push(parent);
  }

  // Check child elements
  parentsToUnmount.push(...Array.from(
    parent.querySelectorAll(`[${preactContainerAttr}=true]`)
  ));

  // Execute unmount
  parentsToUnmount.forEach(container => {
    preact.render(null, container);
    container.removeAttribute(preactContainerAttr);
  });
};