preactjs / preact

⚛️ Fast 3kB React alternative with the same modern API. Components & Virtual DOM.
https://preactjs.com
MIT License
36.89k stars 1.95k forks source link

10.18 regression: setState triggers DOMException: Node.insertBefore: Child to insert before is not a child of this node #4221

Closed emersion closed 7 months ago

emersion commented 1 year ago

Describe the bug Getting this error (and app gets completely stuck) with 10.19.2. Everything works fine after a downgrade to 10.18.2.

Uncaught (in promise) DOMException: Node.insertBefore: Child to insert before is not a child of this node
    Preact 20
    switchBuffer app.js:535
    handleBufferListClick app.js:1573
    items buffer-list.js:66
    handleClick buffer-list.js:8
    Preact 24
    handleMessage app.js:1028
    connect app.js:799
    handleMessage client.js:450
    reconnect client.js:178
    reconnect client.js:176
    Client client.js:153
    connect app.js:762
    handleMessage app.js:1124
    Preact 6
    handleMessage app.js:1110
    connect app.js:799
    handleMessage client.js:450
    reconnect client.js:178
    reconnect client.js:176
    Client client.js:153
    connect app.js:762
    handleConfig app.js:390
    App app.js:242
    promise callback*App app.js:241
    Preact 4
    <anonymous> main.js:4
constants.js:2:13
    Preact 5
    switchBuffer app.js:535
    handleBufferListClick app.js:1573
    items buffer-list.js:66
    handleClick buffer-list.js:8
    Preact 24
    handleMessage app.js:1028
    connect app.js:799
    handleMessage client.js:450
    reconnect client.js:178
    (Async: EventListener.handleEvent)
    reconnect client.js:176
    Client client.js:153
    connect app.js:762
    handleMessage app.js:1124
    M Preact
    some self-hosted:137
    M Preact
    some self-hosted:137
    Preact 4
    handleMessage app.js:1110
    connect app.js:799
    handleMessage client.js:450
    reconnect client.js:178
    (Async: EventListener.handleEvent)
    reconnect client.js:176
    Client client.js:153
    connect app.js:762
    handleConfig app.js:390
    App app.js:242
    (Async: promise callback)
    App app.js:241
    Preact 4
    <anonymous> main.js:4

To Reproduce I don't have a minimal reproducer (yet). Run npm update in gamja and then switch between tabs quickly.

emersion commented 1 year ago

Hm, actually, 10.18 still has that bug, although happens less often. Attempting to downgrade to v10.17.1 now.

JoviDeCroock commented 11 months ago

A minimal reproduction would really be needed, we can't run a whole repo sorry 😅

emersion commented 10 months ago

I haven't managed to come up with a minimal reproduction, but can confirm that the bug is gone with v10.17.1.

emersion commented 10 months ago

Got a better stack trace, the exception is raised here: https://github.com/preactjs/preact/blob/f7ccb9010077ecb46fc271224bbc5e015e00efe6/src/diff/children.js#L366

JoviDeCroock commented 10 months ago

Hmm then it would be one of the commits in 10.18.0 and my assumption is that it would be this one then https://github.com/preactjs/preact/pull/4125/files#diff-1135418bf513cf842acabfdcae5e8e4fb2ab335d4483216d1b39e15a3caf342eR106 but then again that got removed in the latest versions so we would really need a reproduction sorry...

andrewiggins commented 10 months ago

Thanks for the bug report. I know getting a repro can be hard sometimes. Can you perhaps share any other information about your app? For example, what other libraries are you using? hook, compat, signals? Are you using Suspense, memo, forwardRef? From the stack trace it looks you are doing something with web sockets? Are you using any React/Preact libraries to integrate those into your app?

emersion commented 10 months ago

The app is very "basic", as in it only uses Preact class-based components and nothing else. The htm package is used for constructing the DOM (no JSX). No other Preact-related libraries are used. No hooks, nor compat, nor signals, nor Suspense, not memo, not forwardRef. WebSockets are used directly without an intermediate library (but are not involved here).

The error comes from here, called from a click event handler: https://git.sr.ht/~emersion/gamja/tree/141fc3e07c69b1985719d6075e2f101a3f73d6fe/item/components/app.js#L535

The error doesn't happen consistently, have to click multiple times to trigger it.

developit commented 10 months ago

given that it's affected by timing, I wonder if this could be related to the event handler pathing fixes that went out in .19? There's a large VDOM tree here with an event handler defined at the root that should be a stable reference.

JoviDeCroock commented 10 months ago

That did go out in the same release, I am however confused how that would affect setting _nextDom, this does sound in the boathouse of https://github.com/preactjs/preact/issues/4161 when saying "rapid clicking"

JoviDeCroock commented 9 months ago

Mind checking whether 10.19.6 fixes your issue?

emersion commented 9 months ago

Yes, still seeing that bug in this release.

emersion commented 9 months ago

I've tried https://github.com/nbalatsk-oracle/preact but could also reproduce this bug with that fork.

JoviDeCroock commented 9 months ago

@emersion it would help us a lot to have this bug in a codesandbox or stackblitz

objerke commented 8 months ago

@JoviDeCroock I've managed to reproduce the issue in a Stackblitz environment, using preact 10.19.7.

The issue can be seen if a keyed list item is moved to an earlier position, but also rendered as null. Here is the full example.

import { render } from 'preact';
import { useState } from 'preact/hooks';

const firstList: Item[] = [
  { id: 'One' },
  { id: 'Two' },
  { id: 'Three' },
  { id: 'Four' },
];

const secondList: Item[] = [
  { id: 'One' },
  { id: 'Four', renderAsNullInComponent: true },
  { id: 'Six' },
  { id: 'Seven' },
];

render(<App />, document.body);

export function App() {
  let [list, setList] = useState(firstList);

  return (
    <>
      <p>
        This is reproduction of{' '}
        <a href="https://github.com/preactjs/preact/issues/4221">
          Preact Issue 4221
        </a>
        . The list diffing breaks when clicking the button.{' '}
      </p>
      <div>
        {list.map((item) => (
          <RenderedItem key={item.id} item={item} />
        ))}
      </div>
      <button
        onClick={() => {
          setList(secondList);
        }}
      >
        Switch list
      </button>
    </>
  );
}
JoviDeCroock commented 8 months ago

Thank you so much for the reproduction, mind checking out #4312 to see if it addresses your case in your bigger applications?

emersion commented 8 months ago

Many thanks to @objerke and @JoviDeCroock!

Unfortunately, I can still see the regression on 10.20.0…

JoviDeCroock commented 8 months ago

@emersion with a minimal reproduction I can def look at it

EDIT: maybe related to https://github.com/preactjs/preact/issues/4194

StabbarN commented 7 months ago

We can see it in 10.20.1.

marvinhagemeister commented 7 months ago

@StabbarN can you share a reproduction case? We've addressed all the cases we were able to reproduce, but it looks like something eluded us.

JoviDeCroock commented 7 months ago

This should be fixed by https://github.com/preactjs/preact/pull/4318 but a reproduction would be really welcome in this one. This however isn't released yet.

Edit: released in 10.20.2

StabbarN commented 7 months ago

I was going to reproduce in a smaller repo but then I saw 10.20.2. I can reproduce it in 10.20.1 but not in 10.20.2 so this issue can most likely be closed.

Thanks!

emersion commented 7 months ago

Yes, can confirm this is now fixed with 10.20.2. Cheers!