Use devtool for unknown property warning #5590
Conversation
jimfb
force-pushed
the
use-devtool-for-unknown-property-warning
branch
from
d1a38f8
to
9791b0d
Compare
| @@ -35,6 +37,8 @@ var React = { | |||
| render: render, | |||
| unmountComponentAtNode: ReactMount.unmountComponentAtNode, | |||
| version: ReactVersion, | |||
| addDevtool: ReactDOMDevtools.addDevtool, | |||
| removeDevtool: ReactDOMDevtools.removeDevtool, | |||
There was a problem hiding this comment.
These should be on the __REACT_DEVTOOLS_GLOBAL_HOOK__ somehow because there is no reliable way to get to a single instance of a renderer from a global debugger protocol.
There was a problem hiding this comment.
Just removed them, as per conversation, we can figure out how to expose them later.
jimfb
force-pushed
the
use-devtool-for-unknown-property-warning
branch
2 times, most recently
from
2f73188
to
0ca4b55
Compare
jimfb
force-pushed
the
use-devtool-for-unknown-property-warning
branch
from
0ca4b55
to
30ef056
Compare
|
cc @sebmarkbage done. |
| @@ -140,6 +87,7 @@ var DOMPropertyOperations = { | |||
| * @return {?string} Markup string, or null if the property was invalid. | |||
| */ | |||
| createMarkupForProperty: function(name, value) { | |||
| ReactDOMDevtools.emitEvent('createMarkupForProperty', {name, value}); | |||
jimfb
force-pushed
the
use-devtool-for-unknown-property-warning
branch
from
45eecac
to
26f3785
Compare
|
@jimfb updated the pull request. |
…warning Use devtool for unknown property warning
|
|
||
| var ReactDOMDebugTool = require('ReactDOMDebugTool'); | ||
|
|
||
| module.exports = {debugTool: ReactDOMDebugTool}; |
There was a problem hiding this comment.
What is the difference between “debug tool” and a “dev tool”? I got tripped by this a couple of times because I write a Devtool but I’m then adding it to a DebugTool which I refer to as ReactDOMInstrumentation.debugTool. Why is there more than one layer of indirection?
There was a problem hiding this comment.
There is only ever one debug tool. A debug tool doesn't (necessarily) have the overhead of emitting events, so it is suitable for collecting ultra-accurate performance metrics.
The default debug tool is an event emitter, into which you can plug dev tools.
A dev tool is something that is not performance critical. Things like emitting warnings, high-level performance metrics, interactive chrome extensions, etc. You want to maintain good performance, but you're willing to eat the cost of event delegation for the purposes of a slightly better API (ie. the ability to have multiple devtools at the same time, functions you don't implement are ignored, errors thrown in a devtool don't take down the app, etc).
There was a problem hiding this comment.
Thank you for explaining. I’m curious because I wonder how something like Chrome extension would work once we port it to use this API. Currently Chrome extension works with production builds. Would that no longer be the case (presumably because production builds would use a noop debug tool)?
There was a problem hiding this comment.
Correct, my guess is that devtools would only work in dev mode, but it's an easy thing to change if we change our minds.
There was a problem hiding this comment.
To add more context, I am adding some events that will be useful both to React DevTools and new ReactPerf in #6068. If those are DEV-only, the new ReactPerf would also be DEV-only which would defeat the purpose of ReactPerf. How would debug tool / devtool distinction handle this problem?
There was a problem hiding this comment.
I think Sebastian was thinking about creating a third build called "profile", which follows all the "production" codepaths with the exception of having debugtool enabled.
There was a problem hiding this comment.
Do these different builds have any overlap with the feature flags such as https://github.com/facebook/react/blob/3b96650e39ddda5ba49245713ef16dbc52d25e9e/src/shared/utils/ReactFeatureFlags.js?
There was a problem hiding this comment.
Yes, eventually. Right now it's controlled by __DEV__, and is not as simple as just replacing all __DEV__s with a feature flag for reasons around dead code elimination.
There was a problem hiding this comment.
Thanks, @jimfb. The “eventually” part is all I need to know :)
This builds on #5462, and implements a simple devtool that emits the unknown property warning (two commits in PR because the commits should be kept separate). This PR should have no effect on user-visible behavior.
The next PR (which builds on top of this one) will add line numbers to the warning, which was not previously possible when the warning lived in DOMPropertyOperations but is now possible because the warning lives in a devtool which can follow along with the lifecycle of the render :). But let's not get ahead of ourselves - that's a behavior change - we should CR+merge this diff before we get too excited about the massively improved future :).
cc @sebmarkbage @spicyj