Skip to content
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

Add support for componentDidCatch Component method #819

Closed
wants to merge 33 commits into from
Closed
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
d54d337
Add support for Component.componentDidCatch variant with single argument
rpetrich Sep 20, 2017
6ff8cc6
Make root components have their _ancestorComponent property set to un…
rpetrich Sep 20, 2017
721b437
Merge branch 'master' into componentDidCatch-support
developit Jan 25, 2018
dde11b2
Merge branch 'master' into componentDidCatch-support
developit Feb 23, 2018
e105500
Merge remote-tracking branch 'upstream/master' into componentDidCatch…
rpetrich May 8, 2018
3705d4e
Merge remote-tracking branch 'upstream/master' into componentDidCatch…
rpetrich May 14, 2018
e8776c7
Catch errors that occur during async renders
rpetrich May 15, 2018
980b4cb
Catch and propagate errors that occur in componentWillUnmount
rpetrich May 15, 2018
89efb5e
Mount a component if it's about to receive an error and re-render any…
rpetrich May 15, 2018
b671425
Propagate a errors up the chain when a component renders children tha…
rpetrich May 15, 2018
31f5fd9
Merge remote-tracking branch 'origin/componentDidCatch-support' into …
rpetrich May 15, 2018
3a3d7cb
Merge branch 'master' into componentDidCatch-support
reznord May 16, 2018
313ffe6
Merge branch 'master' into componentDidCatch-support
reznord May 19, 2018
d4612f4
Use fewer try/catch boundaries to support componentDidCatch
rpetrich May 22, 2018
1f164dd
Merge remote-tracking branch 'upstream/master' into componentDidCatch…
rpetrich May 29, 2018
b35c534
Test new lifecycle methods for componentDidCatch support
rpetrich May 29, 2018
aa48769
Use a new _caught property to determine if a component has just caugh…
rpetrich May 30, 2018
a2b4c8c
Flush mount events before dispatching errors
rpetrich May 30, 2018
4320ff9
Send componentDidMount to a newly created parent only once when a chi…
rpetrich May 30, 2018
4338a7c
Ensure that componentWillUnmount is sent only once, even in error paths
rpetrich May 30, 2018
74fddde
Remove error parameter in componentDidCatch (which triggered linter w…
rpetrich May 30, 2018
65ff737
Merge remote-tracking branch 'upstream/master' into componentDidCatch…
rpetrich May 30, 2018
8b23777
Enqueue renders triggered from componentDidCatch instead of invoking …
rpetrich May 30, 2018
308d8aa
Fully debounce rendering during lifecycle tests (because error handli…
rpetrich May 30, 2018
b853143
Merge remote-tracking branch 'origin/componentDidCatch-support' into …
rpetrich May 30, 2018
30cde7b
Avoid infinite rerender loop when top-most error boundary attempts to…
rpetrich May 30, 2018
b98159a
Merge remote-tracking branch 'upstream/master' into componentDidCatch…
rpetrich Jun 5, 2018
286e6ea
Add test to verify that componentDidCatch errors stop bubbling once c…
rpetrich Jun 5, 2018
be4dee5
Add JSDoc comments for componentRoot/ancestorComponent parameters
rpetrich Jun 5, 2018
a019562
Check if component is disabled before dispatching beforeUnmount hook …
rpetrich Jun 5, 2018
3a2a39f
Merge branch 'master' into componentDidCatch-support
rpetrich Jun 12, 2018
e7cfc20
Merge branch 'master' into componentDidCatch-support
reznord Jul 12, 2018
3ef85c4
Merge remote-tracking branch 'upstream/master' into componentDidCatch…
rpetrich Aug 5, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion config/properties.json
@@ -1,8 +1,9 @@
{
"props": {
"cname": 16,
"cname": 17,
"props": {
"$_dirty": "__d",
"$_caught": "__e",
"$_disable": "__x",
"$_listeners": "__l",
"$_renderCallbacks": "__h",
Expand All @@ -14,6 +15,7 @@
"$prevProps": "__p",
"$prevState": "__s",
"$_parentComponent": "__u",
"$_ancestorComponent": "__a",
"$_componentConstructor": "_componentConstructor",
"$__html": "__html",
"$_component": "_component",
Expand Down
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -19,7 +19,7 @@
"transpile:esm": "rollup -c config/rollup.config.esm.js",
"transpile:debug": "babel debug/ -o debug.js -s",
"transpile": "npm-run-all transpile:main transpile:esm transpile:devtools transpile:debug",
"optimize": "uglifyjs dist/preact.dev.js -c conditionals=false,sequences=false,loops=false,join_vars=false,collapse_vars=false --pure-funcs=Object.defineProperty --mangle-props --mangle-regex=\"/^(_|normalizedNodeName|nextBase|prev[CPS]|_parentC)/\" --name-cache config/properties.json -b width=120,quote_style=3 -o dist/preact.js -p relative --in-source-map dist/preact.dev.js.map --source-map dist/preact.js.map",
"optimize": "uglifyjs dist/preact.dev.js -c conditionals=false,sequences=false,loops=false,join_vars=false,collapse_vars=false --pure-funcs=Object.defineProperty --mangle-props --mangle-regex=\"/^(_|normalizedNodeName|nextBase|prev[CPS]|_parentC|_ancestorC)/\" --name-cache config/properties.json -b width=120,quote_style=3 -o dist/preact.js -p relative --in-source-map dist/preact.dev.js.map --source-map dist/preact.js.map",
"minify": "uglifyjs dist/preact.js -c collapse_vars,evaluate,screw_ie8,unsafe,loops=false,keep_fargs=false,pure_getters,unused,dead_code -m -o dist/preact.min.js -p relative --in-source-map dist/preact.js.map --source-map dist/preact.min.js.map",
"strip:main": "jscodeshift --run-in-band -s -t config/codemod-strip-tdz.js dist/preact.dev.js && jscodeshift --run-in-band -s -t config/codemod-const.js dist/preact.dev.js && jscodeshift --run-in-band -s -t config/codemod-let-name.js dist/preact.dev.js",
"strip:esm": "jscodeshift --run-in-band -s -t config/codemod-strip-tdz.js dist/preact.esm.js && jscodeshift --run-in-band -s -t config/codemod-const.js dist/preact.esm.js && jscodeshift --run-in-band -s -t config/codemod-let-name.js dist/preact.esm.js",
Expand Down
1 change: 1 addition & 0 deletions src/component.js
Expand Up @@ -19,6 +19,7 @@ import { enqueueRender } from './render-queue';
*/
export function Component(props, context) {
this._dirty = true;
this._caught = false;

/**
* @public
Expand Down
1 change: 1 addition & 0 deletions src/preact.d.ts
Expand Up @@ -80,6 +80,7 @@ declare namespace preact {
shouldComponentUpdate?(nextProps: Readonly<P>, nextState: Readonly<S>, nextContext: any): boolean;
componentWillUpdate?(nextProps: Readonly<P>, nextState: Readonly<S>, nextContext: any): void;
componentDidUpdate?(previousProps: Readonly<P>, previousState: Readonly<S>, previousContext: any): void;
componentDidCatch?(error: any): void;
}

abstract class Component<P, S> {
Expand Down
2 changes: 1 addition & 1 deletion src/render.js
Expand Up @@ -18,5 +18,5 @@ import { diff } from './vdom/diff';
* render(<Thing name="one" />, document.querySelector('#foo'));
*/
export function render(vnode, parent, merge) {
return diff(merge, vnode, {}, false, parent, false);
return diff(merge, vnode, {}, false, parent);
}
6 changes: 4 additions & 2 deletions src/vdom/component-recycler.js
Expand Up @@ -26,9 +26,11 @@ export function collectComponent(component) {
* @param {function} Ctor The constructor of the component to create
* @param {object} props The initial props of the component
* @param {object} context The initial context of the component
* @param {import('../component').Component} [ancestorComponent] The nearest ancestor component beneath
* which the new component will be mounted
* @returns {import('../component').Component}
*/
export function createComponent(Ctor, props, context) {
export function createComponent(Ctor, props, context, ancestorComponent) {
let list = components[Ctor.name],
inst;

Expand All @@ -41,7 +43,7 @@ export function createComponent(Ctor, props, context) {
inst.constructor = Ctor;
inst.render = doRender;
}

inst._ancestorComponent = ancestorComponent;

if (list) {
for (let i=list.length; i--; ) {
Expand Down
249 changes: 148 additions & 101 deletions src/vdom/component.js
Expand Up @@ -53,6 +53,23 @@ export function setComponentProps(component, props, renderMode, context, mountAl
if (component.__ref) component.__ref(component);
}

export function catchErrorInComponent(error, component) {
flushMounts();
for (; component; component = component._ancestorComponent) {
if (component.componentDidCatch && !component._caught) {
try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need another try/catch here? I would think that if componentDidCatch exists, the error is passed to it & it ends there. If the method does not exist, it's uncaught so the component (tree) is unmounted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errors that occur in componentDidCatch should bubble to ancestors, regardless of what lifecycle method generated the error or from what path that lifecycle method occurred. Error paths should be rare, so it's fine for it to be slow.

component.componentDidCatch(error);
component._caught = true;
enqueueRender(component);
return;
} catch (e) {
error = e;
}
}
}
throw error;
}



/**
Expand All @@ -79,129 +96,150 @@ export function renderComponent(component, renderMode, mountAll, isChild) {
initialChildComponent = component._component,
skip = false,
snapshot = previousContext,
rendered, inst, cbase;

if (component.constructor.getDerivedStateFromProps) {
previousState = extend({}, previousState);
component.state = extend(state, component.constructor.getDerivedStateFromProps(props, state));
}

// if updating
if (isUpdate) {
component.props = previousProps;
component.state = previousState;
component.context = previousContext;
if (renderMode!==FORCE_RENDER
&& component.shouldComponentUpdate
&& component.shouldComponentUpdate(props, state, context) === false) {
skip = true;
}
else if (component.componentWillUpdate) {
component.componentWillUpdate(props, state, context);
}
component.props = props;
component.state = state;
component.context = context;
}

component.prevProps = component.prevState = component.prevContext = component.nextBase = null;
component._dirty = false;
rendered, inst, cbase,
exception, clearCaught = component._caught;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand the clearCaught variable more. Is there a test (or subset of tests) that best demonstrate what this variable is trying to prevent? One of your commit messages related to this variable mention "Avoid infinite loops" - is the "should bubble on repeated errors" relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _caught property and clearCaught variable are to coordinate when a component rerenders in response to an error from one of its children, but the new children it returns in response to the first error also throw an error. Without handling this case specifically, the component would receive componentDidCatch again and continue to ask for the same failing child node tree to be rendered. Instead, the error should bubble further up the chain to be handled by another component.

The "should bubble on repeated errors" and "should bubble on ignored errors" tests cover these cases. Repeated errors is the normal case to handle, as it isn't possible for an error boundary component to anticipate every error that could occur in fallback child tree. Components that implement componentDidCatch but fail to even ask to be re-rendered upon catching an error are completely derelict in their duty, but it's useful to have a test for them anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to explain this - it helps!


if (!skip) {
rendered = component.render(props, state, context);

// context to pass to the child, can be updated via (grand-)parent component
if (component.getChildContext) {
context = extend(extend({}, context), component.getChildContext());
try {
if (component.constructor.getDerivedStateFromProps) {
previousState = extend({}, previousState);
component.state = extend(state, component.constructor.getDerivedStateFromProps(props, state));
}

if (isUpdate && component.getSnapshotBeforeUpdate) {
snapshot = component.getSnapshotBeforeUpdate(previousProps, previousState);
// if updating
if (isUpdate) {
component.props = previousProps;
component.state = previousState;
component.context = previousContext;
if (renderMode!==FORCE_RENDER
&& component.shouldComponentUpdate
&& component.shouldComponentUpdate(props, state, context) === false) {
skip = true;
}
else if (component.componentWillUpdate) {
component.componentWillUpdate(props, state, context);
}
component.props = props;
component.state = state;
component.context = context;
}

let childComponent = rendered && rendered.nodeName,
toUnmount, base;
component.prevProps = component.prevState = component.prevContext = component.nextBase = null;
component._dirty = false;

if (typeof childComponent==='function') {
// set up high order component link
if (!skip) {
rendered = component.render(props, state, context);

let childProps = getNodeProps(rendered);
inst = initialChildComponent;
// context to pass to the child, can be updated via (grand-)parent component
if (component.getChildContext) {
context = extend(extend({}, context), component.getChildContext());
}

if (inst && inst.constructor===childComponent && childProps.key==inst.__key) {
setComponentProps(inst, childProps, SYNC_RENDER, context, false);
if (isUpdate && component.getSnapshotBeforeUpdate) {
snapshot = component.getSnapshotBeforeUpdate(previousProps, previousState);
}
else {
toUnmount = inst;

component._component = inst = createComponent(childComponent, childProps, context);
inst.nextBase = inst.nextBase || nextBase;
inst._parentComponent = component;
setComponentProps(inst, childProps, NO_RENDER, context, false);
renderComponent(inst, SYNC_RENDER, mountAll, true);

let childComponent = rendered && rendered.nodeName,
toUnmount, base;
try {
if (typeof childComponent==='function') {
// set up high order component link

let childProps = getNodeProps(rendered);
inst = initialChildComponent;

if (inst && inst.constructor===childComponent && childProps.key==inst.__key) {
setComponentProps(inst, childProps, SYNC_RENDER, context, false);
}
else {
toUnmount = inst;

component._component = inst = createComponent(childComponent, childProps, context, component);
inst.nextBase = inst.nextBase || nextBase;
inst._parentComponent = component;
setComponentProps(inst, childProps, NO_RENDER, context, false);
renderComponent(inst, SYNC_RENDER, mountAll, true);
}

base = inst.base;
}
else {
cbase = initialBase;

// destroy high order component link
toUnmount = initialChildComponent;
if (toUnmount) {
cbase = component._component = null;
}

if (initialBase || renderMode===SYNC_RENDER) {
if (cbase) cbase._component = null;
base = diff(cbase, rendered, context, mountAll || !isUpdate, initialBase && initialBase.parentNode, component);
}
}
} catch (e) {
exception = e;
clearCaught = false;
if (!base) {
base = initialBase || document.createTextNode("");
}
}

base = inst.base;
}
else {
cbase = initialBase;
if (initialBase && base!==initialBase && inst!==initialChildComponent) {
let baseParent = initialBase.parentNode;
if (baseParent && base!==baseParent) {
baseParent.replaceChild(base, initialBase);

// destroy high order component link
toUnmount = initialChildComponent;
if (toUnmount) {
cbase = component._component = null;
if (!toUnmount) {
initialBase._component = null;
recollectNodeTree(initialBase, false);
}
}
}

if (initialBase || renderMode===SYNC_RENDER) {
if (cbase) cbase._component = null;
base = diff(cbase, rendered, context, mountAll || !isUpdate, initialBase && initialBase.parentNode, true);
if (toUnmount && toUnmount.base) {
unmountComponent(toUnmount);
}
}

if (initialBase && base!==initialBase && inst!==initialChildComponent) {
let baseParent = initialBase.parentNode;
if (baseParent && base!==baseParent) {
baseParent.replaceChild(base, initialBase);

if (!toUnmount) {
initialBase._component = null;
recollectNodeTree(initialBase, false);
component.base = base;
if (base && !isChild) {
let componentRef = component,
t = component;
while ((t=t._parentComponent)) {
(componentRef = t).base = base;
}
base._component = componentRef;
base._componentConstructor = componentRef.constructor;
}
}

if (toUnmount) {
unmountComponent(toUnmount);
if (!isUpdate || mountAll) {
mounts.unshift(component);
}

component.base = base;
if (base && !isChild) {
let componentRef = component,
t = component;
while ((t=t._parentComponent)) {
(componentRef = t).base = base;
else if (!skip) {
// Ensure that pending componentDidMount() hooks of child components
// are called before the componentDidUpdate() hook in the parent.
// Note: disabled as it causes duplicate hooks, see https://github.com/developit/preact/issues/750
// flushMounts();

if (component.componentDidUpdate) {
component.componentDidUpdate(previousProps, previousState, snapshot);
}
base._component = componentRef;
base._componentConstructor = componentRef.constructor;
if (options.afterUpdate) options.afterUpdate(component);
}
}

if (!isUpdate || mountAll) {
mounts.unshift(component);
}
else if (!skip) {
// Ensure that pending componentDidMount() hooks of child components
// are called before the componentDidUpdate() hook in the parent.
// Note: disabled as it causes duplicate hooks, see https://github.com/developit/preact/issues/750
// flushMounts();

if (component.componentDidUpdate) {
component.componentDidUpdate(previousProps, previousState, snapshot);
while (component._renderCallbacks.length) component._renderCallbacks.pop().call(component);

if (clearCaught) {
component._caught = false;
}
if (options.afterUpdate) options.afterUpdate(component);

} catch (e) {
catchErrorInComponent(e, component._ancestorComponent);
}

while (component._renderCallbacks.length) component._renderCallbacks.pop().call(component);
if (typeof exception !== "undefined") {
catchErrorInComponent(exception, component);
}

if (!diffLevel && !isChild) flushMounts();
}
Expand All @@ -214,10 +252,12 @@ export function renderComponent(component, renderMode, mountAll, isChild) {
* @param {import('../vnode').VNode} vnode A Component-referencing VNode
* @param {object} context The current context
* @param {boolean} mountAll Whether or not to immediately mount all components
* @param {import('../component').Component} [ancestorComponent] The nearest ancestor component
* beneath which the new component will be mounted
* @returns {import('../dom').PreactElement} The created/mutated element
* @private
*/
export function buildComponentFromVNode(dom, vnode, context, mountAll) {
export function buildComponentFromVNode(dom, vnode, context, mountAll, ancestorComponent) {
let c = dom && dom._component,
originalComponent = c,
oldDom = dom,
Expand All @@ -238,7 +278,7 @@ export function buildComponentFromVNode(dom, vnode, context, mountAll) {
dom = oldDom = null;
}

c = createComponent(vnode.nodeName, props, context);
c = createComponent(vnode.nodeName, props, context, ancestorComponent);
if (dom && !c.nextBase) {
c.nextBase = dom;
// passing dom/oldDom as nextBase will recycle it if unused, so bypass recycling on L229:
Expand All @@ -264,13 +304,20 @@ export function buildComponentFromVNode(dom, vnode, context, mountAll) {
* @private
*/
export function unmountComponent(component) {
if (component._disable) return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this check be the first thing that happens in this function (in other words, before the call to options.beforeUnmount) to avoid calling beforeUnmount multiple times?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely an oversight that the check against _disable wasn't the first operation in the function.

component._disable = true;

if (options.beforeUnmount) options.beforeUnmount(component);

let base = component.base;

component._disable = true;

if (component.componentWillUnmount) component.componentWillUnmount();
if (component.componentWillUnmount) {
try {
component.componentWillUnmount();
} catch (e) {
catchErrorInComponent(e, component._ancestorComponent);
}
}

component.base = null;

Expand Down