Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added :componentStackFrames property to error info object #10484

Closed
wants to merge 2 commits into from

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Aug 18, 2017

We pass a componentStack (string) property to the injected error dialog and the componentDidCatch lifecycle method. This string is pre-formatted string to match the error.stack string for logging convenience. However this approach requires parsing in order to do anything beyond basic logging.

This PR adds a new componentStackFrames property to both of the above methods. It is loosely based on the TC39 error stack strawman. This new property looks like this:

type Source = {
  fileName: string,
  lineNumber: number,
};

type StackFrame = {
  name: string | null,
  source: Source | null,
};
 
// callStackFrames is an Array<StackFrame>

In addition to adding basic tests for this property, I've also updated our fixtures to expose it:
screen shot 2017-08-17 at 5 17 48 pm

Resolves #10461

@sophiebits
Copy link
Collaborator

cc @ljharb since I'm sure you'll hear about this anyway

@@ -1181,6 +1186,9 @@ module.exports = function<T, P, I, TI, PI, C, CX, PL>(
// The risk is that the return path from this Fiber may not be accurate.
// That risk is acceptable given the benefit of providing users more context.
const componentStack = getStackAddendumByWorkInProgressFiber(failedWork);
const componentStackFrames = getStackFramesByWorkInProgressFiber(
Copy link
Contributor Author

@bvaughn bvaughn Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered mapping componentStackFrames through describeComponentFrame for the componentStack string (rather than re-crawling the graph) but decided it wasn't worth it given that we don't hit this code path often.

@ljharb
Copy link
Contributor

ljharb commented Aug 18, 2017

There will likely be additive changes to the StackFrame type - for example, to add offset information for Binary ASTs - but the existing format will remain intact for cross-browser compat, possibly modulo property names.

I'll give this a more thorough review next week; in general it'd be ideal if part of this implementation could one day be smoothly replaced with a System.getStackString polyfill :-)

@sebmarkbage
Copy link
Collaborator

So the reason we did this as a string is because we wanted the option to change this format and the content in it later. There is for example, a proposal for manually tagging stack frames in the browser to support general use cases like Promise-polyfills. This is more powerful because these stacks can interleave. In that case we'd get the stack from the browser, which would be a string (at least until a richer proposal is in place).

The current PR contains concepts like owner that we'd ideally get rid of or change meaning of. At the very least, I'd like to move the owner to just be collapsed into the name.

Almost all of this is DEV only. Seems like it's full of potential breakages based on relying on data structures that only exist at all in DEV.

case ClassComponent:
case HostComponent:
const name = getComponentName(fiber);
const owner = fiber._debugOwner;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These types are not sound. This will be undefined in PROD. All these accesses should be wrapped in DEV blocks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, never mind, you explicitly set them below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. We access these properties outside of DEV blocks in other places, but with null-checks. So I did the same here.

We pass a 'componentStack' (string) property to the injected error dialog and the 'componentDidCatch' lifecycle method. This string is pre-formatted string to match the error.stack string for logging convenience. However this approach requires parsing in order to do anything beyond basic logging.

This PR adds a new 'componentStackFrames' property to both of the above methods. It is loosely based on the TC39 error stack strawman.

In addition to adding basic tests for this property, I've also updated our fixtures to expose it.
@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 18, 2017

The current PR contains concepts like owner that we'd ideally get rid of or change meaning of. At the very least, I'd like to move the owner to just be collapsed into the name.

Are there any other concepts beside owner? I'm not opposed to just removing it. I kind of think it would be weird for "name" to contain name+owner.

I'm going to go ahead and remove it for now. Happy to hear what others thinl though.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Aug 18, 2017

I'm not against this change. My biggest concern is that we have a proposal from Chrome to implement extensible stacks. If we wanted to switch to that, we'd have to parse that stack trace string to turn it back into this and ship a (large?) parser that has to support old and future versions of Chrome's stack format.

I wonder if it would be better to have a smaller library that takes our existing string and parses it. For use in devtools and such. That way we can update it if the future format changes.

But then again, maybe it won't be done until v17 so we can make a breaking change then?

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 18, 2017

I wonder if it would be better to have a smaller library that takes our existing string and parses it. For use in devtools and such. That way we can update it if the future format changes.

That's an interesting idea.

But then again, maybe it won't be done until v17 so we can make a breaking change then?

That seems likely to me, but you'd have more insight into this than I would. Although it's been in stage 1 since Jan, the error stack proposal doesn't seem to be very active.

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 18, 2017

I tend to think this change is okay since it adds very little additional size/complexity and we were already creating the component stack string (which would presumably also be impacted by extensible stacks).

Any consensus from the team?

@gaearon
Copy link
Collaborator

gaearon commented Aug 18, 2017

I'd vote to not do it for now. I agree with Sebastian's concern about parsing browser-supplied stacks (which is also something I might need in CRA). I'd like to avoid React having to ship this logic.

If this truly becomes a common need, we can start with a small standalone package for parsing. If everybody ends up using it in production then yes, let's consider including it in a futur minor release.

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 18, 2017

I'd vote to not do it for now. I agree with Sebastian's concern about parsing browser-supplied stacks (which is also something I might need in CRA). I'd like to avoid React having to ship this logic.

I guess I'm still missing or misunderstanding something here. We aren't parsing anything from the browser for the component stack. We're generating it based on the graph of fibers. If we switched to use a new extensible stack proposal- (admittedly something I don't know much about)- we'd still be generating the component stack from the fiber graph, no? The component stack is fundamentally different from the call stack and, I think it's useful to remain that way.

So where does the parsing concern come in? And do we really think there's a chance that the extensible stack stuff will be actionable any time soon (aka before version 17) given the inactivity on the proposal?

Thanks for the discussion and input. Sorry if I'm being slow to catch on to what's being said. 😄

@gaearon
Copy link
Collaborator

gaearon commented Aug 18, 2017

The extensible stack thing, if I understand it right, is about mixing JS and component stacks. For example renderApp > ReactDOM.render > div > App > Button > render > renderIcon. As you can see we can have JS frames both inside and outside. This gives higher precision stacks (more useful info in one place) and would require us capturing browser stack when entering React tree (and probably in some other cases). My understanding is, if we implement this, we'll take the stack from the browser and just concatenate relevant lines. So this is where parsing becomes annoying (if we promise to expose this as a richer format we have to do the same for browser part of the stack). While browser support for this may not come anytime soon my impression is we might be able to polyfill it. I'm interested in this specifically for CRA.

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 19, 2017

Thanks for elaborating, Dan.

So this is where parsing becomes annoying (if we promise to expose this as a richer format we have to do the same for browser part of the stack).

I guess this is the part we disagree on. This newly-added property (like the pre-existing one) is called "component" stack. I think it's reasonable for it to always remain the component-stack, separate from the call-stack. I think they serve different purposes and I don't see the value of mixing call stack elements into this.

@gaearon
Copy link
Collaborator

gaearon commented Aug 19, 2017

One could also argue that it's weird to expose just a part of the stack in a rich format. Because for example CRA will want the whole stack. So then we either need to parse in CRA (back to square one) or we need to add a second property with the whole stack. But then we have the parsing problem again.

I don't see what's the downsize of releasing a tiny library that can parse these stacks. We can even link to it in the documentation. This gives us the ability to experiment with the output more freely without worrying about making breaking changes. I think by the time 17 is ready we'll have enough input and experience with this API to make such decisions.

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 19, 2017

Okay. I'll add a bullet point to Monday's sync to make a final call on this. If we have reservations against adding this into React proper, I'll create a one-off parser lib for it. 😄

@gaearon
Copy link
Collaborator

gaearon commented Aug 19, 2017

A generic library would also be useful for parsing stacks that aren't from React anyway. For example if you want to unhandled errors from reducers with a custom middleware. So clients of this might need a utility for pure JS error parsing anyway.

@gaearon gaearon mentioned this pull request Oct 4, 2017
16 tasks
@gaearon
Copy link
Collaborator

gaearon commented Jan 5, 2018

Closing as stale.

@gaearon gaearon closed this Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

componentDidCatch: document & decompose info parameter
6 participants