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

Include component stack in more places, including SSR #11284

Merged
merged 5 commits into from
Oct 19, 2017
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
8 changes: 4 additions & 4 deletions packages/react-dom/src/ReactDOMFiberComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ var HTML = '__html';

var {Namespaces: {html: HTML_NAMESPACE}, getIntrinsicNamespace} = DOMNamespaces;

var getStack = emptyFunction;
var getStack = emptyFunction.thatReturnsArgument('');

if (__DEV__) {
getStack = getCurrentFiberStackAddendum;
Expand Down Expand Up @@ -562,7 +562,7 @@ var ReactDOMFiberComponent = {
props = rawProps;
}

assertValidProps(tag, props, getCurrentFiberOwnerName);
assertValidProps(tag, props, getStack);

setInitialDOMProperties(
tag,
Expand Down Expand Up @@ -656,7 +656,7 @@ var ReactDOMFiberComponent = {
break;
}

assertValidProps(tag, nextProps, getCurrentFiberOwnerName);
assertValidProps(tag, nextProps, getStack);

var propKey;
var styleName;
Expand Down Expand Up @@ -971,7 +971,7 @@ var ReactDOMFiberComponent = {
break;
}

assertValidProps(tag, rawProps, getCurrentFiberOwnerName);
assertValidProps(tag, rawProps, getStack);

if (__DEV__) {
var extraAttributeNames: Set<string> = new Set();
Expand Down
20 changes: 8 additions & 12 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ var omittedCloseTags = require('omittedCloseTags');
var isCustomComponent = require('isCustomComponent');

var toArray = React.Children.toArray;
var emptyFunctionThatReturnsNull = emptyFunction.thatReturnsNull;
var getStackAddendum = emptyFunction.thatReturnsArgument('');

if (__DEV__) {
var warning = require('fbjs/lib/warning');
Expand Down Expand Up @@ -80,9 +80,9 @@ if (__DEV__) {
currentDebugStack = null;
ReactDebugCurrentFrame.getCurrentStack = null;
};
var getStackAddendum = function(): null | string {
getStackAddendum = function(): null | string {
if (currentDebugStack === null) {
return null;
return '';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious: Why the change from null => ''?

(Is it to maintain the previous behavior where we had an inline function just returning an empty string?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because we use getStack() directly as %s, which would show up as null right in the message when missing. I agree it's not great though and would be nice to fix the whole thing by adding a separate warningWithStack or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Gotcha.

I asked b'c the Flow return type is also null | string but it's no big deal 😄

}
let stack = '';
let debugStack = currentDebugStack;
Expand Down Expand Up @@ -141,11 +141,7 @@ function createMarkupForStyles(styles) {
var styleValue = styles[styleName];
if (__DEV__) {
if (!isCustomProperty) {
warnValidStyle(
styleName,
styleValue,
() => '', // getCurrentFiberStackAddendum,
);
warnValidStyle(styleName, styleValue, getStackAddendum);
}
}
if (styleValue != null) {
Expand Down Expand Up @@ -574,7 +570,7 @@ class ReactDOMServerRenderer {
ReactControlledValuePropTypes.checkPropTypes(
'input',
props,
() => '', //getCurrentFiberStackAddendum
getStackAddendum,
);

if (
Expand Down Expand Up @@ -632,7 +628,7 @@ class ReactDOMServerRenderer {
ReactControlledValuePropTypes.checkPropTypes(
'textarea',
props,
() => '', //getCurrentFiberStackAddendum
getStackAddendum,
);
if (
props.value !== undefined &&
Expand Down Expand Up @@ -693,7 +689,7 @@ class ReactDOMServerRenderer {
ReactControlledValuePropTypes.checkPropTypes(
'select',
props,
() => '', // getCurrentFiberStackAddendum,
getStackAddendum,
);

for (var i = 0; i < valuePropNames.length; i++) {
Expand Down Expand Up @@ -785,7 +781,7 @@ class ReactDOMServerRenderer {
validatePropertiesInDevelopment(tag, props);
}

assertValidProps(tag, props, emptyFunctionThatReturnsNull);
assertValidProps(tag, props, getStackAddendum);

var out = createOpenTagMarkup(
element.type,
Expand Down
84 changes: 44 additions & 40 deletions packages/react-dom/src/shared/__tests__/ReactDOMComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -954,50 +954,38 @@ describe('ReactDOMComponent', () => {
});

it('should warn against children for void elements', () => {
var container = document.createElement('div');

expect(function() {
const container = document.createElement('div');
let caughtErr;
try {
ReactDOM.render(<input>children</input>, container);
}).toThrowError(
} catch (err) {
caughtErr = err;
}
expect(caughtErr).not.toBe(undefined);
expect(normalizeCodeLocInfo(caughtErr.message)).toContain(
'input is a void element tag and must neither have `children` nor ' +
'use `dangerouslySetInnerHTML`.',
'\n in input (at **)',
);
});

it('should warn against dangerouslySetInnerHTML for void elements', () => {
var container = document.createElement('div');

expect(function() {
const container = document.createElement('div');
let caughtErr;
try {
ReactDOM.render(
<input dangerouslySetInnerHTML={{__html: 'content'}} />,
container,
);
}).toThrowError(
'input is a void element tag and must neither have `children` nor use ' +
'`dangerouslySetInnerHTML`.',
);
});

it('should include owner rather than parent in warnings', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed this test since it's not really useful in component stacks (where we have both).
Would be nice to mark owner in some special way in stacks but that's a separate issue.

var container = document.createElement('div');

function Parent(props) {
return props.children;
}
function Owner() {
// We're using the input dangerouslySetInnerHTML invariant but the
// exact error doesn't matter as long as we have a way to verify
// that warnings and invariants contain owner rather than parent name.
return (
<Parent>
<input dangerouslySetInnerHTML={{__html: 'content'}} />
</Parent>
);
} catch (err) {
caughtErr = err;
}

expect(function() {
ReactDOM.render(<Owner />, container);
}).toThrowError('\n\nThis DOM node was rendered by `Owner`.');
expect(caughtErr).not.toBe(undefined);
expect(normalizeCodeLocInfo(caughtErr.message)).toContain(
'input is a void element tag and must neither have `children` nor ' +
'use `dangerouslySetInnerHTML`.',
'\n in input (at **)',
);
});

it('should emit a warning once for a named custom component using shady DOM', () => {
Expand Down Expand Up @@ -1178,19 +1166,27 @@ describe('ReactDOMComponent', () => {
expect(tracker.getValue()).toEqual('foo');
});

it('should warn for children on void elements', () => {
it('should throw for children on void elements', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just describing what it really tested for.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

class X extends React.Component {
render() {
return <input>moo</input>;
}
}

var container = document.createElement('div');
expect(function() {
const container = document.createElement('div');
let caughtErr;
try {
ReactDOM.render(<X />, container);
}).toThrowError(
} catch (err) {
caughtErr = err;
}

expect(caughtErr).not.toBe(undefined);
expect(normalizeCodeLocInfo(caughtErr.message)).toContain(
'input is a void element tag and must neither have `children` ' +
'nor use `dangerouslySetInnerHTML`.\n\nThis DOM node was rendered by `X`.',
'nor use `dangerouslySetInnerHTML`.' +
'\n in input (at **)' +
'\n in X (at **)',
);
});

Expand Down Expand Up @@ -1303,12 +1299,20 @@ describe('ReactDOMComponent', () => {
}
}

expect(function() {
let caughtErr;
try {
ReactDOM.render(<Animal />, container);
}).toThrowError(
} catch (err) {
caughtErr = err;
}

expect(caughtErr).not.toBe(undefined);
expect(normalizeCodeLocInfo(caughtErr.message)).toContain(
'The `style` prop expects a mapping from style properties to values, ' +
"not a string. For example, style={{marginRight: spacing + 'em'}} " +
'when using JSX.\n\nThis DOM node was rendered by `Animal`.',
'when using JSX.' +
'\n in div (at **)' +
'\n in Animal (at **)',
);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ var PropTypes;

var ROOT_ATTRIBUTE_NAME;

function normalizeCodeLocInfo(str) {
return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
}

describe('ReactDOMServer', () => {
beforeEach(() => {
jest.resetModules();
Expand Down Expand Up @@ -380,11 +384,17 @@ describe('ReactDOMServer', () => {
});

it('should throw prop mapping error for an <iframe /> with invalid props', () => {
expect(() =>
ReactDOMServer.renderToString(<iframe style="border:none;" />),
).toThrowError(
let caughtErr;
try {
ReactDOMServer.renderToString(<iframe style="border:none;" />);
} catch (err) {
caughtErr = err;
}
expect(caughtErr).not.toBe(undefined);
expect(normalizeCodeLocInfo(caughtErr.message)).toContain(
'The `style` prop expects a mapping from style properties to values, not ' +
"a string. For example, style={{marginRight: spacing + 'em'}} when using JSX.",
"a string. For example, style={{marginRight: spacing + 'em'}} when using JSX." +
'\n in iframe (at **)',
);
});
});
Expand Down Expand Up @@ -724,4 +734,16 @@ describe('ReactDOMServer', () => {
'HTML tags in React.',
);
});

it('should warn about contentEditable and 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.

Not new behavior, but we didn't have an SSR test for it.

spyOn(console, 'error');
ReactDOMServer.renderToString(<div contentEditable={true} children="" />);
expectDev(console.error.calls.count()).toBe(1);
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: A component is `contentEditable` and contains `children` ' +
'managed by React. It is now your responsibility to guarantee that ' +
'none of those nodes are unexpectedly modified or duplicated. This ' +
'is probably not intentional.\n in div (at **)',
);
});
});
24 changes: 4 additions & 20 deletions packages/react-dom/src/shared/utils/assertValidProps.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,28 +13,12 @@ var invariant = require('fbjs/lib/invariant');
var voidElementTags = require('voidElementTags');

if (__DEV__) {
var {getCurrentFiberStackAddendum} = require('ReactDebugCurrentFiber');
var warning = require('fbjs/lib/warning');
}

var HTML = '__html';

function getDeclarationErrorAddendum(getCurrentOwnerName) {
if (__DEV__) {
var ownerName = getCurrentOwnerName();
if (ownerName) {
// TODO: also report the stack.
return '\n\nThis DOM node was rendered by `' + ownerName + '`.';
}
}
return '';
}

function assertValidProps(
tag: string,
props: ?Object,
getCurrentOwnerName: () => ?string,
) {
function assertValidProps(tag: string, props: ?Object, getStack: () => string) {
if (!props) {
return;
}
Expand All @@ -45,7 +29,7 @@ function assertValidProps(
'%s is a void element tag and must neither have `children` nor ' +
'use `dangerouslySetInnerHTML`.%s',
tag,
getDeclarationErrorAddendum(getCurrentOwnerName),
getStack(),
);
}
if (props.dangerouslySetInnerHTML != null) {
Expand All @@ -70,15 +54,15 @@ function assertValidProps(
'React. It is now your responsibility to guarantee that none of ' +
'those nodes are unexpectedly modified or duplicated. This is ' +
'probably not intentional.%s',
getCurrentFiberStackAddendum() || '',
getStack(),
);
}
invariant(
props.style == null || typeof props.style === 'object',
'The `style` prop expects a mapping from style properties to values, ' +
"not a string. For example, style={{marginRight: spacing + 'em'}} when " +
'using JSX.%s',
getDeclarationErrorAddendum(getCurrentOwnerName),
getStack(),
);
}

Expand Down
14 changes: 2 additions & 12 deletions scripts/rollup/bundles.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,7 @@ const bundles = [
label: 'dom-server-browser',
manglePropertiesOnProd: false,
name: 'react-dom/server.browser',
paths: [
'packages/react-dom/**/*.js',
// TODO: server shouldn't depend on reconciler modules:
'packages/react-reconciler/**/*.js',
'packages/shared/**/*.js',
],
paths: ['packages/react-dom/**/*.js', 'packages/shared/**/*.js'],
},

{
Expand All @@ -194,12 +189,7 @@ const bundles = [
label: 'dom-server-server-node',
manglePropertiesOnProd: false,
name: 'react-dom/server.node',
paths: [
'packages/react-dom/**/*.js',
// TODO: server shouldn't depend on reconciler modules:
'packages/react-reconciler/**/*.js',
'packages/shared/**/*.js',
],
paths: ['packages/react-dom/**/*.js', 'packages/shared/**/*.js'],
},

/******* React ART *******/
Expand Down