-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
Track source more generically in ReactComponentTreeDevtool #6765
Conversation
@@ -40,9 +41,11 @@ function emitEvent(handlerFunctionName, arg1, arg2, arg3, arg4, arg5) { | |||
|
|||
var ReactDOMDebugTool = { | |||
addDevtool(devtool) { | |||
ReactDebugTool.addDevtool(devtool); |
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.
Are we going to expose just one of them in the end?
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 figured we would probably expose both – though maybe it makes more sense to only expose each platform-specific one. Open to other suggestions on how to set ReactDOMUnknownPropertyDevtool up so that we can listen to both kinds of events though.
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.
maybe it makes more sense to only expose each platform-specific one
Let’s go with this assumption for now. In this case it makes sense to automatically setup forwarding.
Being able to get the source for your parent components seems useful, and ReactComponentTreeDevtool is best poised to be able to do that. I'm also not sure it makes sense to have separate DOM-specific `onMountDOMComponent` and `onUpdateDOMComponent` events, so I removed them for now. Even if we want them, their timing seemed sort of arbitrary. I also made it so DOM devtools can listen to non-DOM events too. Willing to change that if people think it's ugly though.
9418c95
to
346a161
Compare
@spicyj updated the pull request. |
Now tracking element, and removed extra debugging code (moved to #6768). |
LGTM |
@troydemonbreun No worries, your work was still helpful! Thanks for doing most of the work. |
@spicyj You are very generous. |
…6765) Being able to get the source for your parent components seems useful, and ReactComponentTreeDevtool is best poised to be able to do that. I'm also not sure it makes sense to have separate DOM-specific `onMountDOMComponent` and `onUpdateDOMComponent` events, so I removed them for now. Even if we want them, their timing seemed sort of arbitrary. I also made it so DOM devtools can listen to non-DOM events too. Willing to change that if people think it's ugly though. (cherry picked from commit 03f4ba2)
Being able to get the source for your parent components seems useful, and ReactComponentTreeDevtool is best poised to be able to do that. I'm also not sure it makes sense to have separate DOM-specific `onMountDOMComponent` and `onUpdateDOMComponent` events, so I removed them for now. Even if we want them, their timing seemed sort of arbitrary. I also made it so DOM devtools can listen to non-DOM events too. Willing to change that if people think it's ugly though. (cherry picked from commit 03f4ba2)
Being able to get the source for your parent components seems useful, and ReactComponentTreeDevtool is best poised to be able to do that.
I'm also not sure it makes sense to have separate DOM-specific
onMountDOMComponent
andonUpdateDOMComponent
events, so I removed them for now. Even if we want them, their timing seemed sort of arbitrary.I also made it so DOM devtools can listen to non-DOM events too. Willing to change that if people think it's ugly though.
Reviewers: @gaearon