-
Notifications
You must be signed in to change notification settings - Fork 27.9k
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
Improved stacktraces (minor) #4156
Conversation
client/index.js
Outdated
@@ -172,7 +188,9 @@ async function doRender ({ Component, props, hash, err, emitter: emitterProp = e | |||
|
|||
// We need to clear any existing runtime error messages | |||
ReactDOM.unmountComponentAtNode(errorContainer) | |||
renderReactElement(createElement(App, appProps), appContainer) | |||
renderReactElement(<AppContainer errorReporter={ErrorDebugComponent} warnings={false}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we move this to here from the app.js
.? I like this approach. But just need to know why?
And is it possible to use AppContainer
it only in development?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<AppContainer>
becomes an empty component in production, so there's no harm in including it. I've moved it here since our componentDidCatch
in app.js
has to be executed. Also this makes sure we keep the state of app.js (since we expose it to users now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we have to do the webpack magic since in the webpack config we remove react-hot-loader completely (which is currently breaking the tests)
client/source-map-support.js
Outdated
@@ -0,0 +1,55 @@ | |||
// @flow | |||
import fetch from 'unfetch' | |||
import {SourceMapConsumer} from 'source-map' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is a kind a big library. Could you check whether we are getting rid of this in the production mode.
If not, shall we do it with some webpack magic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parent file (source-map-support) is only loaded in development using the same if/else require 👍 nonetheless i'm going to move it into the function for both client/server side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timneutkens okay. Great.
lib/error-debug.js
Outdated
} | ||
</div> | ||
) | ||
await rewriteErrorTrace(error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We won't catch errors here. Better to call .then()
and .catch
here.
lib/error-debug.js
Outdated
} | ||
} | ||
|
||
// On the server side the error comes from a single place, so `ErrorDebug` is rendered directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add more details on here?. The comment is not clear to me.
@timneutkens shall we write some test cases checking the lines of the error. Could be bit hard to write, but I think it's not impossible. |
Yeah we can definitely add tests for this, just wanted you to have a look first at the implementation 👍 |
client/source-map-support.js
Outdated
@@ -33,12 +30,14 @@ async function rewriteTraceLine (trace: string): Promise<string> { | |||
|
|||
const mapPath = `${filePath}.map` | |||
|
|||
const fetch = require('unfetch') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any effect loading modules like this? I hope webpack still see them just like before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The weird thing is, it does work for source-map, but not for unfetch 🤷♂️either way this module is only loaded in dev mode now, same way as ErrorDebug
, so it doesn't matter 👍
I tested this and seeing more consistent build times and a drop from 8-10s to 2s consistent across my application, awesome stuff! 🎉 |
client/index.js
Outdated
sourcemappedErr = await rewriteErrorTrace(err) | ||
} | ||
|
||
const str = stripAnsi(`${err.message}\n${err.stack}${err.info ? `\n\n${err.info.componentStack}` : ''}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be the sourcemappedErr
? But as discussed in slack rewriteErrorTrace
actually mutates the err
so it's the same, but for readability reasons.
</AppContainer> | ||
) | ||
} | ||
return <>{children}</> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove the empty fragment here and just return children
? or is there a clever reason as to why wrap it in a fragment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just didn't want to change behavior from what it was before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice! More readable than before 🕵️♀️
client/index.js
Outdated
if (process.env.NODE_ENV !== 'production') { | ||
sourcemappedErr = await rewriteErrorTrace(err) | ||
await applySourcemaps(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 This is great, so much easier to understand
The change to Old:
New:
From @ptomasroos testing 🙏 |
@timneutkens this is really well done. |
Could we do a canary release with this or are we waiting for something else? Would save us minutes each day 🎉 |
You can just alter this in your own webpack config @albinekb |
* Handle production errors correctly * Improved source map support * Make react-hot-loader hold state again * Remove console.log * Load modules on demand * Catch errors in rewriteErrorTrace * Update comment * Update comment * Remove source-map-support * Load modules in next-dev * Make sure error logged has sourcemaps too * Add tests for production runtime errors * Add tests for development runtime errors. Fix issue with client side errors in development * Move functionality back to renderError now that error handling is consistent * Rename to applySourcemaps
…de errors (#4764) ## What's wrong This problem is specific to errors that happen on the client _after_ the initial mounting of the component. (The router has special logic to handle exceptions thrown in `getInitialProps` during a client-side navigation, and I've confirmed this logic is correct.) Specifically, if the page is mounted, and you raise an exception on the page, the exception will cause the error page to be mounted without ever invoking `getInitialProps` on the new App/Error page pairing. This has been illustrated with multiple repros in #4574. ## Why is it broken This regression was introduced two months ago in #4156, where the invocation of `getInitialProps` was removed from the app's top-level error handler. Specifically, [this line](https://github.com/zeit/next.js/pull/4156/files#diff-895656aeaccff5d7c0f56a113ede9662L147) was removed and [replaced by a comment](https://github.com/zeit/next.js/pull/4156/files#diff-895656aeaccff5d7c0f56a113ede9662R167) that says that "`App` will handle the calling of `getInitialProps`". I believe the sentiment about "`App` will handle calling `getInitialProps`" is mistaken. In fact, it really doesn't make sense on its face, since it would require an instance lifecycle method of `App` (which is mounted immediately after the comment) to invoke the `static getInitialProps` method on the error page. ## How I fixed it I've fixed this in a fork by restoring Lines 146 – 148 that were removed in #4156. I think this is the right fix, but Next.js's handling of `getInitialProps` could certainly be improved. (The code in [this conditional](https://github.com/zeit/next.js/blob/86d01706a67cee5c80796974d04c1e11cdff453a/client/index.js#L173) speaks to the unnecessary complexity around this.)
Fixes #3735 with the change to
devtool
.This change will introduce proper source map support for all modules. It'll also improve recompile time by using cheap-module-source-map instead of source-map. In practice this only improves compile times and has no real downside when debugging.
When an error is thrown we call
applySourcemaps
which will load the sourcemap associated with each line stacktrace line, then it'll find the real line using thesource-map
module and return the newly constructed error line. This is very similar to how Vue handles server side errors.Besides that I also noticed our
componentDidCatch
only worked in production mode, since react-hot-loader's appContainer implementedcomponentDidCatch
. Now we catch the error inApp
and throw it up toAppContainer
in development.