preactjs / preact

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

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

Open eoncarlyle opened 4 days ago

eoncarlyle commented 4 days ago

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:

image

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 commented 3 days ago

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 commented 3 days ago

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 commented 21 hours ago

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