-
Notifications
You must be signed in to change notification settings - Fork 27.5k
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
Handle errors of React lifecycle methods #661
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 |
---|---|---|
@@ -1,4 +1,11 @@ | ||
import 'react-hot-loader/patch' | ||
import * as next from './next' | ||
import patch from './patch-react' | ||
|
||
// apply patch first | ||
patch((err) => { | ||
console.error(err) | ||
next.renderError(err) | ||
}) | ||
|
||
const next = require('./next') | ||
window.next = next |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
// monkeypatch React for fixing https://github.com/facebook/react/issues/2461 | ||
// based on https://gist.github.com/Aldredcz/4d63b0a9049b00f54439f8780be7f0d8 | ||
|
||
import React from 'react' | ||
|
||
let patched = false | ||
|
||
export default (handleError = () => {}) => { | ||
if (patched) { | ||
throw new Error('React is already monkeypatched') | ||
} | ||
|
||
patched = true | ||
|
||
const { createElement } = React | ||
|
||
React.createElement = function (Component, ...rest) { | ||
if (typeof Component === 'function') { | ||
const { prototype } = Component | ||
if (prototype && prototype.render) { | ||
prototype.render = wrapRender(prototype.render) | ||
} else { | ||
// stateless component | ||
Component = wrapRender(Component) | ||
} | ||
} | ||
|
||
return createElement.call(this, Component, ...rest) | ||
} | ||
|
||
const { Component: { prototype: componentPrototype } } = React | ||
const { forceUpdate } = componentPrototype | ||
|
||
componentPrototype.forceUpdate = function (...args) { | ||
if (this.render) { | ||
this.render = wrapRender(this.render) | ||
} | ||
return forceUpdate.apply(this, args) | ||
} | ||
|
||
function wrapRender (render) { | ||
if (render.__wrapped) { | ||
return render.__wrapped | ||
} | ||
|
||
const _render = function (...args) { | ||
try { | ||
return render.apply(this, args) | ||
} catch (err) { | ||
handleError(err) | ||
return null | ||
} | ||
} | ||
|
||
// copy all properties | ||
Object.assign(_render, render) | ||
|
||
render.__wrapped = _render.__wrapped = _render | ||
|
||
return _render | ||
} | ||
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. will this break compat layers ? cc @developit @trueadm 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 see anything that would cause issues in here. It's actually a pretty decent idea and something I've had on my backlog for a while. Only complaint I have is that this wraps every single component in a try/catch, which won't be great for performance. If it's only done in dev mode that might be workable though. 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. @developit I thought 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. ¯\(ツ)/¯ 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. It is dev only though 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. :O 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. The advantage of shipping this in the prod version would be preventing surprises, and handling runtime errors for the end user better. Food for thought. 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. Agreed! In my own work, I'm looking at installing this error boundary in a subset of components at key points in the tree for production. Those can generate useful error logs! 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. Yeah, I think we would probably catch at the page boundary for Next 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. btw - might be useful to pass a reference to |
||
} |
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 attempt to re-wrap in
forceUpdate()
?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'm not sure but it's for hot reload according to the original code.(https://gist.github.com/Aldredcz/4d63b0a9049b00f54439f8780be7f0d8)