-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Changes from all commits
afbb492
efce5dc
add04ef
123a2e2
e1d13d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||
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', () => { | ||
|
@@ -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', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just describing what it really tested for. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 **)', | ||
); | ||
}); | ||
|
||
|
@@ -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 **)', | ||
); | ||
}); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
|
@@ -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 **)', | ||
); | ||
}); | ||
}); | ||
|
@@ -724,4 +734,16 @@ describe('ReactDOMServer', () => { | |
'HTML tags in React.', | ||
); | ||
}); | ||
|
||
it('should warn about contentEditable and children', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 **)', | ||
); | ||
}); | ||
}); |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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 asnull
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 separatewarningWithStack
or something.There was a problem hiding this comment.
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 😄