Prevents missing child from causing removal of last node

[thomas-marcucci]

Description

There is an error being thrown when removeChild is being called and the root is empty. -1 gets passed in as the index, no child is found, then detach runs and a type error is thrown child.id because child is undefined.

What this does

On the remote, it validates that a child is found when removeChild is called. When the child node is not found, instead of passing the -1 index through, it returns early doing nothing. This will prevent the exception as well as the removal of the last child from the parent.

On the host, it prevents the error from occurring by checking for a undefined result when the child node is supposed to be removed and returning early.

[kumar303]

Seems like the right thing to do 👍 I believe Thomas has a shop to reproduce the error in a Shopify checkout and was able to see it fixed with this patch but I'll let him confirm.

[thomas-marcucci]

I believe Thomas has a shop to reproduce the error in a Shopify checkout and was able to see it fixed with this patch but I'll let him confirm.

Yes! I can prove it's working.

[lemonmade]

Can you give me a bit more detail on how this can actually happen in practice? This change being on the host implies that the remote environment is sending the request — what causes it to send a request to remove an element that does not exist?

This solution does work, but it feels like it hides a deeper bug.

[thomas-marcucci]

The real bug is that calling root.removeChild(someNode) when someNode doesn't exist (or isn't a child of root) will remove the last child of root. It accepts a -1 index. I'm wary of fixing that right now in case some extensions expect that behaviour (even if it's a bug).

The way this comes up from calling root.removeChild(someNode) on an empty root. Something like doing removeChild multiple times in a row. I think what I've seen is an extension removing nodes in a general setup phase then again after a query finished. I have a very contrived example that can show the exception and it's basically just calling root.removeChild(node) three times in a row. It's an issue with the remote code, but it causes a type error cause undefined is passed to detach.

[lemonmade]

If the issue is on the caller of root.removeChild, could we add the "is the child actually a child" checking there? I realize this will not solve the error we have in Checkout, because that code is in deployed extensions which will not be updated. If we absolutely must silence that error, the only thing I would feel comfortable with on the host is for it to check for some error conditions, and offer a callback for how to handle them. Silently swallowing the error, and not actually fixing the root cause, is definitely not sufficient here.

[thomas-marcucci]

If I'm understanding, then we should be fixing the missing child issue - but it will only be fixed for new extensions. I think it'd just be something like this in root

function removeChild(
   const {strict} = rootInternals;
 
   return perform(container, rootInternals, {
-    remote: (channel) =>
-      channel(
-        ACTION_REMOVE_CHILD,
-        (container as any).id,
-        container.children.indexOf(child as any),
-      ),
+    remote: (channel) => {
+      const childIndex = container.children.indexOf(child as any);
+      if (childIndex === -1) {
+        return;
+      }
+      return channel(ACTION_REMOVE_CHILD, (container as any).id, childIndex);
+    },
     local: () => {
       removeNodeFromContainer(child, rootInternals);

If child isn't found, do nothing (or something if we want to call attention to it). I don't understand everything in remote-ui, so I'm not sure if the local function needs a fix as well.

The error happens frequently, so we need to handle it somehow. Would wrapping the call to currentReceiver.receive() in checkout in a try-catch and doing some checks and logging there be sufficient?

[lemonmade]

@thomas-marcucci yes, that is roughly what I was imagining. I think you can move the check outside of the perform() function, since to your point, it does also need to be applied to that local function too (it also does not seem to check whether the child is actually present in the parent, I think).

I still think your change on the host might be appropriate, but I'd want to pair it with this change to the remote, and a new API that can inform the consumer when such an invalid message has been received. This gives the consumer the full complement of support — we make it hard to make the mistake in the first place, by updating the remote environment, and we give them the ability to degrade gracefully in the case where invalid messages are received on the host (e.g., because an obnoxious extension was manually sending invalid postmessages, rather than using RemoteRoot#removeChild().