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.

Sign up for GitHub

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

Jump to bottom

[Fiber] Nesting validation warnings #8586

Merged
merged 4 commits into from
Dec 17, 2016
Merged

[Fiber] Nesting validation warnings #8586

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d36ba98
(WIP) Nesting warnings
gaearon Dec 17, 2016
fd036bd
Remove indirection
gaearon Dec 17, 2016
997fe3a
Add a note about namespace
gaearon Dec 17, 2016
414f1ec
Fix Flow and make host context required
gaearon Dec 17, 2016
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
View file
Open in desktop
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
4 changes: 4 additions & 0 deletions scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,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
4 changes: 4 additions & 0 deletions src/renderers/art/ReactARTFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,10 @@ const ARTRenderer = ReactFiberReconciler({
// Noop
},

getRootHostContext() {
return null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIRC you wanted something like this anyway.

},

getChildHostContext() {
return null;
},
Expand Down
71 changes: 65 additions & 6 deletions src/renderers/dom/fiber/ReactDOMFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,15 @@ var {
} = ReactDOMFiberComponent;
var { precacheFiberNode } = ReactDOMComponentTree;

if (__DEV__) {
var {
getChildAncestorInfo,
validateElementNesting,
validateTextNesting,
validateInlineTextNesting,
} = ReactDOMFiberComponent;
}

const DOCUMENT_NODE = 9;

ReactDOMInjection.inject();
Expand All @@ -61,9 +70,29 @@ let selectionInformation : ?mixed = null;

var DOMRenderer = ReactFiberReconciler({

getChildHostContext(parentHostContext : string | null, type : string) {
const parentNamespace = parentHostContext;
return getChildNamespace(parentNamespace, type);
getRootHostContext(rootContainerInstance) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One benefit of this is that we should be able to mount SVG content into an svg root now.
Can add tests for this.

const type = rootContainerInstance.tagName.toLowerCase();
if (__DEV__) {
const namespace = getChildNamespace(null, type);
const isMountingIntoDocument = rootContainerInstance.ownerDocument.documentElement === rootContainerInstance;
const ancestorInfo = getChildAncestorInfo(null, isMountingIntoDocument ? '#document' : type);
Copy link
Collaborator

@sebmarkbage sebmarkbage Dec 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Does the child nesting validation not case about the namespace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. It doesn't (in Stack neither), and this is a bug. Because that should be valid: https://jsfiddle.net/q7e61jpd/.

return {namespace, ancestorInfo};
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: We don't really use else with DEV blocks because it didn't use to work in our transforms but I actually prefer that pattern because it enforces the notion that __DEV__ is only additional and doesn't change behavior. For this case I tend to prefer to use an early return instead. Although in this case I guess it severely changes the whole return type.

return getChildNamespace(null, type);
}
},

getChildHostContext(
parentHostContext : string,
type : string,
) {
if (__DEV__) {
const namespace = getChildNamespace(parentHostContext.namespace, type);
const ancestorInfo = getChildAncestorInfo(parentHostContext.ancestorInfo, type);
return {namespace, ancestorInfo};
} else {
return getChildNamespace(parentHostContext, type);
}
},

prepareForCommit() : void {
Expand All @@ -86,7 +115,20 @@ var DOMRenderer = ReactFiberReconciler({
hostContext : string | null,
internalInstanceHandle : Object,
) : Instance {
const domElement : Instance = createElement(type, props, rootContainerInstance, hostContext);
let parentNamespace;
if (__DEV__) {
validateElementNesting(hostContext.ancestorInfo, type);
if (
typeof props.children === 'string' ||
typeof props.children === 'number'
) {
validateInlineTextNesting(hostContext.ancestorInfo, type, String(props.children));
}
parentNamespace = hostContext.namespace;
} else {
parentNamespace = hostContext;
}
const domElement : Instance = createElement(type, props, rootContainerInstance, parentNamespace);
precacheFiberNode(internalInstanceHandle, domElement);
return domElement;
},
Expand All @@ -108,8 +150,17 @@ var DOMRenderer = ReactFiberReconciler({
domElement : Instance,
type : string,
oldProps : Props,
newProps : Props
newProps : Props,
hostContext : string
) : boolean {
if (__DEV__) {
if (oldProps.children !== newProps.children && (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should probably check that we're moving to having a non-reified child here instead of always running the check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also ^^^ the day I started saying "reified". What have you done to me.

typeof newProps.children === 'string' ||
typeof newProps.children === 'number'
)) {
validateInlineTextNesting(hostContext.ancestorInfo, type, String(newProps.children));
}
}
return true;
},

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

createTextInstance(text : string, rootContainerInstance : Container, internalInstanceHandle : Object) : TextInstance {
createTextInstance(
text : string,
rootContainerInstance : Container,
hostContext,
internalInstanceHandle : Object
) : TextInstance {
if (__DEV__) {
validateTextNesting(hostContext.ancestorInfo, text);
}
var textNode : TextInstance = document.createTextNode(text);
precacheFiberNode(internalInstanceHandle, textNode);
return textNode;
Expand Down
20 changes: 19 additions & 1 deletion src/renderers/dom/fiber/ReactDOMFiberComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ if (__DEV__) {
var ReactDOMInvalidARIAHook = require('ReactDOMInvalidARIAHook');
var ReactDOMNullInputValuePropHook = require('ReactDOMNullInputValuePropHook');
var ReactDOMUnknownPropertyHook = require('ReactDOMUnknownPropertyHook');
var validateDOMNesting = require('validateDOMNesting');
var { updatedAncestorInfo } = validateDOMNesting;
var { validateProperties: validateARIAProperties } = ReactDOMInvalidARIAHook;
var { validateProperties: validateInputPropertes } = ReactDOMNullInputValuePropHook;
var { validateProperties: validateUnknownPropertes } = ReactDOMUnknownPropertyHook;
Expand Down Expand Up @@ -649,7 +651,7 @@ var ReactDOMFiberComponent = {
tag : string,
lastRawProps : Object,
nextRawProps : Object,
rootContainerElement : Element
rootContainerElement : Element,
) : void {
if (__DEV__) {
validatePropertiesInDevelopment(tag, nextRawProps);
Expand Down Expand Up @@ -731,4 +733,20 @@ var ReactDOMFiberComponent = {

};

if (__DEV__) {
ReactDOMFiberComponent.getChildAncestorInfo = function(parentAncestorInfo, type) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like these forwarding things. We have so much of these in the event system that just adds bloat, add risk for cyclic dependencies and confusion.

Do you have a plan with these or can you just move this to ReactDOMFiber?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, the only reason for a separate ReactDOMFiberComponent file is to keep parity with Stack so that we can easier replicate changes. Should get rid of this file later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. I thought I'd need them inside but then turned out I didn't. Will remove.

return updatedAncestorInfo(parentAncestorInfo, type, null);
};
ReactDOMFiberComponent.validateElementNesting = function(parentAncestorInfo, type) {
validateDOMNesting(type, null, null, parentAncestorInfo);
};
ReactDOMFiberComponent.validateTextNesting = function(parentAncestorInfo, text) {
validateDOMNesting(null, text, null, parentAncestorInfo);
};
ReactDOMFiberComponent.validateInlineTextNesting = function(parentAncestorInfo, type, text) {
const ownAncestorInfo = updatedAncestorInfo(parentAncestorInfo, type, null);
validateDOMNesting(null, text, null, ownAncestorInfo);
};
}

module.exports = ReactDOMFiberComponent;
9 changes: 2 additions & 7 deletions src/renderers/dom/fiber/wrappers/ReactDOMFiberOption.js
View file
Open in desktop
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
115 changes: 90 additions & 25 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1272,10 +1272,15 @@ describe('ReactDOMComponent', () => {
spyOn(console, 'error');
ReactTestUtils.renderIntoDocument(<div><tr /><tr /></div>);

var addendum = ReactDOMFeatureFlags.useFiber ?
'\n in tr (at **)' +
'\n in div (at **)' :
' See div > tr.';

expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toBe(
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: validateDOMNesting(...): <tr> cannot appear as a child of ' +
'<div>. See div > tr.'
'<div>.' + addendum
);
});

Expand All @@ -1284,10 +1289,16 @@ describe('ReactDOMComponent', () => {
var p = document.createElement('p');
ReactDOM.render(<span><p /></span>, p);

var addendum = ReactDOMFeatureFlags.useFiber ?
// There is no outer `p` here because root container is not part of the stack.
'\n in p (at **)' +
'\n in span (at **)' :
' See p > ... > p.';

expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toBe(
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: validateDOMNesting(...): <p> cannot appear as a descendant ' +
'of <p>. See p > ... > p.'
'of <p>.' + addendum
);
});

Expand All @@ -1307,22 +1318,39 @@ describe('ReactDOMComponent', () => {
}

ReactTestUtils.renderIntoDocument(<Foo />);

expectDev(console.error.calls.count()).toBe(3);
expectDev(console.error.calls.argsFor(0)[0]).toBe(

var addendum1 = ReactDOMFeatureFlags.useFiber ?
'\n in tr (at **)' +
'\n in Row (at **)' +
'\n in table (at **)' +
'\n in Foo (at **)' :
' See Foo > table > Row > tr.';
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: validateDOMNesting(...): <tr> cannot appear as a child of ' +
'<table>. See Foo > table > Row > tr. Add a <tbody> to your code to ' +
'match the DOM tree generated by the browser.'
'<table>. Add a <tbody> to your code to match the DOM tree generated ' +
'by the browser.' + addendum1
);
expectDev(console.error.calls.argsFor(1)[0]).toBe(

var addendum2 = ReactDOMFeatureFlags.useFiber ?
'\n in tr (at **)' +
'\n in Row (at **)' +
'\n in table (at **)' +
'\n in Foo (at **)' :
' See Row > tr > #text.';
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toBe(
'Warning: validateDOMNesting(...): Text nodes cannot appear as a ' +
'child of <tr>. See Row > tr > #text.'
'child of <tr>.' + addendum2
);
expectDev(console.error.calls.argsFor(2)[0]).toBe(

var addendum3 = ReactDOMFeatureFlags.useFiber ?
'\n in table (at **)' +
'\n in Foo (at **)' :
' See Foo > table > #text.';
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(2)[0])).toBe(
'Warning: validateDOMNesting(...): Whitespace text nodes cannot ' +
'appear as a child of <table>. Make sure you don\'t have any extra ' +
'whitespace between tags on each line of your source code. See Foo > ' +
'table > #text.'
'whitespace between tags on each line of your source code.' + addendum3
);
});

Expand Down Expand Up @@ -1355,8 +1383,14 @@ describe('ReactDOMComponent', () => {
});
ReactTestUtils.renderIntoDocument(<App1 />);
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'See Viz1 > table > FancyRow > Row > tr.'
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toContain(
ReactDOMFeatureFlags.useFiber ?
'\n in tr (at **)' +
'\n in Row (at **)' +
'\n in FancyRow (at **)' +
'\n in table (at **)' +
'\n in Viz1 (at **)' :
'See Viz1 > table > FancyRow > Row > tr.'
);

var Viz2 = React.createClass({
Expand All @@ -1367,26 +1401,51 @@ describe('ReactDOMComponent', () => {
});
ReactTestUtils.renderIntoDocument(<App2 />);
expectDev(console.error.calls.count()).toBe(2);
expectDev(console.error.calls.argsFor(1)[0]).toContain(
'See Viz2 > FancyTable > Table > table > FancyRow > Row > tr.'
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toContain(
ReactDOMFeatureFlags.useFiber ?
'\n in tr (at **)' +
'\n in Row (at **)' +
'\n in FancyRow (at **)' +
'\n in table (at **)' +
'\n in Table (at **)' +
'\n in FancyTable (at **)' +
'\n in Viz2 (at **)' :
'See Viz2 > FancyTable > Table > table > FancyRow > Row > tr.'
);

ReactTestUtils.renderIntoDocument(<FancyTable><FancyRow /></FancyTable>);
expectDev(console.error.calls.count()).toBe(3);
expectDev(console.error.calls.argsFor(2)[0]).toContain(
'See FancyTable > Table > table > FancyRow > Row > tr.'
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(2)[0])).toContain(
ReactDOMFeatureFlags.useFiber ?
'\n in tr (at **)' +
'\n in Row (at **)' +
'\n in FancyRow (at **)' +
'\n in table (at **)' +
'\n in Table (at **)' +
'\n in FancyTable (at **)' :
'See FancyTable > Table > table > FancyRow > Row > tr.'
);

ReactTestUtils.renderIntoDocument(<table><FancyRow /></table>);
expectDev(console.error.calls.count()).toBe(4);
expectDev(console.error.calls.argsFor(3)[0]).toContain(
'See table > FancyRow > Row > tr.'
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(3)[0])).toContain(
ReactDOMFeatureFlags.useFiber ?
'\n in tr (at **)' +
'\n in Row (at **)' +
'\n in FancyRow (at **)' +
'\n in table (at **)' :
'See table > FancyRow > Row > tr.'
);

ReactTestUtils.renderIntoDocument(<FancyTable><tr /></FancyTable>);
expectDev(console.error.calls.count()).toBe(5);
expectDev(console.error.calls.argsFor(4)[0]).toContain(
'See FancyTable > Table > table > tr.'
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(4)[0])).toContain(
ReactDOMFeatureFlags.useFiber ?
'\n in tr (at **)' +
'\n in table (at **)' +
'\n in Table (at **)' +
'\n in FancyTable (at **)' :
'See FancyTable > Table > table > tr.'
);

class Link extends React.Component {
Expand All @@ -1397,8 +1456,14 @@ describe('ReactDOMComponent', () => {

ReactTestUtils.renderIntoDocument(<Link><div><Link /></div></Link>);
expectDev(console.error.calls.count()).toBe(6);
expectDev(console.error.calls.argsFor(5)[0]).toContain(
'See Link > a > ... > Link > a.'
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(5)[0])).toContain(
ReactDOMFeatureFlags.useFiber ?
'\n in a (at **)' +
'\n in Link (at **)' +
'\n in div (at **)' +
'\n in a (at **)' +
'\n in Link (at **)' :
'See Link > a > ... > Link > a.'
);
});

Expand Down
Loading