Conversation
client/index.js
Outdated
| // 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.
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.
<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.
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.
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.
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.
lib/error-debug.js
Outdated
| } | ||
| </div> | ||
| ) | ||
| await rewriteErrorTrace(error) |
There was a problem hiding this comment.
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.
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
|
|
||
| const mapPath = `${filePath}.map` | ||
|
|
||
| const fetch = require('unfetch') |
There was a problem hiding this comment.
Is there any effect loading modules like this? I hope webpack still see them just like before.
There was a problem hiding this comment.
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! 🎉 |
| 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.
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.
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.
Just didn't want to change behavior from what it was before.
| // In development we apply sourcemaps to the error | ||
| if (process.env.NODE_ENV !== 'production') { | ||
| sourcemappedErr = await rewriteErrorTrace(err) | ||
| await applySourcemaps(err) |
There was a problem hiding this comment.
👌 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.)
4 participants
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
applySourcemapswhich will load the sourcemap associated with each line stacktrace line, then it'll find the real line using thesource-mapmodule and return the newly constructed error line. This is very similar to how Vue handles server side errors.Besides that I also noticed our
componentDidCatchonly worked in production mode, since react-hot-loader's appContainer implementedcomponentDidCatch. Now we catch the error inAppand throw it up toAppContainerin development.