Add warning for rendering into container that was updated manually (#…

…10210)

* RFC Add warning for rendering into container that was updated manually

RFC because we still need to tidy this up and verify that all tests
pass.

**what is the change?:**
We want to warn when users render into a container which was manually
emptied or updated outside of React. This can lead to the cryptic error
about not being able to remove a node, or just lead to silent failures
of render. This warning should make things more clear.

Note that this covers the case where the contents of the root container
are manually updated, but does not cover the case where something was
manually updated deeper in the tree.

**why make this change?:**
To maintain parity and increase clarity before releasing v16.0 beta.

**test plan:**
`yarn test`

**issue:**
#8854

last item under the '16 beta' checklist.

* Add test and tweak check for rendering into manually updated container

STILL TODO: figure out how to skip this warning when the component
renders to a portal.

Unfortunately 'ReactPortal.isPortal(children)' returns false, even in
the failing test where we are rendering to a portal.

**what is the change?:**
- added a test for the case where we call 'ReactDOM.render' with a new
  container, using a key or a different type, after the contents of the
  first container were messed with outside of React. This case throws,
  and now at least there will be an informative warning along with the
  error.
- We updated the check to compare the parent of the 'hostInstance' to
  the container; this seems less fragile
- tweaked some comments

**why make this change?:**
Continue improving this to make it more final.

**test plan:**
`yarn test`

**issue:**
#8854

* Stub our `console.error` in one of the portal tests

**what is the change?:**
See title

**why make this change?:**
See comment in the code

**test plan:**
`yarn test`

**issue:**
#8854

* Skip warning in 'ReactDOMFiberEntry' when mounting to Comment node

**what is the change?:**
We have a warning for cases when the container doesn't match the parent
which we remembered the previously rendered content being rendered into.

We are skipping that warning when you render into a 'comment' node.

**why make this change?:**
Basically, if you render into a 'comment' node, then the parent of the
comment node is the container for your rendered content. We could check
for similarity there but rendering into a comment node seems like a
corner case and I'd rather skip the warning without knowing more about
what could happen in that case.

**test plan:**
`yarn test`

**issue:**
#8854

* Improve warning message, remove dedup check, and unmock console.error

**what is the change?:**
Various changes to get this closer to being finished;
- Improved warning message (thanks @spicyj!!!)
- Removed dedup check on warning
- Remove mocking of 'console.error' in portals test

**why make this change?:**
- warning message improvement: communicates better with users
- Remove dedup check: it wasn't important in this case
- Remove mocking of 'console.error'; we don't want to ignore an
  inaccurate warning, even for an "unstable" feature.

**test plan:**
`yarn test` -> follow-up commits will fix the remaining tests

**issue:**
issue #8854

* Possible fix for issue of incorrect warning for portal re-render

**what is the change?:**
Add a property to a container which was rendered into using
`ReactDOM.unstable_createPortal`.

**why make this change?:**
We don't want to warn for mismatching container nodes in this case - the
user intentionally rendered into the portal container instead of the
original container.

concerns;
- will this affect React Native badly?
- will this add bloat to the portal code? seems small enough but not
  sure.

**test plan:**
`yarn test`

**issue:**
#8854

* Fix logic for checking if the host instance container is a portal

**what is the change?:**
When focusing on fixing the warning to not check when we are using
portals, I missed checking for the existence of the host instance parent
before checking if it was a portal. This adds the missing null checks.

**why make this change?:**
To fix a bug that the previous commit introduced.

**test plan:**
`yarn test`
-> follow-up commits fix more of the test failures

**issue:**
#8854

* Clean up new tests in ReactDOMFiber-test

**what is the change?:**
- removed extra single quotes, downgrade double quotes to single
- update expected warning message to match latest warning message
- fix indentation

**why make this change?:**
- get tests passing
- code maintainability/readability

**test plan:**
`yarn test`
follow up commits will fix the remaining tests

**issue:**
#8854

* Add 'unmountComponentAtNode' call in test for reconciling pre-rendered markup

**what is the change?:**
We have a test that verifies React can reconcile text from pre-rendered
mark-up. It tests React doing this for three strings and three empty
strings.

This adds a call to 'unmountComponentAtNode' between the two
expectations for strings and empty strings.

**why make this change?:**
We now warn when someone messes with the DOM inside of a node in such a
way that removes the React-rendered content. This test was doing that. I
can't think of a situation where this would happen with server-side
rendering without the need to call 'unmountComponentAtNode' before
inserting the server-side rendered content.

**test plan:**
`yarn test`

Only one more failing test, will fix that in the next commit.

**issue:**
#8854

* ran prettier

* remove unused variable

* run scripts/fiber/record-tests

* Fix type error and improve name of portal container flag

**NOTE:** I am still looking for a good place to move this flag
assignment to, or a better approach. This does some intermediate fixes.

**what is the change?:**
- fixed flow error by allowing optional flag on a DOMContainer that
  indicates it was used as a portal container.
- renamed the flag to something which makes more sense

**why make this change?:**
- get Flow passing
- make this change make more sense

We are still not sure about adding this flag; a follow-up diff may move
it or take a different approach.

**test plan:**
`yarn test`

**issue:**
#8854

* Add flag to portalContainer on mount instead of in `createPortal`

**what is the change?:**
We add a flag to the container of a 'portal' in the 'commit work' phase
in Fiber. This is right before we call `appendChildToContainer`.

**why make this change?:**
- Sometimes people call `ReactDOM.render(... container)`, then manually
clear the content of the `container`, and then try to call another
`ReactDOM.render(... container)`.
- This leads to cryptic errors or silent failure because we hold a
  reference to the node that was rendered the first time, and expect it
  to still be inside the container.
- We added a warning for this issue in `renderSubtreeIntoContainer`, but
  when a component renders something returned by
  `ReactDOM.unstable_createPortal(<Component />, portalContainer);`,
  then the child is inside the `portalContainer` and not the `container,
  but that is valid and we want to skip warning in that case.

Inside `renderSubtreeIntoContainer` we don't have the info to determine
if a child was rendered into a `portalContainer` or a `container`, and
adding this flag lets us figure that out and skip the warning.

We originally added the flag in the call to
`ReactDOM.unstable_createPortal` but that seemed like a method that
should be "pure" and free of side-effects. This commit moves the
flag-adding to happen when we mount the portal component.

**test plan:**
`yarn test`

**issue:**
#8854

* Force an 'any' type for the `hostInstance.parentNode` in warning check

**what is the change?:**
This is awful. :(
I'm not sure how else to let Flow know that we expect that this might be
a sort of `DOMContainer` type and not just a normal `Node` type.

To at least make the type information clear we added a comment.

**why make this change?:**
To get `flow` passing. Looks like we have `any` types sprinkled
throughout this file. phooey. :(

**test plan:**
`yarn flow`

**issue:**
#8854

* Ignore portals in `DOMRenderer.findHostInstance`

**what is the change?:**
We want to ignore portals when firing a certain warning.

This allows us to get the host instance and ignore portals.

Also added a new snapshot recording while fixing things.

**why make this change?:**
Originally we had added a flag to the DOM node which was used for

rendering the portal, and then could notice and ignore children rendered
into those nodes.

However, it's better to just ignore portals in
`DOMRenderer.findHostInstance` because
 - we will not ignore a non-portal second child with this approach
 - we meant to ignore portals in this method anyway (according to a
   'TODO' comment)
 - this change only affects the DOM renderer, instead of changing code
   which is shared with RN and other renderers
 - we avoid adding unneeded expandos

**test plan:**
`yarn test`

**issue:**
#8854

* Ran prettier

* Remove error snapshot test

I think there is a bug where an empty snapshot is treated as an 'outdated snapshot'.

If I delete the obsolute snapshot, and run ReactDOMFiber-test.js it generates a new snapshot.
But then when I run the test with the newly generated snapshot, it says "1 obsolete snapshot found",
At some point I will file an issue with Jest. For now going to skip the snapshot generation for the error message in the new test.

* Remove expando that we were adding to portal container

**what is the change?:**
see title

**why make this change?:**
this is part of an old approach to detecting portals, and we have
instead added a check in the `findHostInstance` method to filter out
portals.

**test plan:**
`yarn test`

**issue:**
#8854

* Fork `findHostInstance` to make `findHostInstanceWithNoPortals`

**what is the change?:**
We need to get host instances, but filter out portals. There is not
currently a method for that.

**why make this change?:**
Rather than change the existing `findHostInstance` method, which would
affect the behavior of the public `findDOMNode` method, we are forking.

**test plan:**
`yarn test`

**issue:**
#8854

scripts/fiber/tests-passing.txt

@@ -667,6 +667,10 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
 * should not onMouseLeave when staying in the portal
 * should not update event handlers until commit
 * should not crash encountering low-priority tree
+* should not warn when rendering into an empty container
+* should warn when replacing a container which was manually updated outside of React
+* should warn when doing an update to a container manually updated outside of React
+* should warn when doing an update to a container manually cleared outside of React
 * throws if non-element passed to top-level render
 * throws if something other than false, null, or an element is returned from render
 * treats mocked render functions as if they return null

src/renderers/dom/fiber/ReactDOMFiberEntry.js

@@ -73,8 +73,12 @@ findDOMNode._injectFiber(function(fiber: Fiber) {
 });
 
 type DOMContainer =
-  | (Element & {_reactRootContainer: ?Object})
-  | (Document & {_reactRootContainer: ?Object});
+  | (Element & {
+    _reactRootContainer: ?Object,
+  })
+  | (Document & {
+    _reactRootContainer: ?Object,
+  });
 
 type Container = Element | Document;
 type Props = {
@@ -531,6 +535,21 @@ function renderSubtreeIntoContainer(
   );
 
   if (__DEV__) {
+    if (container._reactRootContainer && container.nodeType !== COMMENT_NODE) {
+      const hostInstance = DOMRenderer.findHostInstanceWithNoPortals(
+        container._reactRootContainer.current,
+      );
+      if (hostInstance) {
+        warning(
+          hostInstance.parentNode === container,
+          'render(...): It looks like the React-rendered content of this ' +
+            'container was removed without using React. This is not ' +
+            'supported and will cause errors. Instead, call ' +
+            'ReactDOM.unmountComponentAtNode to empty a container.',
+        );
+      }
+    }
+
     const isRootRenderedBySomeReact = !!container._reactRootContainer;
     const rootEl = getReactRootElementInContainer(container);
     const hasNonRootReactChild = !!(rootEl &&

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

@@ -1004,6 +1004,81 @@ describe('ReactDOMFiber', () => {
         container,
       );
     });
+
+    it('should not warn when rendering into an empty container', () => {
+      spyOn(console, 'error');
+      ReactDOM.render(<div>foo</div>, container);
+      expect(container.innerHTML).toBe('<div>foo</div>');
+      ReactDOM.render(null, container);
+      expect(container.innerHTML).toBe('');
+      expectDev(console.error.calls.count()).toBe(0);
+      ReactDOM.render(<div>bar</div>, container);
+      expect(container.innerHTML).toBe('<div>bar</div>');
+      expectDev(console.error.calls.count()).toBe(0);
+    });
+
+    it('should warn when replacing a container which was manually updated outside of React', () => {
+      spyOn(console, 'error');
+      // when not messing with the DOM outside of React
+      ReactDOM.render(<div key="1">foo</div>, container);
+      ReactDOM.render(<div key="1">bar</div>, container);
+      expect(container.innerHTML).toBe('<div>bar</div>');
+      // then we mess with the DOM before an update
+      // we know this will error - that is expected right now
+      // It's an error of type 'NotFoundError' with no message
+      expect(() => {
+        container.innerHTML = '<div>MEOW.</div>';
+        ReactDOM.render(<div key="2">baz</div>, container);
+      }).toThrowError();
+      expectDev(console.error.calls.count()).toBe(1);
+      expectDev(console.error.calls.argsFor(0)[0]).toContain(
+        'render(...): ' +
+          'It looks like the React-rendered content of this container was ' +
+          'removed without using React. This is not supported and will ' +
+          'cause errors. Instead, call ReactDOM.unmountComponentAtNode ' +
+          'to empty a container.',
+      );
+    });
+
+    it('should warn when doing an update to a container manually updated outside of React', () => {
+      spyOn(console, 'error');
+      // when not messing with the DOM outside of React
+      ReactDOM.render(<div>foo</div>, container);
+      ReactDOM.render(<div>bar</div>, container);
+      expect(container.innerHTML).toBe('<div>bar</div>');
+      // then we mess with the DOM before an update
+      container.innerHTML = '<div>MEOW.</div>';
+      ReactDOM.render(<div>baz</div>, container);
+      // silently fails to update
+      expectDev(console.error.calls.count()).toBe(1);
+      expectDev(console.error.calls.argsFor(0)[0]).toContain(
+        'render(...): ' +
+          'It looks like the React-rendered content of this container was ' +
+          'removed without using React. This is not supported and will ' +
+          'cause errors. Instead, call ReactDOM.unmountComponentAtNode ' +
+          'to empty a container.',
+      );
+    });
+
+    it('should warn when doing an update to a container manually cleared outside of React', () => {
+      spyOn(console, 'error');
+      // when not messing with the DOM outside of React
+      ReactDOM.render(<div>foo</div>, container);
+      ReactDOM.render(<div>bar</div>, container);
+      expect(container.innerHTML).toBe('<div>bar</div>');
+      // then we mess with the DOM before an update
+      container.innerHTML = '';
+      ReactDOM.render(<div>baz</div>, container);
+      // silently fails to update
+      expectDev(console.error.calls.count()).toBe(1);
+      expectDev(console.error.calls.argsFor(0)[0]).toContain(
+        'render(...): ' +
+          'It looks like the React-rendered content of this container was ' +
+          'removed without using React. This is not supported and will ' +
+          'cause errors. Instead, call ReactDOM.unmountComponentAtNode ' +
+          'to empty a container.',
+      );
+    });
   }
 });
 

src/renderers/dom/shared/__tests__/ReactDOMTextComponent-test.js

@@ -120,6 +120,8 @@ describe('ReactDOMTextComponent', () => {
     ReactDOM.render(reactEl, el);
     expect(el.textContent).toBe('foobarbaz');
 
+    ReactDOM.unmountComponentAtNode(el);
+
     reactEl = <div>{''}{''}{''}</div>;
     el.innerHTML = ReactDOMServer.renderToString(reactEl);
 

src/renderers/dom/shared/__tests__/ReactMount-test.js

@@ -12,6 +12,7 @@
 'use strict';
 
 const ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
+const {COMMENT_NODE} = require('HTMLNodeType');
 
 const invariant = require('invariant');
 
@@ -371,7 +372,7 @@ describe('ReactMount', () => {
         containerDiv = document.createElement('div');
         containerDiv.innerHTML = 'A<!-- react-mount-point-unstable -->B';
         mountPoint = containerDiv.childNodes[1];
-        invariant(mountPoint.nodeType === 8, 'Expected comment');
+        invariant(mountPoint.nodeType === COMMENT_NODE, 'Expected comment');
       });
 
       it('renders at a comment node', () => {

src/renderers/shared/fiber/ReactFiberReconciler.js

@@ -37,7 +37,10 @@ if (__DEV__) {
   var getComponentName = require('getComponentName');
 }
 
-var {findCurrentHostFiber} = require('ReactFiberTreeReflection');
+var {
+  findCurrentHostFiber,
+  findCurrentHostFiberWithNoPortals,
+} = require('ReactFiberTreeReflection');
 
 var getContextForSubtree = require('getContextForSubtree');
 
@@ -173,6 +176,9 @@ export type Reconciler<C, I, TI> = {
 
   // Use for findDOMNode/findHostNode. Legacy API.
   findHostInstance(component: Fiber): I | TI | null,
+
+  // Used internally for filtering out portals. Legacy API.
+  findHostInstanceWithNoPortals(component: Fiber): I | TI | null,
 };
 
 getContextForSubtree._injectFiber(function(fiber: Fiber) {
@@ -310,5 +316,13 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
       }
       return hostFiber.stateNode;
     },
+
+    findHostInstanceWithNoPortals(fiber: Fiber): PI | null {
+      const hostFiber = findCurrentHostFiberWithNoPortals(fiber);
+      if (hostFiber === null) {
+        return null;
+      }
+      return hostFiber.stateNode;
+    },
   };
 };

src/renderers/shared/fiber/ReactFiberTreeReflection.js

@@ -25,10 +25,11 @@ if (__DEV__) {
 }
 
 var {
-  HostRoot,
+  ClassComponent,
   HostComponent,
+  HostRoot,
+  HostPortal,
   HostText,
-  ClassComponent,
 } = require('ReactTypeOfWork');
 
 var {NoEffect, Placement} = require('ReactTypeOfSideEffect');
@@ -242,7 +243,41 @@ exports.findCurrentHostFiber = function(parent: Fiber): Fiber | null {
     if (node.tag === HostComponent || node.tag === HostText) {
       return node;
     } else if (node.child) {
-      // TODO: If we hit a Portal, we're supposed to skip it.
+      node.child.return = node;
+      node = node.child;
+      continue;
+    }
+    if (node === currentParent) {
+      return null;
+    }
+    while (!node.sibling) {
+      if (!node.return || node.return === currentParent) {
+        return null;
+      }
+      node = node.return;
+    }
+    node.sibling.return = node.return;
+    node = node.sibling;
+  }
+  // Flow needs the return null here, but ESLint complains about it.
+  // eslint-disable-next-line no-unreachable
+  return null;
+};
+
+exports.findCurrentHostFiberWithNoPortals = function(
+  parent: Fiber,
+): Fiber | null {
+  const currentParent = findCurrentFiberUsingSlowPath(parent);
+  if (!currentParent) {
+    return null;
+  }
+
+  // Next we'll drill down this component to find the first HostComponent/Text.
+  let node: Fiber = currentParent;
+  while (true) {
+    if (node.tag === HostComponent || node.tag === HostText) {
+      return node;
+    } else if (node.child && node.tag !== HostPortal) {
       node.child.return = node;
       node = node.child;
       continue;