Possible bug: phantom VDOM node erroring render when using React Aria ListBoxItem

[eoncarlyle]

• Check if updating to the latest Preact version resolves the issue

Describe the bug
A React Aria ComboBox containing ListBoxItem options fails to render with error Cannot set property previousSibling of #<Node>, render works normally for equivalent React application

To Reproduce

I created an issue demonstration repository here which uses the latest version of Preact (10.24.3 at time of writing). The issue demonstration repository is the bare minimum to reproduce the issue, and compares the erroring Preact application with the normally functioning, equivalent React application.

To reproduce the error in Preact and compare against the working React application, run the following after cloning the repository

$ npm --prefix preact install
$ npm --prefix preact run dev

In another terminal:

$ npm --prefix react install
$ npm --prefix react run dev

Note that

$ diff preact/src/app.tsx react/src/App.tsx
1,2c1,2
< import { useState } from "preact/hooks";
< import preactLogo from "./assets/preact.svg";
---
> import { useState } from "react";
> import reactLogo from "./assets/react.svg";
4c4
< import "./app.css";
---
> import "./App.css";
17a18
>

Observe the following in the developer console in the Preact application:

Uncaught TypeError: Cannot set property previousSibling of #<Node> which has only a getter
    at $681cc3c98f569e39$export$b34a105447964f9f.appendChild (Document.ts:119:13)
    at Object.insertBefore

Observe the following output in the React application:

Expected behavior
The Preact application should produce the same output as the React application. My best guess of what is going on is outlined here, it may be the case that Preact is attempting to render a layer of the VDOM hierarchy that does not exist, but I'm not certain.

[JoviDeCroock]

I think this is expected, on DOM nodes you are not allowed to change the previous sibling. This can only be done during DOM operations.

[eoncarlyle]

What also confused me is that two stack frames up from the error,parentVNode._dom in the function below was undefined. @JoviDeCroock could your explanation also apply here because React Aria is making Preact-illegal changes higher in the call stack?

// preact: children.js:343
function insert(parentVNode, oldDom, parentDom) {
    //...
    parentDom.insertBefore(parentVNode._dom, oldDom || null);
    //...
}

[JoviDeCroock]

Not sure how this library works exactly but it looks like when we call insertBefore on that fake DOM node it somehow triggers this appendChild https://github.com/adobe/react-spectrum/blob/e37ad74f5677475f36bab2998bc3285a10a8549b/packages/%40react-aria/collections/src/Document.ts#L104

You can see that calling insertBefore with null as the second argument makes us fall into appendChild https://github.com/adobe/react-spectrum/blob/e37ad74f5677475f36bab2998bc3285a10a8549b/packages/%40react-aria/collections/src/Document.ts#L138 (just like the normal DOM).

Now, rather than passing a BaseNode to this function we'll be passing a real DOM-node as we try to leverage these primitives.

I am unsure how to interpret this personally 😅 I haven't dug too deep in the code, I've done a pretty shallow exploration. I reckon to fix this we'd need to know what the target is for this piece of code i.e. what is child referring to here. From the error we can derive it's an implementation of Node, while the types suggest this should be an ElementNode, so my assumption is that we are passing a real DOM-node rather than the fake one they expect. https://developer.mozilla.org/en-US/docs/Web/API/Node

EDIT: sorry didn't mean to close

[JoviDeCroock]

Looked a bit deeper into this and yes, they pass a DocumentFragment as a custom parentDom to the createPortal implementation. We don't operate on these false DOM-nodes while we are diffing so we'll pass in a real DOM-node to the insertBefore which it tries to mutate as its own. Not sure whether this is fixable at this stage, this might warrant a backing VNode type of fix as we'd need to be able to diff without DOM pointers I reckon.