Skip to content
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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all 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.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 &&
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
16 changes: 15 additions & 1 deletion src/renderers/shared/fiber/ReactFiberReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@ if (__DEV__) {
var getComponentName = require('getComponentName');
}

var {findCurrentHostFiber} = require('ReactFiberTreeReflection');
var {
findCurrentHostFiber,
findCurrentHostFiberWithNoPortals,
} = require('ReactFiberTreeReflection');

var getContextForSubtree = require('getContextForSubtree');

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -309,5 +315,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;
},
};
};
41 changes: 38 additions & 3 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 @@ -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;
Expand Down