Track source more generically in ReactComponentTreeDevtool (facebook#…

…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.
zpao · May 14, 2016 · 03f4ba2 · 03f4ba2
1 parent c0007d5
commit 03f4ba2
Show file tree

Hide file tree

Showing 7 changed files with 65 additions and 29 deletions.
diff --git a/src/isomorphic/ReactDebugTool.js b/src/isomorphic/ReactDebugTool.js
@@ -244,10 +244,18 @@ var ReactDebugTool = {
     checkDebugID(debugID);
     emitEvent('onMountRootComponent', debugID);
   },
+  onBeforeMountComponent(debugID, element) {
+    checkDebugID(debugID);
+    emitEvent('onBeforeMountComponent', debugID, element);
+  },
   onMountComponent(debugID) {
     checkDebugID(debugID);
     emitEvent('onMountComponent', debugID);
   },
+  onBeforeUpdateComponent(debugID, element) {
+    checkDebugID(debugID);
+    emitEvent('onBeforeUpdateComponent', debugID, element);
+  },
   onUpdateComponent(debugID) {
     checkDebugID(debugID);
     emitEvent('onUpdateComponent', debugID);

diff --git a/src/isomorphic/devtools/ReactComponentTreeDevtool.js b/src/isomorphic/devtools/ReactComponentTreeDevtool.js
@@ -19,6 +19,7 @@ var rootIDs = [];
 function updateTree(id, update) {
   if (!tree[id]) {
     tree[id] = {
+      element: null,
       parentID: null,
       ownerID: null,
       text: null,
@@ -88,6 +89,14 @@ var ReactComponentTreeDevtool = {
     updateTree(id, item => item.text = text);
   },
 
+  onBeforeMountComponent(id, element) {
+    updateTree(id, item => item.element = element);
+  },
+
+  onBeforeUpdateComponent(id, element) {
+    updateTree(id, item => item.element = element);
+  },
+
   onMountComponent(id) {
     updateTree(id, item => item.isMounted = true);
   },
@@ -141,6 +150,13 @@ var ReactComponentTreeDevtool = {
     return item ? item.parentID : null;
   },
 
+  getSource(id) {
+    var item = tree[id];
+    var element = item ? item.element : null;
+    var source = element != null ? element._source : null;
+    return source;
+  },
+
   getText(id) {
     var item = tree[id];
     return item ? item.text : null;

diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js
@@ -29,7 +29,6 @@ var ReactDOMButton = require('ReactDOMButton');
 var ReactDOMComponentFlags = require('ReactDOMComponentFlags');
 var ReactDOMComponentTree = require('ReactDOMComponentTree');
 var ReactDOMInput = require('ReactDOMInput');
-var ReactDOMInstrumentation = require('ReactDOMInstrumentation');
 var ReactDOMOption = require('ReactDOMOption');
 var ReactDOMSelect = require('ReactDOMSelect');
 var ReactDOMTextarea = require('ReactDOMTextarea');
@@ -579,10 +578,6 @@ ReactDOMComponent.Mixin = {
         validateDOMNesting.updatedAncestorInfo(parentInfo, this._tag, this);
     }
 
-    if (__DEV__) {
-      ReactDOMInstrumentation.debugTool.onMountDOMComponent(this._debugID, this._currentElement);
-    }
-
     var mountImage;
     if (transaction.useCreateElement) {
       var ownerDocument = hostContainerInfo._ownerDocument;
@@ -858,10 +853,6 @@ ReactDOMComponent.Mixin = {
       context
     );
 
-    if (__DEV__) {
-      ReactDOMInstrumentation.debugTool.onUpdateDOMComponent(this._debugID, this._currentElement);
-    }
-
     if (this._tag === 'select') {
       // <select> value update needs to occur after <option> children
       // reconciliation

diff --git a/src/renderers/dom/shared/ReactDOMDebugTool.js b/src/renderers/dom/shared/ReactDOMDebugTool.js
@@ -12,6 +12,7 @@
 'use strict';
 
 var ReactDOMUnknownPropertyDevtool = require('ReactDOMUnknownPropertyDevtool');
+var ReactDebugTool = require('ReactDebugTool');
 
 var warning = require('warning');
 
@@ -40,9 +41,11 @@ function emitEvent(handlerFunctionName, arg1, arg2, arg3, arg4, arg5) {
 
 var ReactDOMDebugTool = {
   addDevtool(devtool) {
+    ReactDebugTool.addDevtool(devtool);
     eventHandlers.push(devtool);
   },
   removeDevtool(devtool) {
+    ReactDebugTool.removeDevtool(devtool);
     for (var i = 0; i < eventHandlers.length; i++) {
       if (eventHandlers[i] === devtool) {
         eventHandlers.splice(i, 1);
@@ -62,12 +65,6 @@ var ReactDOMDebugTool = {
   onTestEvent() {
     emitEvent('onTestEvent');
   },
-  onMountDOMComponent(debugID, element) {
-    emitEvent('onMountDOMComponent', debugID, element);
-  },
-  onUpdateDOMComponent(debugID, element) {
-    emitEvent('onMountDOMComponent', debugID, element);
-  },
 };
 
 ReactDOMDebugTool.addDevtool(ReactDOMUnknownPropertyDevtool);

diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
@@ -1315,8 +1315,17 @@ describe('ReactDOMComponent', function() {
       ReactDOMServer.renderToString(<div class="paladin"/>);
       ReactDOMServer.renderToString(<input type="text" onclick="1"/>);
       expect(console.error.argsForCall.length).toBe(2);
-      expect(console.error.argsForCall[0][0]).toMatch(/.*className.*\(.*:\d+\)/);
-      expect(console.error.argsForCall[1][0]).toMatch(/.*onClick.*\(.*:\d+\)/);
+      expect(
+        console.error.argsForCall[0][0].replace(/\(.+?:\d+\)/g, '(**:*)')
+      ).toBe(
+        'Warning: Unknown DOM property class. Did you mean className? (**:*)'
+      );
+      expect(
+        console.error.argsForCall[1][0].replace(/\(.+?:\d+\)/g, '(**:*)')
+      ).toBe(
+        'Warning: Unknown event handler property onclick. Did you mean ' +
+        '`onClick`? (**:*)'
+      );
     });
 
     it('gives source code refs for unknown prop warning for update render', function() {
@@ -1328,7 +1337,11 @@ describe('ReactDOMComponent', function() {
 
       ReactDOMServer.renderToString(<div class="paladin" />, container);
       expect(console.error.argsForCall.length).toBe(1);
-      expect(console.error.argsForCall[0][0]).toMatch(/.*className.*\(.*:\d+\)/);
+      expect(
+        console.error.argsForCall[0][0].replace(/\(.+?:\d+\)/g, '(**:*)')
+      ).toBe(
+        'Warning: Unknown DOM property class. Did you mean className? (**:*)'
+      );
     });
 
     it('gives source code refs for unknown prop warning for exact elements ', function() {

diff --git a/src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js b/src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js
@@ -13,11 +13,12 @@
 
 var DOMProperty = require('DOMProperty');
 var EventPluginRegistry = require('EventPluginRegistry');
+var ReactComponentTreeDevtool = require('ReactComponentTreeDevtool');
 
 var warning = require('warning');
 
 if (__DEV__) {
-  var cachedSource;
+  var lastDebugID;
   var reactProps = {
     children: true,
     dangerouslySetInnerHTML: true,
@@ -47,14 +48,18 @@ if (__DEV__) {
         null
     );
 
+    var source = ReactComponentTreeDevtool.getSource(lastDebugID);
+    var formattedSource = source ?
+      `(${source.fileName.replace(/^.*[\\\/]/, '')}:${source.lineNumber})` : '';
+
     // For now, only warn when we have a suggested correction. This prevents
     // logging too much when using transferPropsTo.
     warning(
       standardName == null,
       'Unknown DOM property %s. Did you mean %s? %s',
       name,
       standardName,
-      formatSource(cachedSource)
+      formattedSource
     );
 
     var registrationName = (
@@ -70,14 +75,10 @@ if (__DEV__) {
       'Unknown event handler property %s. Did you mean `%s`? %s',
       name,
       registrationName,
-      formatSource(cachedSource)
+      formattedSource
     );
   };
 
-  var formatSource = function(source) {
-    return source ? `(${source.fileName.replace(/^.*[\\\/]/, '')}:${source.lineNumber})` : '';
-  };
-
 }
 
 var ReactDOMUnknownPropertyDevtool = {
@@ -90,11 +91,13 @@ var ReactDOMUnknownPropertyDevtool = {
   onDeleteValueForProperty(node, name) {
     warnUnknownProperty(name);
   },
-  onMountDOMComponent(debugID, element) {
-    cachedSource = element ? element._source : null;
+  onBeforeMountComponent(debugID, element) {
+    // TODO: This currently assumes that properties are all set before recursing
+    // and mounting children, which needn't be the case in the future.
+    lastDebugID = debugID;
   },
-  onUpdateDOMComponent(debugID, element) {
-    cachedSource = element ? element._source : null;
+  onBeforeUpdateComponent(debugID, element) {
+    lastDebugID = debugID;
   },
 };
 

diff --git a/src/renderers/shared/stack/reconciler/ReactReconciler.js b/src/renderers/shared/stack/reconciler/ReactReconciler.js
@@ -50,6 +50,10 @@ var ReactReconciler = {
           internalInstance._debugID,
           'mountComponent'
         );
+        ReactInstrumentation.debugTool.onBeforeMountComponent(
+          internalInstance._debugID,
+          internalInstance._currentElement
+        );
       }
     }
     var markup = internalInstance.mountComponent(
@@ -150,6 +154,10 @@ var ReactReconciler = {
           internalInstance._debugID,
           'receiveComponent'
         );
+        ReactInstrumentation.debugTool.onBeforeUpdateComponent(
+          internalInstance._debugID,
+          internalInstance._currentElement
+        );
       }
     }