Add component stack info to key validation warnings #6799

keyz · 2016-05-18T22:45:20Z

This PR implements #6790. ~~I rewrote some of the tests from JSX to createElement calls since #6398 added source locations at JSX compile-time and hard-coding line numbers in tests doesn't sound good.~~

sophiebits · 2016-05-18T23:20:22Z

src/isomorphic/classic/element/ReactElementValidator.js

    addenda.parentOrOwner || '',
    addenda.childOwner || '',
-    addenda.url || ''
+    addenda.url || '',


We shouldn't need these other addenda now since the stack should cover it. Can you remove the code for them? We still will want to dedupe the warnings based on owner though as we're currently doing.

facebook-github-bot · 2016-05-19T21:34:26Z

@keyanzhang updated the pull request.

keyz · 2016-05-19T21:45:03Z

src/renderers/shared/stack/reconciler/ReactChildReconciler.js

+    if (__DEV__) {
+      traverseAllChildren(
+        nestedChildNodes,
+        (childInsts, child, name) => instantiateChild(


I tried to put selfDebugID as the first arg of instantiateChild and to use instantiateChild.bind(null, selfDebugID) here, but that means we need to call bind as well in the production build. Do you think the current approach (arrowed currying, if that's a term) is acceptable? @spicyj

Yeah, this seems fine to me. (I think calling it parentDebugID might be a little clearer, but this is also oaky.)

Hmm, I thought about this; self is more clear to me since we are talking about the current component which happens to be the parent for the children here.

ghost · 2016-05-19T21:59:18Z

@keyanzhang updated the pull request.

ghost · 2016-05-19T23:02:07Z

@keyanzhang updated the pull request.

keyz · 2016-05-19T23:05:26Z

src/addons/transitions/ReactTransitionChildMapping.js

    if (!children) {
      return children;
    }
+    // TODO: `flattenChildren` now takes an extra `selfDebugID` argument


@spicyj what's a good way to get and feed debugID to flattenChildren here? The only place that calls ReactTransitionChildMapping.getChildMapping (syntactic sugar for flattenChildren) is here: https://github.com/facebook/react/blob/master/src/addons/transitions/ReactTransitionGroup.js#L41. In other words, how do I/can I get a debugID from a component?

Also, seems like the only other place we use flattenChildren is in ReactMultiChild and there's already a test for that in ReactMultiChild-test. Interestingly, when ReactChildReconciler finds duplicated keys, the error message talks about "flattenChildren" (which is also test covered in the new ReactChildReconciler-test file).

var internalInstance = ReactInstanceMap.get(this); internalInstance._debugID

should work. Maybe someday we'll have a public API to get this that anyone could use but for now I think it's okay for us to get the internal instance for this here.

…-component-stack

keyz · 2016-05-20T01:21:06Z

This PR should be ready for review. I refactored ReactElementValidator (inlined a function) but didn't change the warning message since the current message format is more explicit than the component stack info. Please feel free to let me know if you want me to change it. Thanks @spicyj!

ghost · 2016-05-20T01:33:00Z

@keyanzhang updated the pull request.

ghost · 2016-05-20T01:46:39Z

@keyanzhang updated the pull request.

sophiebits · 2016-05-20T06:10:17Z

src/renderers/shared/devtools/ReactComponentTreeDevtool.js

    var currentOwner = ReactCurrentOwner.current;
    var id = currentOwner && currentOwner._debugID;
+
+    info += ReactComponentTreeDevtool.getStackAddendumByID(id);


sophiebits · 2016-05-20T06:14:30Z

Did you change anything in ReactElementValidator other than adding the stack? It's hard to tell because the code moved around.

sophiebits · 2016-05-20T06:15:03Z

(Also, pretty sure I meant to do needs-revision on this last time! Just a few more questions and I think we'll be good.)

ghost · 2016-05-20T20:58:33Z

@keyanzhang updated the pull request.

keyz · 2016-05-20T21:00:23Z

src/addons/transitions/ReactTransitionGroup.js


  getInitialState: function() {
    return {
+      // TODO: can we get useful debug information to show at this point?


Interestingly, ReactInstanceMap doesn't have information for the component at this point. That's probably because the component hasn't been mounted.

sophiebits · 2016-05-20T21:14:35Z

looks good to me, feel free to "squash and merge"

tnrich · 2016-05-28T00:08:17Z

Yay I am so stoked for this to be merged in! Great work react team!

* Add component stack info to key validation warnings * Add `ReactComponentTreeDevtool.getStackAddendumByID` (cherry picked from commit 47e49ae)

(cherry picked from commit 74c29b3 and bc1d59e)

Added component stack info to key validation warnings

9d4e2ff

ghost added GH Review: review-needed CLA Signed labels May 18, 2016

sophiebits reviewed May 18, 2016
View reviewed changes

sophiebits added GH Review: accepted and removed GH Review: review-needed labels May 18, 2016

sophiebits mentioned this pull request May 19, 2016

Add warning for unknown properties. #6800

Merged

Keyan Zhang added 2 commits May 19, 2016 14:22

Switch tests back to JSX and normalize code location info in tests

53d8ca3

add ReactComponentTreeDevtool.getStackAddendumByID

3f112af

Let ReactChildReconciler use debugID

af4a32a

keyz reviewed May 19, 2016
View reviewed changes

keyz changed the title ~~Added component stack info to key validation warnings~~ Add component stack info to key validation warnings May 19, 2016

Let flattenChildren use debugID

14dbe15

keyz reviewed May 19, 2016
View reviewed changes

keyz changed the title ~~Add component stack info to key validation warnings~~ [WIP] Add component stack info to key validation warnings May 19, 2016

Keyan Zhang added 2 commits May 19, 2016 18:11

Refactored ReactElementValidator

570fe38

Merge remote-tracking branch 'upstream/master' into key-warnings-with…

5e1c16d

…-component-stack

keyz changed the title ~~[WIP] Add component stack info to key validation warnings~~ Add component stack info to key validation warnings May 20, 2016

added a jsdoc annotation

7274283

sophiebits reviewed May 20, 2016
View reviewed changes

sophiebits added GH Review: needs-revision and removed GH Review: accepted labels May 20, 2016

Feed debugID to ReactTransitionChildMapping.getChildMapping

1a0f6ca

ghost added GH Review: review-needed and removed GH Review: needs-revision labels May 20, 2016

keyz reviewed May 20, 2016
View reviewed changes

sophiebits added GH Review: accepted and removed GH Review: review-needed labels May 20, 2016

keyz merged commit 47e49ae into facebook:master May 20, 2016

keyz mentioned this pull request May 20, 2016

Add both child and parent name to key-missing warning stack #6823

Closed

keyz deleted the key-warnings-with-component-stack branch May 20, 2016 23:26

sophiebits added the semver-patch label May 22, 2016

sophiebits added this to the 15.y.z milestone May 22, 2016

zpao pushed a commit that referenced this pull request Jun 14, 2016

Add component stack info to key validation warnings (#6799)

23d3e70

* Add component stack info to key validation warnings * Add `ReactComponentTreeDevtool.getStackAddendumByID` (cherry picked from commit 47e49ae)

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

gaearon mentioned this pull request Jun 26, 2016

Add more detail to missing-key warning #6163

Closed

troydemonbreun referenced this pull request Jul 5, 2016

Changelog for 15.2.0

23cfe03

(cherry picked from commit 74c29b3 and bc1d59e)

renovate bot mentioned this pull request Feb 2, 2018

Update dependency react to v0.14.9 - autoclosed signavio/backbone-rel#29

Closed

1 task

Add component stack info to key validation warnings #6799

Add component stack info to key validation warnings #6799

Uh oh!

Conversation

keyz commented May 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented May 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented May 19, 2016

Uh oh!

ghost commented May 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyz May 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyz commented May 20, 2016

Uh oh!

ghost commented May 20, 2016

Uh oh!

ghost commented May 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sophiebits commented May 20, 2016

Uh oh!

sophiebits commented May 20, 2016

Uh oh!

ghost commented May 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sophiebits commented May 20, 2016

Uh oh!

tnrich commented May 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

keyz commented May 18, 2016 •

edited

Loading

keyz May 19, 2016 •

edited

Loading