@samitouri / QOS-React / commits / 99e1024051

Check if a child is a new child before calling moveBefore (#32567)

This fixes a critical issue with moveBefore. I was told that the disconnected -> connected case was going to be relaxed and not be an error but apparently that is not the case. This means that we can't use this for initial insertions. Only moves. Unfortunately React's internals doesn't distinguish these cases. This adds a hack that checks each nodes but this is pretty bad for performance. We should only call this in one or the other case. Given that we still need feature detection. Both of which means that these calls are no longer inlined and this extra code. I wonder if it's even worth it given that you can't even rely on it working anyway since not all browsers have it. Kind of don't want to ship this until all browsers have it. Even then we'd ideally refactor React to use separate code paths for initial insertion vs moves. Which leads to some unfortunate code duplication.

Sebastian Markbåge committed Mar 10, 2025 at 18:12 UTC 99e1024051f2e6b2d2849b966e2f4354aef2a1d0
1 file changed +5 -5
packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
+5 -5
@@ -808,7 +808,7 @@ export function appendChild(
808 parentInstance: Instance,
809 child: Instance | TextInstance,
810 ): void {
811 - if (supportsMoveBefore) {
811 + if (supportsMoveBefore && child.parentNode !== null) {
812 // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore.
813 parentInstance.moveBefore(child, null);
814 } else {
@@ -828,7 +828,7 @@ export function appendChildToContainer(
828 container.nodeType === COMMENT_NODE
829 ) {
830 parentNode = (container.parentNode: any);
831 - if (supportsMoveBefore) {
831 + if (supportsMoveBefore && child.parentNode !== null) {
832 // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore.
833 parentNode.moveBefore(child, container);
834 } else {
@@ -840,7 +840,7 @@ export function appendChildToContainer(
840 } else {
841 parentNode = (container: any);
842 }
843 - if (supportsMoveBefore) {
843 + if (supportsMoveBefore && child.parentNode !== null) {
844 // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore.
845 parentNode.moveBefore(child, null);
846 } else {
@@ -870,7 +870,7 @@ export function insertBefore(
870 child: Instance | TextInstance,
871 beforeChild: Instance | TextInstance | SuspenseInstance,
872 ): void {
873 - if (supportsMoveBefore) {
873 + if (supportsMoveBefore && child.parentNode !== null) {
874 // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore.
875 parentInstance.moveBefore(child, beforeChild);
876 } else {
@@ -896,7 +896,7 @@ export function insertInContainerBefore(
896 } else {
897 parentNode = (container: any);
898 }
899 - if (supportsMoveBefore) {
899 + if (supportsMoveBefore && child.parentNode !== null) {
900 // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore.
901 parentNode.moveBefore(child, beforeChild);
902 } else {