Remove react-hot-loader #4500
Remove react-hot-loader #4500
Conversation
client/dev-error-overlay.js
Outdated
| // Since componentDidMount doesn't handle errors we use then/catch here | ||
| applySourcemaps(error).then(() => { | ||
| this.setState({mappedError: error}) | ||
| }).catch(console.error) |
There was a problem hiding this comment.
Maybe here we can set this.state.mappedError to the error from applySourcemaps, or atleast set the error to something like "Couldn't render error. Check console for additional debugging details".
client/error-boundary.js
Outdated
|
|
||
| export type RuntimeError = { | ||
| ...Error, | ||
| module: ?{ |
There was a problem hiding this comment.
I highly recommend using {| ... |} over { ... } when possible. I'm not sure if possible here?
client/error-boundary.js
Outdated
| export type Info = null | ComponentDidCatchInfo | ||
|
|
||
| export type RuntimeError = { | ||
| ...Error, |
There was a problem hiding this comment.
export type RuntimeError = Error & {module: ?{| rawRequest: string |}}
client/index.js
Outdated
| renderReactElement(<App {...appProps} />, appContainer) | ||
| let onError = null | ||
|
|
||
| if (process.env.NODE_ENV === 'production') { |
There was a problem hiding this comment.
how about process.env.NODE_ENV !== 'development'
There was a problem hiding this comment.
Seems safer to me to fail over to "pretty-printing" an error
| @@ -7,6 +7,7 @@ import { loadGetInitialProps, getURL } from '../lib/utils' | |||
| import PageLoader from '../lib/page-loader' | |||
There was a problem hiding this comment.
possible to // @flow this or should we do that in a later PR (i.e. PR just to flowtype more things)
There was a problem hiding this comment.
Yeah, I didn't on purpose, carefully chose which files to start with 👍
lib/app.js
Outdated
| } | ||
| // Kept here for backwards compatibility. | ||
| componentDidCatch (err) { | ||
| throw err |
There was a problem hiding this comment.
wish we could get rid of this, but oh well. Much cleaner 😍
There was a problem hiding this comment.
I'm not sure how we're going to deprecate this correctly, we can't warn since it's triggered in normal cases where an error is thrown too. I'm guessing we should just include it in the release notes / blogpost of Next 7.
There was a problem hiding this comment.
Doesn't componentDidCatch implicitly just throw err upwards anyway? I haven't used Error Boundaries, but from reading the release I understood that this works similar to context and it'll just bubble up.
There was a problem hiding this comment.
What's the purpose of the explicit componentDidCatch?
There was a problem hiding this comment.
It's backward compatibility, with the current implementation we told people to do this:
componentDidCatch(err, info) {
console.log('YOUR OWN LOGGING')
super.componentDidCatch(err, info)
}
And since React.Component doesn't implement the method we can't let it fall back on that.
There was a problem hiding this comment.
This applies when using a custom pages/_app.js that overrides this file and extends the App class.
There was a problem hiding this comment.
oh I see! They should also be able to:
There was a problem hiding this comment.
componentDidCatch(err, info) {
console.log('YOUR OWN LOGGING')
throw err
}right?
There was a problem hiding this comment.
With the new solution, it can be easily done "the React way" by just throwing the error up the bubble, eg:
componentDidCatch(err, info) {
console.log('YOUR OWN LOGGING')
throw err
}
Fixes #4494