Fix for #6062 : Show source line number on unknown property warning #6398

troydemonbreun · 2016-04-01T22:27:41Z

Fix for #6062

Let me know if I'm off base by using this approach.

(in trying to rebase with a merge conflict, I created commit noise, sorry)

Thanks!

jimfb · 2016-04-01T23:33:37Z

src/renderers/dom/shared/ReactDOMComponent.js

@@ -651,6 +652,9 @@ ReactDOMComponent.Mixin = {
            markup = DOMPropertyOperations.createMarkupForCustomAttribute(propKey, propValue);
          }
        } else {
+          if (__DEV__) {
+            ReactDOMInstrumentation.debugTool.onCreateMarkupForProperty(propKey, propValue, this._currentElement);


This line is the main one that concerns me. It seems like this call should be inside createMarkupForProperty such that it gets invoked whenever anyone calls it will get the onCreateMarkupForProperty event. However, we should not be passing the element to createMarkupForProperty.

Thus, why this PR is difficult, thus my comments in #6062 (comment) and #6062 (comment)

Ah, I didn't realize that operation was called directly. I've another
approach in mind as well, I'll try it out next week.
On Apr 1, 2016 6:35 PM, "Jim" notifications@github.com wrote:

In src/renderers/dom/shared/ReactDOMComponent.js
#6398 (comment):

@@ -651,6 +652,9 @@ ReactDOMComponent.Mixin = {
markup = DOMPropertyOperations.createMarkupForCustomAttribute(propKey, propValue);
}
} else {

if (**DEV**) {

ReactDOMInstrumentation.debugTool.onCreateMarkupForProperty(propKey, propValue, this._currentElement);

This line is the main one that concerns me. It seems like this call should
be inside createMarkupForProperty such that it gets invoked whenever
anyone calls it will get the onCreateMarkupForProperty event. However, we
should not be passing the element to createMarkupForProperty.

Thus, why this PR is difficult, thus my comments in #6062 (comment)
#6062 (comment)
and #6062 (comment)
#6062 (comment)

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/facebook/react/pull/6398/files/295b048adf2a1de62d2572c179da584bccfc7cb1#r58281313

facebook-github-bot · 2016-04-04T20:51:43Z

@troydemonbreun updated the pull request.

facebook-github-bot · 2016-04-05T17:24:22Z

@troydemonbreun updated the pull request.

facebook-github-bot · 2016-04-05T17:54:29Z

@troydemonbreun updated the pull request.

troydemonbreun · 2016-04-05T17:58:29Z

@jimfb What about this approach? Thanks for your time.

jimfb · 2016-04-05T18:28:09Z

@troydemonbreun Yes, this is the approach that I had in mind. However, you're going to want to save the source when a component is rendering (ie. as oppose to onInstantiateReactComponent), because the warning could fire during an update (which won't have a corresponding instantiation). Also, you'll want to give the source location of the jsx element being rendered, rather than the jsx element that lead to the component that has the problem. Maybe something like onMountComponent and onUpdateComponent.

We will need unit tests for all the various cases. Three that come to mind are:

A custom/composite component renders 10 div tags, one of which has an unknown property - the source should specify which of the 10 divs is the problem.
Unknown property during initial render.
No unknown properties during initial render, but yes unknown properties for an update render.

facebook-github-bot · 2016-04-07T18:44:12Z

@troydemonbreun updated the pull request.

troydemonbreun · 2016-04-07T18:46:52Z

@jimfb Thanks for the direction! I noticed that ReactDebugTool has those events, is it okay to add those events also to ReactDOMDebugTool, or would you prefer a better way to be DRY about it?

jimfb · 2016-04-08T16:37:26Z

@troydemonbreun Probably ReactDOM should register a ReactDebugTool that forwards all the ReactDebugTool events to the ReactDOMDebugTool.

jimfb · 2016-04-13T22:49:07Z

Ping @troydemonbreun

troydemonbreun · 2016-04-13T23:29:38Z

Sorry about the delay, my Dev VM is having problems, looks like I will
need to rebuild. Should get back to it tomorrow.
On Apr 13, 2016 5:51 PM, "Jim" notifications@github.com wrote:

facebook-github-bot · 2016-04-14T18:51:41Z

@troydemonbreun updated the pull request.

troydemonbreun · 2016-04-15T15:26:27Z

@jimfb Trying to grok your last piece of advice. To keep from bothering you, I've spent some time browsing the code trying to infer from current debug tool examples, etc.

Do you mean to create another debug tool and .addDevtool(ReactDebugToolEventForwarderDevTool) in ReactDebugTool? Granted, that doesn't seem to meet the criteria of ReactDOM doing the registering.

I've also thought of .addDevtool(ReactDOMUnknownPropertyDevtool) in ReactDebugTool and extending it to handle the ReactDebugTool events.

Thanks for your time.

facebook-github-bot · 2016-04-15T15:45:40Z

@troydemonbreun updated the pull request.

jimfb · 2016-04-15T16:09:12Z

Do you mean to create another debug tool and .addDevtool(ReactDebugToolEventForwarderDevTool) in ReactDebugTool?

Yes, I was thinking of a ReactDebugToolEventForwarderDevTool, but no, probably not registered IN ReactDebugTool (see below).

Granted, that doesn't seem to meet the criteria of ReactDOM doing the registering.

I think registration would occur in ReactDefaultInjection (which is ReactDOM specific) or in ReactDOM.js (which would invoke ReactInstrumentation.debugTool.addDevtool();). That way, ReactDOM is doing the registering. I don't pretend to understand the injection stuff, so maybe I'm wrong. cc @sebmarkbage may have better advice there. Also, if you post something and it looks suboptimal, someone will probably be able to jump in with feedback, but obviously it would be better if those people (@sebmarkbage, @spicyj, @syranide, @gaearon) can provide feedback now instead after the code is written.

facebook-github-bot · 2016-04-15T20:49:02Z

@troydemonbreun updated the pull request.

troydemonbreun · 2016-04-15T20:50:18Z

@jimfb onMountComponent appears to be triggered after the unknown property warning is thrown. Is there another method/event in the workflow that might get the specific instance being warned, or did I just code this wrong?

I have pushed my latest work in progress to this PR to show my progress.

Thanks!

jimfb · 2016-04-15T21:03:10Z

src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js

    );
  };
+
+  var formatSource = function(source) {
+    return source ? `(${source.fileName.replace(/^.*[\\\/]/, '')}:${source.lineNumber})` : '';


We generally prefer string concatenation instead of template literals.

I don't think we have a policy on this; either is fine in my book.

jimfb · 2016-04-15T21:09:32Z

The places that those elements get generated is within MountComponent and UpdateComponent, but they're a few calls deeper (I think). My comment about those functions was with regards to your use of instantiateComponent, which was not correct (because of things like updates) Sorry, I wasn't looking at the code and my answer was a little too high-level.

The places that we currently warn are: onCreateMarkupForProperty, onSetValueForProperty, and onDeleteValueForProperty. Each of those properties comes from an element (that element gets created on Mount or Update). We need to know which element those properties came from.

Does that make sense? Let me know if not / if you have questions that I can clarify.

troydemonbreun · 2016-04-18T14:40:01Z

I think that makes sense. I'll dissect internalInstance to get more familiar with that structure.

facebook-github-bot · 2016-04-29T20:24:03Z

@troydemonbreun updated the pull request.

troydemonbreun · 2016-04-29T21:52:48Z

@jimfb I've added in more tests for ReactDOMUnknownPropertyDevtool.onCreateMarkupForProperty. How do I trigger the ReactDOMTextareaedge case, in other words, how do I write the test so I know I'm capturing that edge case? I tried some tracing on DOMPropertyOperations.setValueForProperty and rendered a textarea, but I didn't see it make it to that path for the invalid property I set. Thanks!

jimfb · 2016-05-02T23:42:29Z

@troydemonbreun To trigger that case (with the value prop in ReactDOMTextarea), I think you need to update the component:

var container = document.createElement('div');
ReactDOM.render(<textarea value="foo" />, container);
ReactDOM.render(<textarea value="bar" />, container);

ReactDOM will never fire an unknown property warning for that case, so it is probably fine to fix that as a followup PR. (or feel free to fix up this PR).

I'm going to tentatively accept, since I think this PR makes the world incrementally better.

troydemonbreun · 2016-05-03T14:53:23Z

@jimfb Great, thanks for all your help! I'll keep this PR as it is now functionally to keep things simple.

gaearon · 2016-05-03T23:25:20Z

@troydemonbreun Great job! Sorry that I haven’t been very responsive.

troydemonbreun · 2016-05-04T02:14:44Z

Thanks! You have been responsive and helpful.
On May 3, 2016 6:27 PM, "Dan Abramov" notifications@github.com wrote:

@troydemonbreun https://github.com/troydemonbreun Great job! Sorry that
I haven’t been very responsive.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6398 (comment)

…warning (facebook#6398) * New approach for 6062 fix : Show source line number on unknown property warning * WIP: ReactDebugToolEventForwarderDevTool * Update event signature to debugID * Trigger events in ReactDOMComponent * Renamed to onMountDOMComponent; passing in element directly * Added debugID; updated simple test * Added test for multi-div JSX to ref exact line * Added test for composite component (cherry picked from commit 7cf61db)

…6398) * New approach for 6062 fix : Show source line number on unknown property warning * WIP: ReactDebugToolEventForwarderDevTool * Update event signature to debugID * Trigger events in ReactDOMComponent * Renamed to onMountDOMComponent; passing in element directly * Added debugID; updated simple test * Added test for multi-div JSX to ref exact line * Added test for composite component (cherry picked from commit 7cf61db)

(cherry picked from commit 74c29b3 and bc1d59e)

facebook-github-bot added GH Review: review-needed CLA Signed labels Apr 1, 2016

jimfb reviewed Apr 1, 2016
View reviewed changes

troydemonbreun force-pushed the r-6062 branch from 392f881 to 768bf26 Compare April 5, 2016 17:46

troydemonbreun force-pushed the r-6062 branch 3 times, most recently from 728c96d to 0012131 Compare April 7, 2016 18:37

jimfb mentioned this pull request Apr 8, 2016

Strip whitelist. Move casing warnings to dev tools #6459

Closed

nhunzaker mentioned this pull request Apr 13, 2016

Warn when a DOM element has unsupported property #6465

Closed

troydemonbreun force-pushed the r-6062 branch from 0012131 to 57f12bd Compare April 14, 2016 18:44

troydemonbreun force-pushed the r-6062 branch from 57f12bd to 55d7007 Compare April 15, 2016 15:38

jimfb reviewed Apr 15, 2016
View reviewed changes

jimfb added GH Review: accepted and removed GH Review: review-needed labels May 2, 2016

jimfb merged commit 7cf61db into facebook:master May 3, 2016

This was referenced May 4, 2016

Unknown plugin babel-plugin-transform-react-jsx-source #6695

Closed

Fix babel-plugin-transform-react-jsx-source dependency #6697

Merged

troydemonbreun deleted the r-6062 branch May 4, 2016 14:43

troydemonbreun mentioned this pull request May 6, 2016

ReactDebugToolEventForwarderDevTool #6718

Closed

zpao mentioned this pull request May 16, 2016

Embed JSX filename paths relative to repo root #6778

Merged

zpao added the semver-minor label May 16, 2016

zpao added this to the 15.y.0 milestone May 16, 2016

keyz mentioned this pull request May 18, 2016

Add component stack info to key validation warnings #6799

Merged

17 tasks

troydemonbreun mentioned this pull request May 25, 2016

Track source more generically in ReactComponentTreeDevtool #6765

Merged

zpao modified the milestones: 15.y.0, 15-next Jun 1, 2016

zpao modified the milestones: 15-next, 15.2.0 Jun 14, 2016

troydemonbreun referenced this pull request Jul 5, 2016

Changelog for 15.2.0

23cfe03

(cherry picked from commit 74c29b3 and bc1d59e)

bvaughn mentioned this pull request Jan 3, 2018

Update additional tests to use .toWarnDev() matcher #11957

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #6062 : Show source line number on unknown property warning #6398

Fix for #6062 : Show source line number on unknown property warning #6398

troydemonbreun commented Apr 1, 2016

jimfb Apr 1, 2016

troydemonbreun Apr 2, 2016

facebook-github-bot commented Apr 4, 2016

facebook-github-bot commented Apr 5, 2016

facebook-github-bot commented Apr 5, 2016

troydemonbreun commented Apr 5, 2016

jimfb commented Apr 5, 2016

facebook-github-bot commented Apr 7, 2016

troydemonbreun commented Apr 7, 2016

jimfb commented Apr 8, 2016

jimfb commented Apr 13, 2016

troydemonbreun commented Apr 13, 2016 •

edited

Loading

facebook-github-bot commented Apr 14, 2016

troydemonbreun commented Apr 15, 2016 •

edited

Loading

facebook-github-bot commented Apr 15, 2016

jimfb commented Apr 15, 2016 •

edited

Loading

facebook-github-bot commented Apr 15, 2016

troydemonbreun commented Apr 15, 2016 •

edited

Loading

jimfb Apr 15, 2016

sophiebits Apr 15, 2016

jimfb commented Apr 15, 2016 •

edited

Loading

troydemonbreun commented Apr 18, 2016

facebook-github-bot commented Apr 29, 2016

troydemonbreun commented Apr 29, 2016

jimfb commented May 2, 2016

troydemonbreun commented May 3, 2016

gaearon commented May 3, 2016

troydemonbreun commented May 4, 2016

Fix for #6062 : Show source line number on unknown property warning #6398

Fix for #6062 : Show source line number on unknown property warning #6398

Conversation

troydemonbreun commented Apr 1, 2016

jimfb Apr 1, 2016

Choose a reason for hiding this comment

troydemonbreun Apr 2, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Apr 4, 2016

facebook-github-bot commented Apr 5, 2016

facebook-github-bot commented Apr 5, 2016

troydemonbreun commented Apr 5, 2016

jimfb commented Apr 5, 2016

facebook-github-bot commented Apr 7, 2016

troydemonbreun commented Apr 7, 2016

jimfb commented Apr 8, 2016

jimfb commented Apr 13, 2016

troydemonbreun commented Apr 13, 2016 • edited Loading

facebook-github-bot commented Apr 14, 2016

troydemonbreun commented Apr 15, 2016 • edited Loading

facebook-github-bot commented Apr 15, 2016

jimfb commented Apr 15, 2016 • edited Loading

facebook-github-bot commented Apr 15, 2016

troydemonbreun commented Apr 15, 2016 • edited Loading

jimfb Apr 15, 2016

Choose a reason for hiding this comment

sophiebits Apr 15, 2016

Choose a reason for hiding this comment

jimfb commented Apr 15, 2016 • edited Loading

troydemonbreun commented Apr 18, 2016

facebook-github-bot commented Apr 29, 2016

troydemonbreun commented Apr 29, 2016

jimfb commented May 2, 2016

troydemonbreun commented May 3, 2016

gaearon commented May 3, 2016

troydemonbreun commented May 4, 2016

troydemonbreun commented Apr 13, 2016 •

edited

Loading

troydemonbreun commented Apr 15, 2016 •

edited

Loading

jimfb commented Apr 15, 2016 •

edited

Loading

troydemonbreun commented Apr 15, 2016 •

edited

Loading

jimfb commented Apr 15, 2016 •

edited

Loading