-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Added error boundaries to CodeEditor and layout component #96
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/** | ||
* Copyright (c) 2013-present, Facebook, Inc. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
* @emails react-core | ||
*/ | ||
|
||
'use strict'; | ||
|
||
import React, {Component} from 'react'; | ||
|
||
function ErrorBoundary(WrappedComponent) { | ||
return class extends Component{ | ||
constructor(props, context) { | ||
super(props, context); | ||
|
||
this.state = { | ||
error: null, | ||
}; | ||
} | ||
|
||
componentDidCatch(error) { | ||
this.setState({ error }); | ||
} | ||
|
||
render() { | ||
console.log(this.state); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this^ 😄 |
||
if (this.state.error) { | ||
alert('Error, try to whitelist the site in AdBlocker/Cookie Whitelist'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is the right way to warn people about this. I think a
We should also keep in mind that not all errors are caused by this so our wording shouldn't be overly assertive that this is definitely the cause/fix. Users might see an error even if they aren't using adblockers and haven't disabled cookies, and that would be confusing. |
||
return; | ||
} | ||
return <WrappedComponent {...this.props} />; | ||
} | ||
} | ||
} | ||
|
||
export default ErrorBoundary; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
/** | ||
* Copyright (c) 2013-present, Facebook, Inc. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
* @emails react-core | ||
*/ | ||
|
||
'use strict'; | ||
|
||
import ErrorBoundary from './ErrorBoundary'; | ||
|
||
export default ErrorBoundary; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ import '../prism-styles'; | |
import 'glamor/reset'; | ||
import 'css/reset.css'; | ||
import 'css/algolia.css'; | ||
import ErrorBoundary from '../components/ErrorBoundary'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit import ErrorBoundary from 'components/ErrorBoundary'; |
||
|
||
class Template extends Component { | ||
componentDidMount() { | ||
|
@@ -82,4 +83,4 @@ class Template extends Component { | |
} | ||
} | ||
|
||
export default Template; | ||
export default ErrorBoundary(Template); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Errors in the template will actually result in a totally empty/white page. This is probably not great. We should show something as a fallback in the browser (in addition to logging error info to the console). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you suggest as the fallback? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably need to show a basic error page. It doesn't have to look super pretty. @joecritch or I can pretty it up. |
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.
nit