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

Show component stack in PropTypes warnings #6771

Merged
merged 1 commit into from May 15, 2016

Conversation

@sophiebits
Copy link
Collaborator

@sophiebits sophiebits commented May 14, 2016

image

This works pretty well, even if you don't have source info compiled in – it falls back to annotating with owner info. The weirdest thing I noticed is that if A renders <section><B /></section> and B renders <div><C /></div> and C gives a warning, then the stack will be

in C (created by B)
in B (created by A)
in section (created by A)
in A

and makes no mention of the div that should live between B and C: it can't, because that element hasn't been created yet. I think this probably won't be too confusing in practice but I wonder if there's something we can do to make it clearer.

cc @facebook/react-core

@sophiebits
Copy link
Collaborator Author

@sophiebits sophiebits commented May 14, 2016

I had to track the stack because we don't know the parent hierarchy until the children are fully mounted. I wonder if we can change that.

Loading

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented May 14, 2016

@spicyj updated the pull request.

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented May 14, 2016

Can we add onSetParent and pass the parent debugID down to the children as they are being mounted? It would never change so not too much of a hassle.

I think we should try harder to avoid relying on the stack again because this can become hard to rip out later.

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented May 14, 2016

I think we don’t even need to pass it to the children as long as we set it right away after instantiating them but before recursing.

Loading

@sophiebits
Copy link
Collaborator Author

@sophiebits sophiebits commented May 14, 2016

Yes, that works out nicely. How do you like this?

Loading

@ghost
Copy link

@ghost ghost commented May 14, 2016

@spicyj updated the pull request.

Loading

@gaearon
Copy link
Member

@gaearon gaearon commented May 14, 2016

👍

Loading

nextChild.parentID = id;
}
nextChild.parentID = id;
// TODO: invariant(nextChild.parentID === id) makes sense but doesn't
Copy link
Member

@gaearon gaearon May 14, 2016

Choose a reason for hiding this comment

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

Can we add an invariant for the cases where we their parentId is not missing? It being missing is just a subset of cases, is it not? I mean something like invariant(nextChild.parentID == null || nextChild.parentID === id)

Loading

Copy link
Collaborator Author

@sophiebits sophiebits May 15, 2016

Choose a reason for hiding this comment

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

Yup, done.

Loading

@sophiebits sophiebits merged commit 378c879 into facebook:master May 15, 2016
1 check was pending
Loading
@ghost
Copy link

@ghost ghost commented May 15, 2016

@spicyj updated the pull request.

Loading

@zpao zpao added this to the 15.y.0 milestone May 16, 2016
@zpao zpao removed this from the 15.y.0 milestone Jun 1, 2016
@zpao zpao added this to the 15-next milestone Jun 1, 2016
@zpao zpao added this to the 15-next milestone Jun 1, 2016
@zpao zpao removed this from the 15.y.0 milestone Jun 1, 2016
zpao added a commit to zpao/react that referenced this issue Jun 8, 2016
zpao added a commit that referenced this issue Jun 14, 2016
@zpao zpao removed this from the 15-next milestone Jun 14, 2016
@zpao zpao added this to the 15.2.0 milestone Jun 14, 2016
@zpao zpao added this to the 15.2.0 milestone Jun 14, 2016
@zpao zpao removed this from the 15-next milestone Jun 14, 2016
troydemonbreun referenced this issue Jul 5, 2016
(cherry picked from commit 74c29b3 and  bc1d59e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants