Skip to content

Add warning for rendering into container that was updated manually #10210

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
845d349
RFC Add warning for rendering into container that was updated manually
flarnie Jul 18, 2017
72dfbaf
Add test and tweak check for rendering into manually updated container
flarnie Jul 18, 2017
a439ba6
Stub our `console.error` in one of the portal tests
flarnie Jul 18, 2017
5a7da89
Skip warning in 'ReactDOMFiberEntry' when mounting to Comment node
flarnie Jul 19, 2017
cfd3c35
Improve warning message, remove dedup check, and unmock console.error
flarnie Jul 19, 2017
fff93e3
Possible fix for issue of incorrect warning for portal re-render
flarnie Jul 19, 2017
178ac2b
Fix logic for checking if the host instance container is a portal
flarnie Jul 19, 2017
c9a6e6c
Clean up new tests in ReactDOMFiber-test
flarnie Jul 19, 2017
194adf1
Add 'unmountComponentAtNode' call in test for reconciling pre-rendere…
flarnie Jul 19, 2017
a2fdfd2
ran prettier
flarnie Jul 19, 2017
48f8f83
remove unused variable
flarnie Jul 19, 2017
64ce334
run scripts/fiber/record-tests
flarnie Jul 19, 2017
c927309
Fix type error and improve name of portal container flag
flarnie Jul 19, 2017
7f9648b
Add flag to portalContainer on mount instead of in `createPortal`
flarnie Jul 19, 2017
2225b98
Force an 'any' type for the `hostInstance.parentNode` in warning check
flarnie Jul 19, 2017
f7efe64
Ignore portals in `DOMRenderer.findHostInstance`
flarnie Jul 20, 2017
e64c2c1
Ran prettier
flarnie Jul 20, 2017
0b0035c
Remove error snapshot test
flarnie Jul 20, 2017
178e1a4
Remove expando that we were adding to portal container
flarnie Jul 20, 2017
f9edf89
Fork `findHostInstance` to make `findHostInstanceWithNoPortals`
flarnie Jul 20, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
23 changes: 21 additions & 2 deletions src/renderers/dom/fiber/ReactDOMFiberEntry.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -531,6 +535,21 @@ function renderSubtreeIntoContainer(
);

if (__DEV__) {
if (container._reactRootContainer && container.nodeType !== COMMENT_NODE) {
const hostInstance = DOMRenderer.findHostInstance(
container._reactRootContainer.current,
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As suggested by @spicyj we could expose another method here that doesn't drill through portals. The problem with that is dead code elimination. It's a bit awkward that we expose a new method on the public Reconciler API that is only ever needed for a specific DEV warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it would be possible to write such a method, without adding a flag to the portalContainer node, then there must already be something we could look at here that would tell us if it's a portal. Is that right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No because the portal is between the container and the hostInstance.

HostRoot -> ... -> HostPortal -> ... -> HostComponent

You'd have to traverse the Fibers to figure that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok. Interesting.

What is the advantage of adding another method to DOMRenderer rather than setting a boolean flag on the portalContainer node?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lets us not mutate the node in what seems to be a pure function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it help that we now do the mutating inside of ReactFiberCommitWork? That is where we do other DOM updates.

If we really want to avoid adding this flag then I can implement another method on DOMRenderer but I think @sebmarkbage's point that "It's a bit awkward that we expose a new method on the public Reconciler API that is only ever needed for a specific DEV warning." is valid, and I'd rather avoid that.

Copy link
Collaborator

@sebmarkbage sebmarkbage Jul 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a situation like this: render([<Portal />,<div />], container) we'd find the portal node and then ignore checking the second one. So no warning if we mutated container. If we did @spicyj's suggestion, we could find all the direct children or at least the first one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - talked more with @spicyj offline also and it looks like we may also just be able to change this spot to filter portals out, and then we might not need a new method?

// TODO: If we hit a Portal, we're supposed to skip it.

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 &&
Expand Down
75 changes: 75 additions & 0 deletions src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.',
);
});
}
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
3 changes: 2 additions & 1 deletion src/renderers/dom/shared/__tests__/ReactMount-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
'use strict';

const ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
const {COMMENT_NODE} = require('HTMLNodeType');

const invariant = require('invariant');

Expand Down Expand Up @@ -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', () => {
Expand Down
8 changes: 4 additions & 4 deletions src/renderers/shared/fiber/ReactFiberTreeReflection.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,11 @@ if (__DEV__) {
}

var {
HostRoot,
ClassComponent,
HostComponent,
HostRoot,
HostPortal,
HostText,
ClassComponent,
} = require('ReactTypeOfWork');

var {NoEffect, Placement} = require('ReactTypeOfSideEffect');
Expand Down Expand Up @@ -241,8 +242,7 @@ exports.findCurrentHostFiber = function(parent: Fiber): Fiber | null {
while (true) {
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.
} else if (node.child && node.tag !== HostPortal) {
node.child.return = node;
node = node.child;
continue;
Expand Down