honox icon indicating copy to clipboard operation
honox copied to clipboard

When island components are nested, the child island is broken

Open berlysia opened this issue 1 year ago • 5 comments

What version of HonoX are you using?

0.1.15

What steps can reproduce the bug?

Prepare two islands (can be same):

  1. as parent: An island that accepts children and have something can cause update
  2. as child: An island that have state and/or effect

Here we have Wrapper component like this:

export default function Wrapper({children, name}: PropsWithChildren<{ name: string; }>) {
  const [count, setCount] = useState(0)
  const id = useState(() => Math.random().toString(36))[0];

  useEffect(() => {
    console.log("effect", name, id);
    return () => {
      console.log("cleanup", name, id);
    }
  }, [name, id])

  return (
    <div className={className}>
      <p>{count}</p>
      <button onClick={() => setCount(count + 1)}>Increment</button>
      {children}
    </div>
  )
}

https://github.com/honojs/honox/assets/950573/f017a160-6874-4134-b811-1aa81bf18578

reproduce repo is here: https://github.com/berlysia/repro-honox-client-unmount-and-nocleanup

What is the expected behavior?

The number of renders will be minimized, and DOMs that produce the same result will be reused unless a key is given. The effect must disappear properly by calling a cleanup function.

What do you see instead?

Problems:

  1. The body of the child is evaluated multiple times during the initial render.
  2. The child unexpectedly remounts when updating the parent. The child's state will reset at this time.
  3. Cleanup function is not called when unmounting.

So if we have timer or event listeners in the child's effect, they will be over-replicated.

Additional information

No response

berlysia avatar Apr 19 '24 18:04 berlysia

Hi @berlysia !!

Thank you for raising the issue. One question. Does this problem happen in a development environment? I've tried it. Indeed, it does not work with HTML and script built with SSG. But it works well on dev server.

yusukebe avatar Apr 20 '24 07:04 yusukebe

Thank you for trying and clarifying.

I did investigation again. In development environment:

  1. Happened. (unexpected multiple render + does not cleanup)
    • To observe this, use setInterval timer and cleanup logic in effect.
      • With grandparent -> parent -> child hierarchy, number of renderings are like this:
        • grandparent: 1
        • parent: 2. There is two setInterval timers.
        • child: 3. There is three timers.
  2. Not happened. (parent state change cause unexpected remount, is not happen)
  3. It depends.
    • For problem 1: Happened, does not cleanup, already described above.
    • For problem 2: Not happened. (But I've found another issue about usage of key)

berlysia avatar Apr 20 '24 08:04 berlysia

To summarize issues with nested island components:

  1. When hydrating the outputted HTML, nested islands are evaluated multiple times without cleanup. The number of evaluation is same as the level of nesting.
    • "Problem 1" described in the top of this Issue
  2. In the production build (not observed in development), updating the state of a parent island causes the state of child islands to reset. It looks like a remount behavior without cleanup.
    • "Problem 2" described in the top of this Issue
  3. When updating the key of a child island to re-initialize explicitly, the part corresponding to the island with the changed key disappears from the screen. During this process, no cleanup occurs, nor is the body of the new child island evaluated.
    • This is another issue that I found in https://github.com/honojs/honox/issues/154#issuecomment-2067607031

berlysia avatar Apr 20 '24 09:04 berlysia

Hi @berlysia

Thank you for explaining! We should fix them. I'll investigate, but now we have problem #155. I don't know if it is related to this issue, but I'm working on it first.

yusukebe avatar Apr 21 '24 12:04 yusukebe

Hi @berlysia @yusukebe

Sorry, this is probably a bug in "hono/jsx/dom".

I believe https://github.com/honojs/hono/pull/2563 will resolve this issue

usualoma avatar Apr 27 '24 00:04 usualoma

I think we can close this now.

yusukebe avatar May 06 '24 20:05 yusukebe