-
Notifications
You must be signed in to change notification settings - Fork 48.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
Conversation
); | ||
}); | ||
|
||
it('should include owner rather than parent in warnings', () => { |
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.
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.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Just describing what it really tested for.
@@ -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 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.
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 think this looks good 😄
if (currentDebugStack === null) { | ||
return null; | ||
return ''; |
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 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.
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 😄
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
👍
props: ?Object, | ||
getStack: () => string, | ||
) { | ||
function assertValidProps(tag: string, props: ?Object, getStack: () => 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.
😆 "gosh"
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.
LGTM :)
This includes component stack instead of owner name in more places, including previously TODO'd places in the server renderer. It also gets rid of the last SSR dependency on reconciler modules so I could remove
react-reconciler/*
from the source whitelist for SSR bundles.