Add a warning when passing root node to unmountComponentAtNode

[sophiebits]

Tricky because you could actually use a React root element as a container…

cc @dylanvee

[syranide]

It's kind of late, but isn't this straight-forward? You shouldn't be looking at the mount node, but the node being unmounted. If it isn't a root id only, you've done something wrong.

[charliermarsh]

@syranide Yeah, I think we want to throw if the argument is a non-container, non-empty React root node.

[syranide]

@crm416 Well no, my point is that you do not want to look at the container pointed to by the argument, but only its children. If it doesn't have a React-node then everything is good, if there's a React node there then it must be a root node or it's an error.

[charliermarsh]

@syranide Ah, ok. I agree that would be an error, but isn't it a slightly more general warning than is being described here?

E.g., for the conditions you listed, you could pass in a non-root React node as the "container" and it still might qualify, but I thought here we were specifically looking at the more common case in which you accidentally pass in the actual component (i.e., its root node) rather than its container.

[syranide]

@crm416 Unless I'm mistaken, it's the only error that makes sense.

<!-- OK -->
<div mount-here>
</div>

<!-- OK -->
<div reactid=".0" mount-here>
</div>

<!-- OK (empty) -->
<div mount-here>
  <div></div>
</div>

<!-- OK (reuse)-->
<div mount-here>
  <div reactid=".0"></div>
</div>

<!-- OK (empty) -->
<div reactid=".0" mount-here>
  <div></div>
</div>

<!-- OK (reuse) -->
<div reactid=".0" mount-here>
  <div reactid=".1"></div>
</div>

<!-- ERROR -->
<div reactid=".0" mount-here>
  <div reactid=".0.0"></div>
</div>

[charliermarsh]

@syranide I agree with those cases. You definitely have a lot more experience here than I do, so I could be confused, but here's what I was referring to:

<!-- ERROR -->
<div reactid=".0">
  <div reactid=".0.0" mount-here>
    <div reactid=".0.0.0"></div>
  </div>
</div>

In that case, you wouldn't want the warning to be "You're passing a root node to unmountComponentAtNode", even though the argument you passed in as the container contains a non-root React node, right?

[syranide]

@crm416 Perhaps we're misunderstanding each other. :)

It seems to me that your example is equal to mine (it's just deeper down the hierarchy), it's not about the node with mount-here at all, it can be valid to mount there even if its React ID is .0 or .0.1.2.3. So only the child is important, if that child isn't a React root (i.e. .0) then it's an error, when it's a React root it's "reused".

[charliermarsh]

@syranide Ahh, got it--thanks for explaining :) Indeed, the actual errors are the same.

I still wonder if we want to have a specific warning for when you pass in a React root node in particular? Seems like the error we're describing would happen most frequently out of caller confusion RE React root node vs. container, and that a root-node-specific warning would be useful (CC: @spicyj). Regardless, a warning for the general case is useful too and probably the right first step.

[ruiaraujo]

This was apparently fixed with #2065.