Skip to content

Commit

Permalink
[Fiber] Nesting validation warnings (#8586)
Browse files Browse the repository at this point in the history
* (WIP) Nesting warnings

* Remove indirection

* Add a note about namespace

* Fix Flow and make host context required

This makes it easier to avoid accidental nulls.
I also added a test for the production case to avoid regressing because of __DEV__ branches.
  • Loading branch information
gaearon authored Dec 17, 2016
1 parent e1eccbf commit 70f704d
Show file tree
Hide file tree
Showing 15 changed files with 420 additions and 181 deletions.
4 changes: 0 additions & 4 deletions scripts/fiber/tests-passing-except-dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,6 @@ src/addons/transitions/__tests__/ReactTransitionGroup-test.js

src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
* should not warn when server-side rendering `onScroll`
* warns on invalid nesting
* warns on invalid nesting at root
* warns nicely for table rows
* gives useful context in warnings
* should warn about incorrect casing on properties (ssr)
* should warn about incorrect casing on event handlers (ssr)
* should warn about class (ssr)
Expand Down
5 changes: 5 additions & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,7 @@ src/renderers/dom/__tests__/ReactDOMProduction-test.js
* should use prod React
* should handle a simple flow
* should call lifecycle methods
* should keep track of namespace across portals in production

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
* should render strings as children
Expand Down Expand Up @@ -690,6 +691,10 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
* should throw when an attack vector is used server-side
* should throw when an invalid tag name is used
* should throw when an attack vector is used
* warns on invalid nesting
* warns on invalid nesting at root
* warns nicely for table rows
* gives useful context in warnings
* should warn about incorrect casing on properties
* should warn about incorrect casing on event handlers
* should warn about class
Expand Down
7 changes: 6 additions & 1 deletion src/renderers/art/ReactARTFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ require('art/modes/current').setCurrent(
const Mode = require('art/modes/current');
const Transform = require('art/core/transform');
const invariant = require('fbjs/lib/invariant');
const emptyObject = require('emptyObject');
const React = require('React');
const ReactFiberReconciler = require('ReactFiberReconciler');

Expand Down Expand Up @@ -485,8 +486,12 @@ const ARTRenderer = ReactFiberReconciler({
// Noop
},

getRootHostContext() {
return emptyObject;
},

getChildHostContext() {
return null;
return emptyObject;
},

scheduleAnimationCallback: window.requestAnimationFrame,
Expand Down
59 changes: 59 additions & 0 deletions src/renderers/dom/__tests__/ReactDOMProduction-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
describe('ReactDOMProduction', () => {
var oldProcess;

var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');

var React;
var ReactDOM;

Expand Down Expand Up @@ -193,4 +195,61 @@ describe('ReactDOMProduction', () => {
' for full errors and additional helpful warnings.'
);
});

if (ReactDOMFeatureFlags.useFiber) {
// This test is originally from ReactDOMFiber-test but we replicate it here
// to avoid production-only regressions because of host context differences
// in dev and prod.
it('should keep track of namespace across portals in production', () => {
var svgEls, htmlEls;
var expectSVG = {ref: el => svgEls.push(el)};
var expectHTML = {ref: el => htmlEls.push(el)};
var usePortal = function(tree) {
return ReactDOM.unstable_createPortal(
tree,
document.createElement('div')
);
};
var assertNamespacesMatch = function(tree) {
var container = document.createElement('div');
svgEls = [];
htmlEls = [];
ReactDOM.render(tree, container);
svgEls.forEach(el => {
expect(el.namespaceURI).toBe('http://www.w3.org/2000/svg');
});
htmlEls.forEach(el => {
expect(el.namespaceURI).toBe('http://www.w3.org/1999/xhtml');
});
ReactDOM.unmountComponentAtNode(container);
expect(container.innerHTML).toBe('');
};

assertNamespacesMatch(
<div {...expectHTML}>
<svg {...expectSVG}>
<foreignObject {...expectSVG}>
<p {...expectHTML} />
{usePortal(
<svg {...expectSVG}>
<image {...expectSVG} />
<svg {...expectSVG}>
<image {...expectSVG} />
<foreignObject {...expectSVG}>
<p {...expectHTML} />
</foreignObject>
{usePortal(<p {...expectHTML} />)}
</svg>
<image {...expectSVG} />
</svg>
)}
<p {...expectHTML} />
</foreignObject>
<image {...expectSVG} />
</svg>
<p {...expectHTML} />
</div>
);
});
}
});
82 changes: 75 additions & 7 deletions src/renderers/dom/fiber/ReactDOMFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ var {
} = ReactDOMFiberComponent;
var { precacheFiberNode } = ReactDOMComponentTree;

if (__DEV__) {
var validateDOMNesting = require('validateDOMNesting');
var { updatedAncestorInfo } = validateDOMNesting;
}

const DOCUMENT_NODE = 9;

ReactDOMInjection.inject();
Expand All @@ -52,17 +57,44 @@ findDOMNode._injectFiber(function(fiber: Fiber) {
type DOMContainerElement = Element & { _reactRootContainer: ?Object };

type Container = Element;
type Props = { className ?: string };
type Props = { children ?: mixed };
type Instance = Element;
type TextInstance = Text;

type HostContextDev = {
namespace : string,
ancestorInfo : mixed,
};
type HostContextProd = string;
type HostContext = HostContextDev | HostContextProd;

let eventsEnabled : ?boolean = null;
let selectionInformation : ?mixed = null;

var DOMRenderer = ReactFiberReconciler({

getChildHostContext(parentHostContext : string | null, type : string) {
const parentNamespace = parentHostContext;
getRootHostContext(rootContainerInstance : Container) : HostContext {
const type = rootContainerInstance.tagName.toLowerCase();
if (__DEV__) {
const namespace = getChildNamespace(null, type);
const isMountingIntoDocument = rootContainerInstance.ownerDocument.documentElement === rootContainerInstance;
const ancestorInfo = updatedAncestorInfo(null, isMountingIntoDocument ? '#document' : type, null);
return {namespace, ancestorInfo};
}
return getChildNamespace(null, type);
},

getChildHostContext(
parentHostContext : HostContext,
type : string,
) : HostContext {
if (__DEV__) {
const parentHostContextDev = ((parentHostContext : any) : HostContextDev);
const namespace = getChildNamespace(parentHostContextDev.namespace, type);
const ancestorInfo = updatedAncestorInfo(parentHostContextDev.ancestorInfo, type, null);
return {namespace, ancestorInfo};
}
const parentNamespace = ((parentHostContext : any) : HostContextProd);
return getChildNamespace(parentNamespace, type);
},

Expand All @@ -83,10 +115,26 @@ var DOMRenderer = ReactFiberReconciler({
type : string,
props : Props,
rootContainerInstance : Container,
hostContext : string | null,
hostContext : HostContext,
internalInstanceHandle : Object,
) : Instance {
const domElement : Instance = createElement(type, props, rootContainerInstance, hostContext);
let parentNamespace : string;
if (__DEV__) {
// TODO: take namespace into account when validating.
const hostContextDev = ((hostContext : any) : HostContextDev);
validateDOMNesting(type, null, null, hostContextDev.ancestorInfo);
if (
typeof props.children === 'string' ||
typeof props.children === 'number'
) {
const ownAncestorInfo = updatedAncestorInfo(hostContextDev.ancestorInfo, type, null);
validateDOMNesting(null, String(props.children), null, ownAncestorInfo);
}
parentNamespace = hostContextDev.namespace;
} else {
parentNamespace = ((hostContext : any) : HostContextProd);
}
const domElement : Instance = createElement(type, props, rootContainerInstance, parentNamespace);
precacheFiberNode(internalInstanceHandle, domElement);
return domElement;
},
Expand All @@ -108,8 +156,19 @@ var DOMRenderer = ReactFiberReconciler({
domElement : Instance,
type : string,
oldProps : Props,
newProps : Props
newProps : Props,
hostContext : HostContext,
) : boolean {
if (__DEV__) {
const hostContextDev = ((hostContext : any) : HostContextDev);
if (typeof newProps.children !== typeof oldProps.children && (
typeof newProps.children === 'string' ||
typeof newProps.children === 'number'
)) {
const ownAncestorInfo = updatedAncestorInfo(hostContextDev.ancestorInfo, type, null);
validateDOMNesting(null, String(newProps.children), null, ownAncestorInfo);
}
}
return true;
},

Expand Down Expand Up @@ -143,7 +202,16 @@ var DOMRenderer = ReactFiberReconciler({
domElement.textContent = '';
},

createTextInstance(text : string, rootContainerInstance : Container, internalInstanceHandle : Object) : TextInstance {
createTextInstance(
text : string,
rootContainerInstance : Container,
hostContext : HostContext,
internalInstanceHandle : Object
) : TextInstance {
if (__DEV__) {
const hostContextDev = ((hostContext : any) : HostContextDev);
validateDOMNesting(null, text, null, hostContextDev.ancestorInfo);
}
var textNode : TextInstance = document.createTextNode(text);
precacheFiberNode(internalInstanceHandle, textNode);
return textNode;
Expand Down
24 changes: 14 additions & 10 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ var STYLE = 'style';
var HTML = '__html';

var {
html: HTML_NAMESPACE,
svg: SVG_NAMESPACE,
mathml: MATH_NAMESPACE,
} = DOMNamespaces;
Expand Down Expand Up @@ -451,26 +452,26 @@ function updateDOMProperties(
}

// Assumes there is no parent namespace.
function getIntrinsicNamespace(type : string) : string | null {
function getIntrinsicNamespace(type : string) : string {
switch (type) {
case 'svg':
return SVG_NAMESPACE;
case 'math':
return MATH_NAMESPACE;
default:
return null;
return HTML_NAMESPACE;
}
}

var ReactDOMFiberComponent = {
getChildNamespace(parentNamespace : string | null, type : string) : string | null {
if (parentNamespace == null) {
// No parent namespace: potential entry point.
getChildNamespace(parentNamespace : string | null, type : string) : string {
if (parentNamespace == null || parentNamespace === HTML_NAMESPACE) {
// No (or default) parent namespace: potential entry point.
return getIntrinsicNamespace(type);
}
if (parentNamespace === SVG_NAMESPACE && type === 'foreignObject') {
// We're leaving SVG.
return null;
return HTML_NAMESPACE;
}
// By default, pass namespace below.
return parentNamespace;
Expand All @@ -480,14 +481,17 @@ var ReactDOMFiberComponent = {
type : string,
props : Object,
rootContainerElement : Element,
parentNamespace : string | null
parentNamespace : string
) : Element {
// We create tags in the namespace of their parent container, except HTML
// tags get no namespace.
var ownerDocument = rootContainerElement.ownerDocument;
var domElement : Element;
var namespaceURI = parentNamespace || getIntrinsicNamespace(type);
if (namespaceURI == null) {
var namespaceURI = parentNamespace;
if (namespaceURI === HTML_NAMESPACE) {
namespaceURI = getIntrinsicNamespace(type);
}
if (namespaceURI === HTML_NAMESPACE) {
if (__DEV__) {
warning(
type === type.toLowerCase() ||
Expand Down Expand Up @@ -649,7 +653,7 @@ var ReactDOMFiberComponent = {
tag : string,
lastRawProps : Object,
nextRawProps : Object,
rootContainerElement : Element
rootContainerElement : Element,
) : void {
if (__DEV__) {
validatePropertiesInDevelopment(tag, nextRawProps);
Expand Down
9 changes: 2 additions & 7 deletions src/renderers/dom/fiber/wrappers/ReactDOMFiberOption.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,25 +15,20 @@
var React = require('React');

var warning = require('warning');
var didWarnInvalidOptionChildren = false;

function flattenChildren(children) {
var content = '';

// Flatten children and warn if they aren't strings or numbers;
// invalid types are ignored.
// We can silently skip them because invalid DOM nesting warning
// catches these cases in Fiber.
React.Children.forEach(children, function(child) {
if (child == null) {
return;
}
if (typeof child === 'string' || typeof child === 'number') {
content += child;
} else if (!didWarnInvalidOptionChildren) {
didWarnInvalidOptionChildren = true;
warning(
false,
'Only strings and numbers are supported as <option> children.'
);
}
});

Expand Down
Loading

0 comments on commit 70f704d

Please sign in to comment.