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

Use React-provided stacktrace in crash overlay #1746

Closed
Timer opened this issue Mar 7, 2017 · 12 comments
Closed

Use React-provided stacktrace in crash overlay #1746

Timer opened this issue Mar 7, 2017 · 12 comments
Milestone

Comments

@Timer
Copy link
Contributor

Timer commented Mar 7, 2017

The recently merged crash overlay should support whitelisted warnings thrown by React.

Currently, this can be accomplished by enabling the console proxy via:

proxyConsole('error');

However, this relies on a synthetically generated stack-trace and not the stack-trace provided by React in the warning. e.g.

    var error = void 0;
    try {
      throw new Error(message);
    } catch (e) {
      error = e;
    }

This is problematic, however, because of how React works.

In React, components aren't rendered recursively, so there's never more than one component's render() on the stack
- @gaearon

This results in an unreconstructed tree of components specifying where the problem is stemming from.


The stack-trace from React is actually already parsed, and used to replace the line references on the synthetically generated stack-trace.

The proper behavior, however, would be to not generate a synthetic stack-trace and rely solely on the stack-trace given by React.

We realize this may be a daunting issue, but if you are up to the challenge, we will answer your every question!

@Timer
Copy link
Contributor Author

Timer commented Mar 7, 2017

/cc @brandonmikeska who expressed interest in helping with the overlay

@brandonmikeska
Copy link

Hey @Timer I am all for helping, but would need guidance on how to properly tackle this.

@Timer
Copy link
Contributor Author

Timer commented Mar 7, 2017

Sure!

To start working on this feature, first uncomment https://github.com/facebookincubator/create-react-app/blob/ed5c016d6169f0bac7e29625afba1bf7bccac1d9/packages/react-dev-utils/crashOverlay.js#L823.

This will enable the console.error proxy, defined here: https://github.com/facebookincubator/create-react-app/blob/ed5c016d6169f0bac7e29625afba1bf7bccac1d9/packages/react-dev-utils/crashOverlay.js#L786-L821.

However, our area of focus is to remove reliance on https://github.com/facebookincubator/create-react-app/blob/ed5c016d6169f0bac7e29625afba1bf7bccac1d9/packages/react-dev-utils/crashOverlay.js#L810-L815.
We want to create an error with the stack purely from the already extracted stack: https://github.com/facebookincubator/create-react-app/blob/ed5c016d6169f0bac7e29625afba1bf7bccac1d9/packages/react-dev-utils/crashOverlay.js#L794-L809.

However, due to the way this works the source resolving works, you're going to need to find a way to match these already-mapped files back to the bundled files (because of https://github.com/facebookincubator/create-react-app/blob/ed5c016d6169f0bac7e29625afba1bf7bccac1d9/packages/react-dev-utils/crashOverlay.js#L685-L691).

Now, you don't necessarily need to do the reverse of the sourcemap ... but perhaps propose a different solution. That is what this issue is about -- figuring out how to do this best.

Let me know if this requires changes to stack-frame-resolver, and we can make it happen!

@gaearon
Copy link
Contributor

gaearon commented Mar 7, 2017

FWIW, the way I imagined it is we have a middleware on the dev server running at /debug-symbolicate or something like this. The error box can call it with the component stack trace from React warning. The middleware would look up the source lines around the locations sent by error box, and send the code in response. The error box would nicely format the code for each component stack frame and display it.

@brandonmikeska
Copy link

Hmm ok! I have not implemented a middleware before, but I wouldn't mind learning if someone has time for a screen share or documentation links that I can read through to get something started.

@Timer
Copy link
Contributor Author

Timer commented Mar 13, 2017

@brandonmikeska we should probably start with #1322, would you like to tackle that issue? 😄
It'll make mapping files to system files easier.

@Timer
Copy link
Contributor Author

Timer commented Mar 14, 2017

I've updated stack-frame to support this use-case.
This should be relatively trivial now. 😄

@Timer
Copy link
Contributor Author

Timer commented Mar 14, 2017

this is a crude POC

@brandonmikeska
Copy link

@Timer I can look at #1322 but it seems like you already have a proposed solution. Do you still want me to start there?

@Timer
Copy link
Contributor Author

Timer commented Mar 14, 2017

Yes, please! #1322 needs addressing -- I've explored how to implement the solution but not which path base is best for the solution itself.
If you can figure out the best base path (whether it be appBase or appSrc or appBuild, etc) and send a PR that'd be great. Feel more than free to copy my little snippet.

We'll need something for production, too.

@Timer
Copy link
Contributor Author

Timer commented May 14, 2017

This has been completed across multiple PRs. 😄

@Timer Timer closed this as completed May 14, 2017
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants