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

[Fiber] Nesting validation warnings #8586

Merged
merged 4 commits into from
Dec 17, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gaearon This doesn't seem right. If I'm rendering SVG into an SVG tree, then the current namespace isn't HTML and the tag name isn't enough to determine it since I could be rendering into any SVG root. It seems to me that we should be reading the namespace of the root instead of the tagName.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

},

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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was a bit easier to keep nesting checks in ReactDOMFiber than shoving them inside ReactDOMFiberComponent. As a result I'm dealing with rawProps rather than "host props" and validate them earlier than in Stack. I could have fixed this but it felt like the nesting check is already generic enough that we can just remove this special case. I could also remove it in Stack if needed.

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