Add component stack to invariant #11652

Ethan-Arrowood · 2017-11-24T20:28:34Z

Initial PR shows changes in ReactFiber.js. Notice I needed to update some unit tests to expect the component stacks. Currently I had to hard-code the component stack into the expected response. Is there a way to implement this using globs or some other method? If anyone ever makes other changes to these test files those hard-coded lines will break.

gaearon · 2017-11-24T23:01:58Z

Regarding CI failure, might be a bug in the new bundle testing system: #11653. Can you cherry pick my change there to verify if it helps?

Ethan-Arrowood · 2017-11-25T01:47:45Z

Okay cherry picked and pushed. Let's see what happens to the build ( 😢 😄 )

gaearon · 2017-11-25T01:58:52Z

Oh, I see what's going on. Our build script clears the changes to the error code file.

gaearon · 2017-11-25T02:14:42Z

#11655

Ethan-Arrowood · 2017-11-25T02:17:37Z

Nice work!

Ethan-Arrowood · 2017-11-27T01:08:36Z

@gaearon can you review what I've done so far and give me some feedback?

gaearon · 2017-12-07T22:34:57Z

Could you please bring this up to date?

Ethan-Arrowood · 2017-12-07T23:32:56Z

Will do

Ethan-Arrowood · 2017-12-08T00:09:02Z

Uh oh I don't think I did that correctly.

@gaearon is there something I can do to fix this? Should I just open a new PR on an update branch. I'm sorry I messed this up 🤦‍♀️

gaearon · 2017-12-08T00:25:46Z

scripts/circleci/build.sh

@@ -3,6 +3,7 @@
 set -e		

 yarn build --extract-errors
+


Merge conflict mistake. Will change

gaearon · 2017-12-08T00:26:29Z

packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js

@@ -751,7 +751,9 @@ describe('ReactIncrementalErrorHandling', () => {
          (__DEV__
            ? " You likely forgot to export your component from the file it's " +
              'defined in, or you might have mixed up default and named imports.' +
-              '\n\nCheck the render method of `BrokenRender`.'
+              '\n\nCheck the render method of `BrokenRender`.' +


Can we remove this line now? Seems unnecessary with the stack.

Yes. I need to make the following two lines globs instead of hardcoded values too.

gaearon · 2017-12-08T00:45:58Z

I usually just write down the hashes of commits I want to keep and then do

git fetch origin # assuming origin points to our react repo, might be 'upstream' for you
git reset --hard origin/master
git cherry-pick <my first commit hash>
git cherry-pick <my second commit hash>
...
git push -f

Ethan-Arrowood · 2017-12-08T00:49:50Z

@gaearon thank you for this! I'm honestly going to print it and pin it to my workspace so I can use it in the future.

gaearon · 2017-12-08T01:12:40Z

Just make sure you have all the commits you need written down before you reset.

gaearon · 2017-12-12T14:41:33Z

Ping :-)

Ethan-Arrowood · 2017-12-12T14:57:32Z

@gaearon apologies on the delay I just finished finals yesterday and I have to move out of the dorms today. I'll get to work on this tomorrow and hopefully have something substantial by friday

gaearon · 2017-12-12T15:12:08Z

Ah sorry, I didn’t know. Take your time please!

Ethan-Arrowood · 2017-12-15T04:17:01Z

Okay I've done a bunch of clean up. And now I'm going to try and implement the callstack on invariants!

Ethan-Arrowood · 2017-12-15T04:41:56Z

@gaearon do you have any ideas on how I can get the unit tests to expect the stack trace? If there is a way to use string wildcards that might suffice; otherwise, I'm a little lost. Will continue to try stuff this evening.

~~I was able to import the getCurrentStackAddendum into the test file but it is returning null when I use it within the test~~ 😔 Still returns 'is not a function'

gaearon · 2018-01-05T17:29:04Z

Can you rebase? You can see how our new custom toWarnDev() matcher implements the stack aliasing and do something similar, e.g. by overriding the built-in toError() matcher.

gaearon · 2018-01-05T17:29:21Z

Or maybe you could add toErrorWithStack() or something like this.

Ethan-Arrowood · 2018-01-05T18:21:04Z

I'll give it a shot thank you!

Ethan-Arrowood · 2018-01-06T03:26:59Z

I don't know why this closed itself. . . I was just rebaseing. I'll try to make some changes and reopen.

Ethan-Arrowood · 2018-01-06T04:57:54Z

After a lot of head scratching tonight I'm not exactly sure where to go with this. The tests that are failing after I add the stack to the invariant are .toEqual( ). These toEqual methods contain the span array returned from ReactNoop.getChildren(). I cannot get the getCurrentFiberStackAddendum() to run in the test file (for whatever reason it says it is 'not a function' but I'm importing it just like any other module).

Ethan-Arrowood · 2018-01-06T05:03:10Z

But I think I understand your recommendation on making a custom matcher.

Getting the component stack in the test file doesn't matter if I can essentially parse the .getChildren() result and replace all the at (File/Location/Path/thing.js:234) with at **.

Working on a custom matcher now. Never wrote one before so lets see how this goes!

Ethan-Arrowood · 2018-01-06T05:35:14Z

Commit 8825ba2 adds a custom matcher toErrorWithStack; however, I am having some trouble integrating it with jest. -- working on it now.

Ethan-Arrowood · 2018-01-06T05:46:12Z

🎉 I got the custom matcher to work. (I was using an arrow function before which didn't pass the jest this through).

Now I gotta go add the component stack to all invariants and then update failing tests!

gaearon · 2018-01-06T13:14:58Z

packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js

@@ -792,14 +794,16 @@ describe('ReactIncrementalErrorHandling', () => {
    expect(ReactNoop.flush).toWarnDev(
      'Warning: React.createElement: type is invalid -- expected a string',
    );
-    expect(ReactNoop.getChildren()).toEqual([
+    expect(ReactNoop.getChildren()).toErrorWithStack([


This is not an error. It’s comparing the actual children (because the error was “rendered” into the DOM by an error boundary).

Just change this to toContain and completely drop the part with the stack. It doesn’t matter because we have other tests verifying the stack works.

gaearon · 2018-01-06T13:15:46Z

packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js

@@ -745,14 +745,16 @@ describe('ReactIncrementalErrorHandling', () => {
    expect(ReactNoop.flush).toWarnDev(
      'Warning: React.createElement: type is invalid -- expected a string',
    );
-    expect(ReactNoop.getChildren()).toEqual([
+    expect(ReactNoop.getChildren()).toErrorWithStack([


Same as below. It was not throwing an error so it shouldn’t throw one now either.

Ethan-Arrowood · 2018-01-08T18:07:32Z

Okay so I made the changes. I'll be adding the component stack to all invariants soon

gaearon · 2018-01-09T11:01:31Z

packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js

@@ -745,7 +745,7 @@ describe('ReactIncrementalErrorHandling', () => {
    expect(ReactNoop.flush).toWarnDev(
      'Warning: React.createElement: type is invalid -- expected a string',
    );
-    expect(ReactNoop.getChildren()).toEqual([
+    expect(ReactNoop.getChildren()).toContent([


I don't think we should add a custom matcher just for this one use case.

I'm sorry I wasn't clear before—when I said about custom matchers I meant about changing the code that currently uses toThrow(), not this use case.

Here, you can just use toContain and remove the part with the stack trace (it's not essential to this test)

toContain is checking if something is within an array with ===. The span() method returns an object which would fail the strict equals. Could I keep it as .toEqual() and strip out the stack trace? I'll get to work on a code sample for this right now.

You can do expect(ReactNoop.getChildren()[0].children[0]).toContain(

In these instances it seems as though children is an empty array. I'm getting: Expected array but received undefined when I use that line in the expect() statement.

Well, you can inspect the structure and figure out which one to compare 😉

gaearon · 2018-01-10T15:28:21Z

packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js

+  }
+
+  function stripStackTrace(obj) {
+    obj[0].prop = removeStack(obj[0].prop);


Can we return a copy here please instead of mutating?

gaearon · 2018-01-11T00:50:57Z

packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js

-    obj[0].prop = removeStack(obj[0].prop);
-    return obj;
+    let stripped = [...obj];
+    stripped[0].prop = removeStack(stripped[0].prop);


This still mutates the object.

Okay I guess I don't fully understand mutation. Let me know how the most recent changes look. I'm also going to start trying to fix the build errors.

gaearon · 2018-01-11T12:38:52Z

packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js

+    return [{
+      ...compSpan[0],
+      prop: removeStack(compSpan[0].prop),
+    }];


If there is (incorrectly) more than one child, this will ignore it. Let's make sure we don't "lose" any input children?

Gotcha. Let me see what I can come up with.

facebook-github-bot added the CLA Signed label Nov 24, 2017

gaearon mentioned this pull request Nov 24, 2017

Try to fix CI for missing error code #11653

Closed

gaearon mentioned this pull request Nov 25, 2017

Don't reset error codes on CI build #11655

Merged

gaearon reviewed Dec 8, 2017

View reviewed changes

Ethan-Arrowood force-pushed the add-callstack-to-invariant branch 2 times, most recently from fe77ee9 to 7ae3037 Compare December 15, 2017 04:12

Ethan-Arrowood closed this Jan 6, 2018

Ethan-Arrowood force-pushed the add-callstack-to-invariant branch from 5bdd984 to 48833f6 Compare January 6, 2018 03:24

Attempt add getCurrentFiberStackAddendum

9813b63

Ethan-Arrowood reopened this Jan 6, 2018

add toErrorWithStack

8825ba2

Fixed custom matcher

c4b1655

gaearon reviewed Jan 6, 2018

View reviewed changes

change toContent

3911764

prettify and lint

d117d9c

gaearon reviewed Jan 9, 2018

View reviewed changes

Ethan-Arrowood added 3 commits January 9, 2018 11:17

add stripStackTrace

ffbae2f

remove toContent. add childFiber

257ef7e

Add stack to child, input, textarea

f87e8c9

gaearon reviewed Jan 10, 2018

View reviewed changes

remove mutation

8ae3596

gaearon reviewed Jan 11, 2018

View reviewed changes

Ethan-Arrowood added 2 commits January 11, 2018 00:21

actually remove mutation maybe

b325320

Fix test-prod issues

204d97c

gaearon reviewed Jan 11, 2018

View reviewed changes

keep noop children

26e9a9e

Ethan-Arrowood closed this Jan 11, 2019

Add component stack to invariant #11652

Add component stack to invariant #11652

Conversation

Ethan-Arrowood commented Nov 24, 2017

gaearon commented Nov 24, 2017

Ethan-Arrowood commented Nov 25, 2017 • edited Loading

gaearon commented Nov 25, 2017

gaearon commented Nov 25, 2017

Ethan-Arrowood commented Nov 25, 2017

Ethan-Arrowood commented Nov 27, 2017

gaearon commented Dec 7, 2017

Ethan-Arrowood commented Dec 7, 2017

Ethan-Arrowood commented Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Dec 8, 2017

Ethan-Arrowood commented Dec 8, 2017

gaearon commented Dec 8, 2017

gaearon commented Dec 12, 2017

Ethan-Arrowood commented Dec 12, 2017

gaearon commented Dec 12, 2017

Ethan-Arrowood commented Dec 15, 2017

Ethan-Arrowood commented Dec 15, 2017 • edited Loading

gaearon commented Jan 5, 2018

gaearon commented Jan 5, 2018

Ethan-Arrowood commented Jan 5, 2018

Ethan-Arrowood commented Jan 6, 2018

Ethan-Arrowood commented Jan 6, 2018

Ethan-Arrowood commented Jan 6, 2018 • edited Loading

Ethan-Arrowood commented Jan 6, 2018

Ethan-Arrowood commented Jan 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ethan-Arrowood commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ethan-Arrowood commented Nov 25, 2017 •

edited

Loading

Ethan-Arrowood commented Dec 8, 2017 •

edited

Loading

Ethan-Arrowood commented Dec 15, 2017 •

edited

Loading

Ethan-Arrowood commented Jan 6, 2018 •

edited

Loading