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

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 19, 2017

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.

);
});

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.

@@ -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.

@@ -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.

Copy link
Contributor

@bvaughn bvaughn left a 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 '';
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 😄

@@ -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
Contributor

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

😆 "gosh"

Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

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

LGTM :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants