From d54d337e4a8c59df01adb44f5740c6f4d2052765 Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Wed, 20 Sep 2017 12:51:36 -0400 Subject: [PATCH 01/19] Add support for Component.componentDidCatch variant with single argument --- config/properties.json | 1 + package.json | 2 +- src/preact.d.ts | 1 + src/vdom/component-recycler.js | 4 +- src/vdom/component.js | 81 ++++++++++------ src/vdom/diff.js | 43 +++++---- test/browser/lifecycle.js | 168 +++++++++++++++++++++++++++++++++ 7 files changed, 249 insertions(+), 51 deletions(-) diff --git a/config/properties.json b/config/properties.json index ebde50a3d6..7b218e22d8 100644 --- a/config/properties.json +++ b/config/properties.json @@ -14,6 +14,7 @@ "$prevProps": "__p", "$prevState": "__s", "$_parentComponent": "__u", + "$_ancestorComponent": "__a", "$_componentConstructor": "_componentConstructor", "$__html": "__html", "$_component": "_component", diff --git a/package.json b/package.json index 2e7ab6181a..c6fdb5dbcb 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,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", diff --git a/src/preact.d.ts b/src/preact.d.ts index 6fcdb02f3f..ea205ca788 100644 --- a/src/preact.d.ts +++ b/src/preact.d.ts @@ -30,6 +30,7 @@ declare namespace preact { shouldComponentUpdate?(nextProps:PropsType,nextState:StateType,nextContext:any):boolean; componentWillUpdate?(nextProps:PropsType,nextState:StateType,nextContext:any):void; componentDidUpdate?(previousProps:PropsType,previousState:StateType,previousContext:any):void; + componentDidCatch?(error:any):void; } interface FunctionalComponent { diff --git a/src/vdom/component-recycler.js b/src/vdom/component-recycler.js index 33f74d905d..1f1b93950a 100644 --- a/src/vdom/component-recycler.js +++ b/src/vdom/component-recycler.js @@ -15,7 +15,7 @@ export function collectComponent(component) { /** Create a component. Normalizes differences between PFC's and classful Components. */ -export function createComponent(Ctor, props, context) { +export function createComponent(Ctor, props, context, ancestorComponent) { let list = components[Ctor.name], inst; @@ -28,7 +28,7 @@ export function createComponent(Ctor, props, context) { inst.constructor = Ctor; inst.render = doRender; } - + inst._ancestorComponent = ancestorComponent; if (list) { for (let i=list.length; i--; ) { diff --git a/src/vdom/component.js b/src/vdom/component.js index 5286735174..fd39aade5f 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -49,6 +49,18 @@ export function setComponentProps(component, props, opts, context, mountAll) { if (component.__ref) component.__ref(component); } +export function catchErrorInComponent(error, component) { + for (; component; component = component._ancestorComponent) { + if (component.componentDidCatch) { + try { + return component.componentDidCatch(error); + } catch (e) { + error = e; + } + } + } + throw error; +} /** Render a Component, triggering necessary lifecycle events and taking High-Order Components into account. @@ -105,40 +117,45 @@ export function renderComponent(component, opts, mountAll, isChild) { let childComponent = rendered && rendered.nodeName, toUnmount, base; - if (typeof childComponent==='function') { - // set up high order component link + try { + if (typeof childComponent==='function') { + // set up high order component link - let childProps = getNodeProps(rendered); - inst = initialChildComponent; + 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; + 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); + } - 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); + base = inst.base; } + else { + cbase = initialBase; - base = inst.base; - } - else { - cbase = initialBase; - - // destroy high order component link - toUnmount = initialChildComponent; - if (toUnmount) { - cbase = component._component = null; - } + // destroy high order component link + toUnmount = initialChildComponent; + if (toUnmount) { + cbase = component._component = null; + } - if (initialBase || opts===SYNC_RENDER) { - if (cbase) cbase._component = null; - base = diff(cbase, rendered, context, mountAll || !isUpdate, initialBase && initialBase.parentNode, true); + if (initialBase || opts===SYNC_RENDER) { + if (cbase) cbase._component = null; + base = diff(cbase, rendered, context, mountAll || !isUpdate, initialBase && initialBase.parentNode, component); + } } + } catch (e) { + base = initialBase || document.createTextNode(""); + catchErrorInComponent(e, component); } if (initialBase && base!==initialBase && inst!==initialChildComponent) { @@ -179,7 +196,11 @@ export function renderComponent(component, opts, mountAll, isChild) { // flushMounts(); if (component.componentDidUpdate) { - component.componentDidUpdate(previousProps, previousState, previousContext); + try { + component.componentDidUpdate(previousProps, previousState, previousContext); + } catch (e) { + catchErrorInComponent(e, component._ancestorComponent); + } } if (options.afterUpdate) options.afterUpdate(component); } @@ -199,7 +220,7 @@ export function renderComponent(component, opts, mountAll, isChild) { * @returns {Element} dom 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, @@ -220,7 +241,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: diff --git a/src/vdom/diff.js b/src/vdom/diff.js index 6cc9bb7106..c1ace29875 100644 --- a/src/vdom/diff.js +++ b/src/vdom/diff.js @@ -1,6 +1,6 @@ import { ATTR_KEY } from '../constants'; import { isSameNodeType, isNamedNode } from './index'; -import { buildComponentFromVNode } from './component'; +import { buildComponentFromVNode, catchErrorInComponent } from './component'; import { createNode, setAccessor } from '../dom/index'; import { unmountComponent } from './component'; import options from '../options'; @@ -23,7 +23,13 @@ export function flushMounts() { let c; while ((c=mounts.pop())) { if (options.afterMount) options.afterMount(c); - if (c.componentDidMount) c.componentDidMount(); + if (c.componentDidMount) { + try { + c.componentDidMount(); + } catch (e) { + catchErrorInComponent(e, c._ancestorComponent); + } + } } } @@ -44,19 +50,20 @@ export function diff(dom, vnode, context, mountAll, parent, componentRoot) { hydrating = dom!=null && !(ATTR_KEY in dom); } - let ret = idiff(dom, vnode, context, mountAll, componentRoot); - - // append the element if its a new parent - if (parent && ret.parentNode!==parent) parent.appendChild(ret); - - // diffLevel being reduced to 0 means we're exiting the diff - if (!--diffLevel) { - hydrating = false; - // invoke queued componentDidMount lifecycle methods - if (!componentRoot) flushMounts(); + let ret; + try { + return ret = idiff(dom, vnode, context, mountAll, componentRoot); + } finally { + // append the element if its a new parent + if (ret && parent && ret.parentNode!==parent) parent.appendChild(ret); + + // diffLevel being reduced to 0 means we're exiting the diff + if (!--diffLevel) { + hydrating = false; + // invoke queued componentDidMount lifecycle methods + if (!componentRoot) flushMounts(); + } } - - return ret; } @@ -97,7 +104,7 @@ function idiff(dom, vnode, context, mountAll, componentRoot) { // If the VNode represents a Component, perform a component diff: let vnodeName = vnode.nodeName; if (typeof vnodeName==='function') { - return buildComponentFromVNode(dom, vnode, context, mountAll); + return buildComponentFromVNode(dom, vnode, context, mountAll, componentRoot); } @@ -140,7 +147,7 @@ function idiff(dom, vnode, context, mountAll, componentRoot) { } // otherwise, if there are existing or new children, diff them: else if (vchildren && vchildren.length || fc!=null) { - innerDiffNode(out, vchildren, context, mountAll, hydrating || props.dangerouslySetInnerHTML!=null); + innerDiffNode(out, vchildren, context, mountAll, hydrating || props.dangerouslySetInnerHTML!=null, componentRoot); } @@ -162,7 +169,7 @@ function idiff(dom, vnode, context, mountAll, componentRoot) { * @param {Boolean} mountAll * @param {Boolean} isHydrating If `true`, consumes externally created elements similar to hydration */ -function innerDiffNode(dom, vchildren, context, mountAll, isHydrating) { +function innerDiffNode(dom, vchildren, context, mountAll, isHydrating, componentRoot) { let originalChildren = dom.childNodes, children = [], keyed = {}, @@ -217,7 +224,7 @@ function innerDiffNode(dom, vchildren, context, mountAll, isHydrating) { } // morph the matched/found/created DOM child to match vchild (deep) - child = idiff(child, vchild, context, mountAll); + child = idiff(child, vchild, context, mountAll, componentRoot); f = originalChildren[i]; if (child && child!==dom && child!==f) { diff --git a/test/browser/lifecycle.js b/test/browser/lifecycle.js index 4a22c78e55..8c392e31eb 100644 --- a/test/browser/lifecycle.js +++ b/test/browser/lifecycle.js @@ -455,6 +455,174 @@ describe('Lifecycle methods', () => { }); }); + describe('#componentDidCatch', () => { + + it('should be called when child fails in constructor', () => { + class ErrorReceiverComponent extends Component { + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + class ErrorGeneratorComponent extends Component { + constructor(props, context) { + super(props, context); + throw "Error!"; + } + } + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + }); + + it('should be called when child fails in componentWillMount', () => { + class ErrorReceiverComponent extends Component { + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + class ErrorGeneratorComponent extends Component { + componentWillMount() { + throw "Error!"; + } + } + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + }); + + it('should be called when child fails in render', () => { + class ErrorReceiverComponent extends Component { + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + class ErrorGeneratorComponent extends Component { + render() { + throw "Error!"; + } + } + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + }); + + it('should be called when child fails in componentDidMount', () => { + class ErrorReceiverComponent extends Component { + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + class ErrorGeneratorComponent extends Component { + componentDidMount() { + throw "Error!"; + } + } + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + }); + + it('should be called when functional child fails', () => { + class ErrorReceiverComponent extends Component { + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + function ErrorGeneratorComponent() { + throw "Error!"; + } + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + }); + + it('should re-render with new content', () => { + class ErrorReceiverComponent extends Component { + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return {this.state.error ? String(this.state.error) : this.props.children}; + } + } + class ErrorGeneratorComponent extends Component { + render() { + return "No error!?!?"; + } + componentWillMount() { + throw "Error contents"; + } + } + render(, scratch); + rerender(); + expect(scratch).to.have.property('textContent', 'Error contents'); + }); + + it('should be able to adapt and rethrow errors', () => { + class ErrorReceiverComponent extends Component { + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + class ErrorAdapterComponent extends Component { + componentDidCatch(error) { + throw "Adapted " + String(error); + } + render() { + return
{this.props.children}
; + } + } + function ErrorGeneratorComponent() { + throw "Error!"; + } + sinon.spy(ErrorAdapterComponent.prototype, 'componentDidCatch'); + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); + expect(ErrorAdapterComponent.prototype.componentDidCatch).to.have.been.called; + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + rerender(); + expect(scratch).to.have.property('textContent', 'Adapted Error!'); + }); + + it('should be called through non-component parent elements', () => { + class ErrorReceiverComponent extends Component { + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + class ErrorGeneratorComponent extends Component { + constructor(props, context) { + super(props, context); + throw "Error!"; + } + } + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(
, scratch); + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + }); + + }); describe('Lifecycle DOM Timing', () => { it('should be invoked when dom does (DidMount, WillUnmount) or does not (WillMount, DidUnmount) exist', () => { From 6ff8cc6272b28602886ca6486ae5986922b1fb28 Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Wed, 20 Sep 2017 13:47:57 -0400 Subject: [PATCH 02/19] Make root components have their _ancestorComponent property set to undefined; Add tests for _ancestorComponent/_parentComponent --- src/render.js | 2 +- test/browser/components.js | 46 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/render.js b/src/render.js index 2774a5e92b..e0e8526ec2 100644 --- a/src/render.js +++ b/src/render.js @@ -16,5 +16,5 @@ import { diff } from './vdom/diff'; * render(, document.querySelector('#foo')); */ export function render(vnode, parent, merge) { - return diff(merge, vnode, {}, false, parent, false); + return diff(merge, vnode, {}, false, parent); } diff --git a/test/browser/components.js b/test/browser/components.js index 904559df11..f2d270470f 100644 --- a/test/browser/components.js +++ b/test/browser/components.js @@ -774,4 +774,50 @@ describe('Components', () => { expect(C3.prototype.componentWillMount, 'inject between, C3 w/ intermediary div').to.have.been.calledOnce; }); }); + + describe('Properties', () => { + let ancestorComponent; + let childComponent; + class Ancestor extends Component { + constructor(props, context) { + super(props, context); + ancestorComponent = this; + } + render() { + return this.props.children[0]; + } + } + class Child extends Component { + constructor(props, context) { + super(props, context); + childComponent = this; + } + render() { + return
; + } + } + const HighOrderChild = () => ; + it('should have correct ancestor/parent with immediate child', () => { + render(, scratch); + expect(childComponent._ancestorComponent).to.equal(ancestorComponent); + expect(childComponent._parentComponent).to.equal(ancestorComponent); + expect(ancestorComponent._ancestorComponent).to.equal(undefined); + expect(ancestorComponent._parentComponent).to.equal(undefined); + }); + it('should have correct ancestor/parent with grandchild', () => { + render(
, scratch); + expect(childComponent._ancestorComponent).to.equal(ancestorComponent); + expect(childComponent._parentComponent).to.equal(undefined); + expect(ancestorComponent._ancestorComponent).to.equal(undefined); + expect(ancestorComponent._parentComponent).to.equal(undefined); + }); + it('should have correct ancestor/parent with high-order grandchild', () => { + render(
, scratch); + expect(childComponent._ancestorComponent).to.not.equal(undefined); + expect(childComponent._ancestorComponent).to.equal(childComponent._parentComponent); + expect(childComponent._ancestorComponent._ancestorComponent).to.equal(ancestorComponent); + expect(ancestorComponent._ancestorComponent).to.equal(undefined); + expect(ancestorComponent._parentComponent).to.equal(undefined); + }); + }); }); From e8776c7f2222240b8fbec97a0461523cfb54c7dd Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Mon, 14 May 2018 21:41:40 -0400 Subject: [PATCH 03/19] Catch errors that occur during async renders --- src/render-queue.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/render-queue.js b/src/render-queue.js index 42021f69c7..52a2d09f70 100644 --- a/src/render-queue.js +++ b/src/render-queue.js @@ -1,6 +1,6 @@ import options from './options'; import { defer } from './util'; -import { renderComponent } from './vdom/component'; +import { renderComponent, catchErrorInComponent } from './vdom/component'; /** Managed queue of dirty components to be re-rendered */ @@ -16,6 +16,12 @@ export function rerender() { let p, list = items; items = []; while ( (p = list.pop()) ) { - if (p._dirty) renderComponent(p); + if (p._dirty) { + try { + renderComponent(p); + } catch (e) { + catchErrorInComponent(e, p._ancestorComponent); + } + } } } From 980b4cbf50dae9c399ef9de1fa915891232c7adf Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Mon, 14 May 2018 21:42:52 -0400 Subject: [PATCH 04/19] Catch and propagate errors that occur in componentWillUnmount --- src/vdom/component.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/vdom/component.js b/src/vdom/component.js index fd39aade5f..6912062dc3 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -272,7 +272,13 @@ export function unmountComponent(component) { component._disable = true; - if (component.componentWillUnmount) component.componentWillUnmount(); + if (component.componentWillUnmount) { + try { + component.componentWillUnmount(); + } catch (e) { + catchErrorInComponent(e, component._ancestorComponent); + } + } component.base = null; From 89efb5e138736ed9826a41a266f974ab21c8f461 Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Mon, 14 May 2018 21:44:58 -0400 Subject: [PATCH 05/19] Mount a component if it's about to receive an error and re-render any components that update their state as a result of handling an error that occurred during render --- src/vdom/component.js | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/vdom/component.js b/src/vdom/component.js index 6912062dc3..bd69a0c60b 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -83,7 +83,8 @@ export function renderComponent(component, opts, mountAll, isChild) { initialBase = isUpdate || nextBase, initialChildComponent = component._component, skip = false, - rendered, inst, cbase; + rendered, inst, cbase, + exception, hasException; // if updating if (isUpdate) { @@ -155,7 +156,8 @@ export function renderComponent(component, opts, mountAll, isChild) { } } catch (e) { base = initialBase || document.createTextNode(""); - catchErrorInComponent(e, component); + exception = e; + hasException = true; } if (initialBase && base!==initialBase && inst!==initialChildComponent) { @@ -196,11 +198,7 @@ export function renderComponent(component, opts, mountAll, isChild) { // flushMounts(); if (component.componentDidUpdate) { - try { - component.componentDidUpdate(previousProps, previousState, previousContext); - } catch (e) { - catchErrorInComponent(e, component._ancestorComponent); - } + component.componentDidUpdate(previousProps, previousState, previousContext); } if (options.afterUpdate) options.afterUpdate(component); } @@ -209,6 +207,14 @@ export function renderComponent(component, opts, mountAll, isChild) { while (component._renderCallbacks.length) component._renderCallbacks.pop().call(component); } + if (hasException) { + flushMounts(); + catchErrorInComponent(exception, component); + if (component._dirty) { + renderComponent(component, opts, mountAll, isChild); + } + } + if (!diffLevel && !isChild) flushMounts(); } From b6714250b0b33ca5b0752b83d6052bbae6916427 Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Mon, 14 May 2018 21:53:59 -0400 Subject: [PATCH 06/19] Propagate a errors up the chain when a component renders children that throw an error after having just received an error via componentDidCatch (to handle cases where an error boundary fails to properly render an error it's received) --- src/constants.js | 3 ++- src/vdom/component.js | 11 +++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/constants.js b/src/constants.js index 23be941052..438b20f08d 100644 --- a/src/constants.js +++ b/src/constants.js @@ -3,7 +3,8 @@ export const NO_RENDER = 0; export const SYNC_RENDER = 1; export const FORCE_RENDER = 2; -export const ASYNC_RENDER = 3; +export const ASYNC_RENDER = 4; +export const REATTEMPT_RENDER = SYNC_RENDER + 8; // Like sync render, but throws any errors that occur export const ATTR_KEY = '__preactattr_'; diff --git a/src/vdom/component.js b/src/vdom/component.js index bd69a0c60b..2034596490 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -1,4 +1,4 @@ -import { SYNC_RENDER, NO_RENDER, FORCE_RENDER, ASYNC_RENDER, ATTR_KEY } from '../constants'; +import { SYNC_RENDER, NO_RENDER, FORCE_RENDER, ASYNC_RENDER, REATTEMPT_RENDER, ATTR_KEY } from '../constants'; import options from '../options'; import { extend } from '../util'; import { enqueueRender } from '../render-queue'; @@ -38,7 +38,7 @@ export function setComponentProps(component, props, opts, context, mountAll) { component._disable = false; if (opts!==NO_RENDER) { - if (opts===SYNC_RENDER || options.syncComponentUpdates!==false || !component.base) { + if ((opts&SYNC_RENDER) || options.syncComponentUpdates!==false || !component.base) { renderComponent(component, SYNC_RENDER, mountAll); } else { @@ -149,12 +149,15 @@ export function renderComponent(component, opts, mountAll, isChild) { cbase = component._component = null; } - if (initialBase || opts===SYNC_RENDER) { + if (initialBase || (opts&SYNC_RENDER)) { if (cbase) cbase._component = null; base = diff(cbase, rendered, context, mountAll || !isUpdate, initialBase && initialBase.parentNode, component); } } } catch (e) { + if (opts == REATTEMPT_RENDER) { + throw e; + } base = initialBase || document.createTextNode(""); exception = e; hasException = true; @@ -211,7 +214,7 @@ export function renderComponent(component, opts, mountAll, isChild) { flushMounts(); catchErrorInComponent(exception, component); if (component._dirty) { - renderComponent(component, opts, mountAll, isChild); + renderComponent(component, REATTEMPT_RENDER, mountAll, isChild); } } From d4612f438e68a786b264ac83d279ca78af10086f Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Tue, 22 May 2018 11:45:00 -0400 Subject: [PATCH 07/19] Use fewer try/catch boundaries to support componentDidCatch --- src/constants.js | 2 +- src/render-queue.js | 10 +--- src/vdom/component.js | 115 ++++++++++++++++++++++-------------------- 3 files changed, 63 insertions(+), 64 deletions(-) diff --git a/src/constants.js b/src/constants.js index 438b20f08d..ceec02bbe6 100644 --- a/src/constants.js +++ b/src/constants.js @@ -4,7 +4,7 @@ export const NO_RENDER = 0; export const SYNC_RENDER = 1; export const FORCE_RENDER = 2; export const ASYNC_RENDER = 4; -export const REATTEMPT_RENDER = SYNC_RENDER + 8; // Like sync render, but throws any errors that occur +export const ERROR_RENDER = SYNC_RENDER | 8; // Like sync render, but throws any errors that occur export const ATTR_KEY = '__preactattr_'; diff --git a/src/render-queue.js b/src/render-queue.js index 52a2d09f70..42021f69c7 100644 --- a/src/render-queue.js +++ b/src/render-queue.js @@ -1,6 +1,6 @@ import options from './options'; import { defer } from './util'; -import { renderComponent, catchErrorInComponent } from './vdom/component'; +import { renderComponent } from './vdom/component'; /** Managed queue of dirty components to be re-rendered */ @@ -16,12 +16,6 @@ export function rerender() { let p, list = items; items = []; while ( (p = list.pop()) ) { - if (p._dirty) { - try { - renderComponent(p); - } catch (e) { - catchErrorInComponent(e, p._ancestorComponent); - } - } + if (p._dirty) renderComponent(p); } } diff --git a/src/vdom/component.js b/src/vdom/component.js index 2034596490..faf0efcb46 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -1,4 +1,4 @@ -import { SYNC_RENDER, NO_RENDER, FORCE_RENDER, ASYNC_RENDER, REATTEMPT_RENDER, ATTR_KEY } from '../constants'; +import { SYNC_RENDER, NO_RENDER, FORCE_RENDER, ASYNC_RENDER, ERROR_RENDER, ATTR_KEY } from '../constants'; import options from '../options'; import { extend } from '../util'; import { enqueueRender } from '../render-queue'; @@ -39,7 +39,7 @@ export function setComponentProps(component, props, opts, context, mountAll) { if (opts!==NO_RENDER) { if ((opts&SYNC_RENDER) || options.syncComponentUpdates!==false || !component.base) { - renderComponent(component, SYNC_RENDER, mountAll); + renderComponent(component, opts|SYNC_RENDER, mountAll); } else { enqueueRender(component); @@ -62,15 +62,7 @@ export function catchErrorInComponent(error, component) { throw error; } - -/** Render a Component, triggering necessary lifecycle events and taking High-Order Components into account. - * @param {Component} component - * @param {Object} [opts] - * @param {boolean} [opts.build=false] If `true`, component will build and store a DOM node if not already associated with one. - * @private - */ -export function renderComponent(component, opts, mountAll, isChild) { - if (component._disable) return; +function renderComponentAttempt(component, opts, mountAll, isChild) { let props = component.props, state = component.state, @@ -83,8 +75,7 @@ export function renderComponent(component, opts, mountAll, isChild) { initialBase = isUpdate || nextBase, initialChildComponent = component._component, skip = false, - rendered, inst, cbase, - exception, hasException; + rendered, inst, cbase; // if updating if (isUpdate) { @@ -118,49 +109,40 @@ export function renderComponent(component, opts, mountAll, isChild) { 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 (typeof childComponent==='function') { + // set up high order component link - 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); - } + let childProps = getNodeProps(rendered); + inst = initialChildComponent; - base = inst.base; + if (inst && inst.constructor===childComponent && childProps.key==inst.__key) { + setComponentProps(inst, childProps, opts|SYNC_RENDER, context, false); } else { - cbase = initialBase; + toUnmount = inst; - // destroy high order component link - toUnmount = initialChildComponent; - if (toUnmount) { - cbase = component._component = null; - } + 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, opts|SYNC_RENDER, mountAll, true); + } - if (initialBase || (opts&SYNC_RENDER)) { - if (cbase) cbase._component = null; - base = diff(cbase, rendered, context, mountAll || !isUpdate, initialBase && initialBase.parentNode, component); - } + base = inst.base; + } + else { + cbase = initialBase; + + // destroy high order component link + toUnmount = initialChildComponent; + if (toUnmount) { + cbase = component._component = null; } - } catch (e) { - if (opts == REATTEMPT_RENDER) { - throw e; + + if (initialBase || (opts&SYNC_RENDER)) { + if (cbase) cbase._component = null; + base = diff(cbase, rendered, context, mountAll || !isUpdate, initialBase && initialBase.parentNode, component); } - base = initialBase || document.createTextNode(""); - exception = e; - hasException = true; } if (initialBase && base!==initialBase && inst!==initialChildComponent) { @@ -210,15 +192,38 @@ export function renderComponent(component, opts, mountAll, isChild) { while (component._renderCallbacks.length) component._renderCallbacks.pop().call(component); } - if (hasException) { - flushMounts(); - catchErrorInComponent(exception, component); - if (component._dirty) { - renderComponent(component, REATTEMPT_RENDER, mountAll, isChild); + if (!diffLevel && !isChild) flushMounts(); +} + + +/** Render a Component, triggering necessary lifecycle events and taking High-Order Components into account. + * @param {Component} component + * @param {Object} [opts] + * @param {boolean} [opts.build=false] If `true`, component will build and store a DOM node if not already associated with one. + * @private + */ +export function renderComponent(component, opts, mountAll, isChild) { + if (component._disable) return; + + if (opts !== ERROR_RENDER) { + try { + renderComponentAttempt(component, opts, mountAll, isChild); + } catch (e) { + catchErrorInComponent(e, component); } - } - if (!diffLevel && !isChild) flushMounts(); + if (component._dirty && component.componentDidCatch) { + // Error was caught, rerender with errors in children propagated via throw + try { + renderComponentAttempt(component, ERROR_RENDER, mountAll, isChild); + } catch (e) { + catchErrorInComponent(e, component._ancestorComponent); + } + } + } else { + // Asked by a call further up the stack to propagate errors via throw + renderComponentAttempt(component, opts, mountAll, isChild); + } } From b35c5345b26cc1573673303f22db596aa421e69f Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Tue, 29 May 2018 18:15:08 -0400 Subject: [PATCH 08/19] Test new lifecycle methods for componentDidCatch support --- test/browser/lifecycle.js | 168 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 168 insertions(+) diff --git a/test/browser/lifecycle.js b/test/browser/lifecycle.js index 136d114652..699ca83b7b 100644 --- a/test/browser/lifecycle.js +++ b/test/browser/lifecycle.js @@ -945,6 +945,174 @@ describe('Lifecycle methods', () => { expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; }); + it('should be called when child fails in getDerivedStateFromProps', () => { + class ErrorReceiverComponent extends Component { + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + class ErrorGeneratorComponent extends Component { + static getDerivedStateFromProps() { + throw "Error!"; + } + render() { + return Should not get here; + } + } + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + sinon.spy(ErrorGeneratorComponent.prototype, 'render'); + render(, scratch); + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + expect(ErrorGeneratorComponent.prototype.render).not.to.have.been.called; + }); + + it('should be called when child fails in getSnapshotBeforeUpdate', () => { + let receiver; + class ErrorReceiverComponent extends Component { + constructor() { + super(); + receiver = this; + } + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + class ErrorGeneratorComponent extends Component { + getSnapshotBeforeUpdate() { + throw "Error!"; + } + render() { + return ; + } + } + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); + receiver.forceUpdate(); + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + }); + + it('should be called when child fails in componentDidUpdate', () => { + let receiver; + class ErrorReceiverComponent extends Component { + constructor() { + super(); + receiver = this; + } + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + class ErrorGeneratorComponent extends Component { + componentDidUpdate() { + throw "Error!"; + } + render() { + return ; + } + } + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); + receiver.forceUpdate(); + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + }); + + it('should be called when child fails in componentWillUpdate', () => { + let receiver; + class ErrorReceiverComponent extends Component { + constructor() { + super(); + receiver = this; + } + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + class ErrorGeneratorComponent extends Component { + componentWillUpdate() { + throw "Error!"; + } + render() { + return ; + } + } + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); + receiver.forceUpdate(); + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + }); + + it('should be called when child fails in componentWillReceiveProps', () => { + let receiver; + class ErrorReceiverComponent extends Component { + constructor() { + super(); + this.state = { foo: "bar" }; + receiver = this; + } + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : }
; + } + } + class ErrorGeneratorComponent extends Component { + componentWillReceiveProps() { + throw "Error!"; + } + render() { + return ; + } + } + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); + receiver.setState({ foo: "baz" }); + receiver.forceUpdate(); + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + }); + + it('should be called when child fails in shouldComponentUpdate', () => { + let receiver; + class ErrorReceiverComponent extends Component { + constructor() { + super(); + this.state = { foo: "bar" }; + receiver = this; + } + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : }
; + } + } + class ErrorGeneratorComponent extends Component { + shouldComponentUpdate() { + throw "Error!"; + } + render() { + return ; + } + } + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); + receiver.setState({ foo: "baz" }); + receiver.forceUpdate(); + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + }); + it('should be called when functional child fails', () => { class ErrorReceiverComponent extends Component { componentDidCatch(error) { From aa4876988816162d0369e8d3f7a437600ddad111 Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Tue, 29 May 2018 21:01:18 -0400 Subject: [PATCH 09/19] Use a new _caught property to determine if a component has just caught an error and is re-rendering; Don't let components handle their own errors (by properly passing to the ancestor component immediately in all scenarios); Pass errors to a parent error boundary when an error boundary or its children repeatedly throw --- config/properties.json | 3 +- src/component.js | 1 + src/constants.js | 3 +- src/vdom/component.js | 137 +++++++++++++++++++------------------- test/browser/lifecycle.js | 75 +++++++++++++++++++-- 5 files changed, 140 insertions(+), 79 deletions(-) diff --git a/config/properties.json b/config/properties.json index 7b218e22d8..b6d0403377 100644 --- a/config/properties.json +++ b/config/properties.json @@ -1,8 +1,9 @@ { "props": { - "cname": 16, + "cname": 17, "props": { "$_dirty": "__d", + "$_caught": "__e", "$_disable": "__x", "$_listeners": "__l", "$_renderCallbacks": "__h", diff --git a/src/component.js b/src/component.js index eff08225f2..2eef71c5f4 100644 --- a/src/component.js +++ b/src/component.js @@ -16,6 +16,7 @@ import { enqueueRender } from './render-queue'; */ export function Component(props, context) { this._dirty = true; + this._caught = false; /** * @public diff --git a/src/constants.js b/src/constants.js index ceec02bbe6..23be941052 100644 --- a/src/constants.js +++ b/src/constants.js @@ -3,8 +3,7 @@ export const NO_RENDER = 0; export const SYNC_RENDER = 1; export const FORCE_RENDER = 2; -export const ASYNC_RENDER = 4; -export const ERROR_RENDER = SYNC_RENDER | 8; // Like sync render, but throws any errors that occur +export const ASYNC_RENDER = 3; export const ATTR_KEY = '__preactattr_'; diff --git a/src/vdom/component.js b/src/vdom/component.js index 9a1bf98943..66b9f6fd0c 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -1,4 +1,4 @@ -import { SYNC_RENDER, NO_RENDER, FORCE_RENDER, ASYNC_RENDER, ERROR_RENDER, ATTR_KEY } from '../constants'; +import { SYNC_RENDER, NO_RENDER, FORCE_RENDER, ASYNC_RENDER, ATTR_KEY } from '../constants'; import options from '../options'; import { extend } from '../util'; import { enqueueRender } from '../render-queue'; @@ -39,8 +39,8 @@ export function setComponentProps(component, props, opts, context, mountAll) { component._disable = false; if (opts!==NO_RENDER) { - if ((opts&SYNC_RENDER) || options.syncComponentUpdates!==false || !component.base) { - renderComponent(component, opts|SYNC_RENDER, mountAll); + if (opts===SYNC_RENDER || options.syncComponentUpdates!==false || !component.base) { + renderComponent(component, SYNC_RENDER, mountAll); } else { enqueueRender(component); @@ -52,9 +52,11 @@ export function setComponentProps(component, props, opts, context, mountAll) { export function catchErrorInComponent(error, component) { for (; component; component = component._ancestorComponent) { - if (component.componentDidCatch) { + if (component.componentDidCatch && !component._caught) { try { - return component.componentDidCatch(error); + component.componentDidCatch(error); + component._caught = true; + return; } catch (e) { error = e; } @@ -116,70 +118,74 @@ function renderComponentAttempt(component, opts, mountAll, isChild) { snapshot = component.getSnapshotBeforeUpdate(previousProps, previousState); } - let childComponent = rendered && rendered.nodeName, - toUnmount, base; + try { + let childComponent = rendered && rendered.nodeName, + toUnmount, base; - if (typeof childComponent==='function') { - // set up high order component link + if (typeof childComponent==='function') { + // set up high order component link - let childProps = getNodeProps(rendered); - inst = initialChildComponent; + let childProps = getNodeProps(rendered); + inst = initialChildComponent; - if (inst && inst.constructor===childComponent && childProps.key==inst.__key) { - setComponentProps(inst, childProps, opts|SYNC_RENDER, context, false); - } - else { - toUnmount = inst; + 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); + } - 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, opts|SYNC_RENDER, mountAll, true); + base = inst.base; } + else { + cbase = initialBase; - base = inst.base; - } - else { - cbase = initialBase; - - // destroy high order component link - toUnmount = initialChildComponent; - if (toUnmount) { - cbase = component._component = null; - } + // destroy high order component link + toUnmount = initialChildComponent; + if (toUnmount) { + cbase = component._component = null; + } - if (initialBase || (opts&SYNC_RENDER)) { - if (cbase) cbase._component = null; - base = diff(cbase, rendered, context, mountAll || !isUpdate, initialBase && initialBase.parentNode, component); + if (initialBase || opts===SYNC_RENDER) { + if (cbase) cbase._component = null; + base = diff(cbase, rendered, context, mountAll || !isUpdate, initialBase && initialBase.parentNode, component); + } } - } - if (initialBase && base!==initialBase && inst!==initialChildComponent) { - let baseParent = initialBase.parentNode; - if (baseParent && base!==baseParent) { - baseParent.replaceChild(base, initialBase); + 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); + if (!toUnmount) { + initialBase._component = null; + recollectNodeTree(initialBase, false); + } } } - } - if (toUnmount) { - unmountComponent(toUnmount); - } + if (toUnmount) { + unmountComponent(toUnmount); + } - component.base = base; - if (base && !isChild) { - let componentRef = component, - t = component; - while ((t=t._parentComponent)) { - (componentRef = t).base = base; + 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; } - base._component = componentRef; - base._componentConstructor = componentRef.constructor; + } catch (e) { + catchErrorInComponent(e, component); } } @@ -217,24 +223,15 @@ function renderComponentAttempt(component, opts, mountAll, isChild) { export function renderComponent(component, opts, mountAll, isChild) { if (component._disable) return; - if (opts !== ERROR_RENDER) { - try { + try { + renderComponentAttempt(component, opts, mountAll, isChild); + if (component._caught) { + // Attempt rendering again, but let any further errors pass through to ancestor renderComponentAttempt(component, opts, mountAll, isChild); - } catch (e) { - catchErrorInComponent(e, component); + component._caught = false; } - - if (component._dirty && component.componentDidCatch) { - // Error was caught, rerender with errors in children propagated via throw - try { - renderComponentAttempt(component, ERROR_RENDER, mountAll, isChild); - } catch (e) { - catchErrorInComponent(e, component._ancestorComponent); - } - } - } else { - // Asked by a call further up the stack to propagate errors via throw - renderComponentAttempt(component, opts, mountAll, isChild); + } catch (e) { + catchErrorInComponent(e, component._ancestorComponent); } } diff --git a/test/browser/lifecycle.js b/test/browser/lifecycle.js index 699ca83b7b..bb8fb35be4 100644 --- a/test/browser/lifecycle.js +++ b/test/browser/lifecycle.js @@ -1079,7 +1079,7 @@ describe('Lifecycle methods', () => { sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); render(, scratch); receiver.setState({ foo: "baz" }); - receiver.forceUpdate(); + rerender(); expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; }); @@ -1109,7 +1109,7 @@ describe('Lifecycle methods', () => { sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); render(, scratch); receiver.setState({ foo: "baz" }); - receiver.forceUpdate(); + rerender(); expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; }); @@ -1163,22 +1163,85 @@ describe('Lifecycle methods', () => { } class ErrorAdapterComponent extends Component { componentDidCatch(error) { - throw "Adapted " + String(error); + throw new Error("Adapted " + String(error && "message" in error ? error.message : error)); } render() { return
{this.props.children}
; } } function ErrorGeneratorComponent() { - throw "Error!"; + throw new Error("Error!"); + } + sinon.spy(ErrorAdapterComponent.prototype, 'componentDidCatch'); + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); + expect(ErrorAdapterComponent.prototype.componentDidCatch).to.have.been.called; + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + rerender(); + expect(scratch).to.have.property('textContent', 'Error: Adapted Error!'); + }); + + it('should bubble on repeated errors', () => { + class ErrorReceiverComponent extends Component { + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + class ErrorAdapterComponent extends Component { + componentDidCatch(error) { + // Try to handle the error + this.setState({ error }); + } + render() { + // But fail at doing so + if (this.state.error) { + throw this.state.error; + } + return
{this.props.children}
; + } + } + function ErrorGeneratorComponent() { + throw new Error("Error!"); } sinon.spy(ErrorAdapterComponent.prototype, 'componentDidCatch'); sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); render(, scratch); + rerender(); expect(ErrorAdapterComponent.prototype.componentDidCatch).to.have.been.called; expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + expect(scratch).to.have.property('textContent', 'Error: Error!'); + }); + + it('should bubble on ignored errors', () => { + class ErrorReceiverComponent extends Component { + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + class ErrorAdapterComponent extends Component { + componentDidCatch(error) { + // Ignore the error + } + render() { + return
{this.props.children}
; + } + } + function ErrorGeneratorComponent() { + throw new Error("Error!"); + } + sinon.spy(ErrorAdapterComponent.prototype, 'componentDidCatch'); + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); rerender(); - expect(scratch).to.have.property('textContent', 'Adapted Error!'); + expect(ErrorAdapterComponent.prototype.componentDidCatch).to.have.been.called; + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + expect(scratch).to.have.property('textContent', 'Error: Error!'); }); it('should be called through non-component parent elements', () => { @@ -1193,7 +1256,7 @@ describe('Lifecycle methods', () => { class ErrorGeneratorComponent extends Component { constructor(props, context) { super(props, context); - throw "Error!"; + throw new Error("Error!"); } } sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); From a2b4c8c411b6e81b4602212229b6e2eb7509b340 Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Wed, 30 May 2018 15:28:47 -0400 Subject: [PATCH 10/19] Flush mount events before dispatching errors --- src/vdom/component.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/vdom/component.js b/src/vdom/component.js index 66b9f6fd0c..558c90d684 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -51,6 +51,7 @@ export function setComponentProps(component, props, opts, context, mountAll) { } export function catchErrorInComponent(error, component) { + flushMounts(); for (; component; component = component._ancestorComponent) { if (component.componentDidCatch && !component._caught) { try { From 4320ff96ea657d41413ba8a903acb653d3e964a7 Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Wed, 30 May 2018 15:30:49 -0400 Subject: [PATCH 11/19] Send componentDidMount to a newly created parent only once when a child component fails to render and the parent catches its error in componentDidCatch --- src/vdom/component.js | 61 ++++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/src/vdom/component.js b/src/vdom/component.js index 558c90d684..5e89d4633b 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -79,7 +79,8 @@ function renderComponentAttempt(component, opts, mountAll, isChild) { initialChildComponent = component._component, skip = false, snapshot = previousContext, - rendered, inst, cbase; + rendered, inst, cbase, + exception; if (component.constructor.getDerivedStateFromProps) { previousState = extend({}, previousState); @@ -119,10 +120,9 @@ function renderComponentAttempt(component, opts, mountAll, isChild) { snapshot = component.getSnapshotBeforeUpdate(previousProps, previousState); } + let childComponent = rendered && rendered.nodeName, + toUnmount, base; try { - let childComponent = rendered && rendered.nodeName, - toUnmount, base; - if (typeof childComponent==='function') { // set up high order component link @@ -158,35 +158,38 @@ function renderComponentAttempt(component, opts, mountAll, isChild) { base = diff(cbase, rendered, context, mountAll || !isUpdate, initialBase && initialBase.parentNode, component); } } + } catch (e) { + exception = e; + if (!base) { + base = initialBase || document.createTextNode(""); + } + } - if (initialBase && base!==initialBase && inst!==initialChildComponent) { - let baseParent = initialBase.parentNode; - if (baseParent && base!==baseParent) { - baseParent.replaceChild(base, initialBase); + 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); - } + if (!toUnmount) { + initialBase._component = null; + recollectNodeTree(initialBase, false); } } + } - if (toUnmount) { - unmountComponent(toUnmount); - } + if (toUnmount && toUnmount.base) { + unmountComponent(toUnmount); + } - 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; + component.base = base; + if (base && !isChild) { + let componentRef = component, + t = component; + while ((t=t._parentComponent)) { + (componentRef = t).base = base; } - } catch (e) { - catchErrorInComponent(e, component); + base._component = componentRef; + base._componentConstructor = componentRef.constructor; } } @@ -207,6 +210,10 @@ function renderComponentAttempt(component, opts, mountAll, isChild) { while (component._renderCallbacks.length) component._renderCallbacks.pop().call(component); + if (typeof exception !== "undefined") { + catchErrorInComponent(exception, component); + } + if (!diffLevel && !isChild) flushMounts(); } @@ -228,7 +235,7 @@ export function renderComponent(component, opts, mountAll, isChild) { renderComponentAttempt(component, opts, mountAll, isChild); if (component._caught) { // Attempt rendering again, but let any further errors pass through to ancestor - renderComponentAttempt(component, opts, mountAll, isChild); + renderComponentAttempt(component, opts, false, isChild); component._caught = false; } } catch (e) { From 4338a7c012be9ee3944499317cc142c132924372 Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Wed, 30 May 2018 15:31:56 -0400 Subject: [PATCH 12/19] Ensure that componentWillUnmount is sent only once, even in error paths --- src/vdom/component.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/vdom/component.js b/src/vdom/component.js index 5e89d4633b..1493f287e4 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -301,10 +301,11 @@ export function buildComponentFromVNode(dom, vnode, context, mountAll, ancestorC export function unmountComponent(component) { if (options.beforeUnmount) options.beforeUnmount(component); - let base = component.base; - + if (component._disable) return; component._disable = true; + let base = component.base; + if (component.componentWillUnmount) { try { component.componentWillUnmount(); From 74fddde493e747b3e78d4bf9bbdfd1c96e2d1047 Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Wed, 30 May 2018 15:32:22 -0400 Subject: [PATCH 13/19] Remove error parameter in componentDidCatch (which triggered linter warning) --- test/browser/lifecycle.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/browser/lifecycle.js b/test/browser/lifecycle.js index bb8fb35be4..b10765dc7e 100644 --- a/test/browser/lifecycle.js +++ b/test/browser/lifecycle.js @@ -1225,7 +1225,7 @@ describe('Lifecycle methods', () => { } } class ErrorAdapterComponent extends Component { - componentDidCatch(error) { + componentDidCatch() { // Ignore the error } render() { From 8b23777d6195b4d907559463d79a1fb5895c94da Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Wed, 30 May 2018 16:20:25 -0400 Subject: [PATCH 14/19] Enqueue renders triggered from componentDidCatch instead of invoking further up the stack during initial render --- src/vdom/component.js | 253 +++++++++++++++++++++--------------------- 1 file changed, 125 insertions(+), 128 deletions(-) diff --git a/src/vdom/component.js b/src/vdom/component.js index ca59b47a1c..9232f003e4 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -60,6 +60,7 @@ export function catchErrorInComponent(error, component) { try { component.componentDidCatch(error); component._caught = true; + enqueueRender(component); return; } catch (e) { error = e; @@ -69,7 +70,20 @@ export function catchErrorInComponent(error, component) { throw error; } -function irenderComponent(component, renderMode, mountAll, isChild) { + + +/** + * Render a Component, triggering necessary lifecycle events and taking + * High-Order Components into account. + * @param {import('../component').Component} component The component to render + * @param {number} [renderMode] render mode, see constants.js for available options. + * @param {boolean} [mountAll] Whether or not to immediately mount all components + * @param {boolean} [isChild] ? + * @private + */ +export function renderComponent(component, renderMode, mountAll, isChild) { + if (component._disable) return; + let props = component.props, state = component.state, context = component.context, @@ -83,135 +97,144 @@ function irenderComponent(component, renderMode, mountAll, isChild) { skip = false, snapshot = previousContext, rendered, inst, cbase, - exception; + exception, caught = component._caught; - 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); + try { + if (component.constructor.getDerivedStateFromProps) { + previousState = extend({}, previousState); + component.state = extend(state, component.constructor.getDerivedStateFromProps(props, state)); } - component.props = props; - component.state = state; - component.context = context; - } - - component.prevProps = component.prevState = component.prevContext = component.nextBase = null; - component._dirty = false; - 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()); + // 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; } - if (isUpdate && component.getSnapshotBeforeUpdate) { - snapshot = component.getSnapshotBeforeUpdate(previousProps, previousState); - } + component.prevProps = component.prevState = component.prevContext = component.nextBase = null; + component._dirty = false; - let childComponent = rendered && rendered.nodeName, - toUnmount, base; - try { - 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); + } + + 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 { - 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); + 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; + if (!base) { + base = initialBase || document.createTextNode(""); } - - base = inst.base; } - else { - cbase = initialBase; - // destroy high order component link - toUnmount = initialChildComponent; - if (toUnmount) { - cbase = component._component = null; - } + if (initialBase && base!==initialBase && inst!==initialChildComponent) { + let baseParent = initialBase.parentNode; + if (baseParent && base!==baseParent) { + baseParent.replaceChild(base, initialBase); - if (initialBase || renderMode===SYNC_RENDER) { - if (cbase) cbase._component = null; - base = diff(cbase, rendered, context, mountAll || !isUpdate, initialBase && initialBase.parentNode, component); + if (!toUnmount) { + initialBase._component = null; + recollectNodeTree(initialBase, false); + } } } - } catch (e) { - exception = e; - if (!base) { - base = initialBase || document.createTextNode(""); - } - } - if (initialBase && base!==initialBase && inst!==initialChildComponent) { - let baseParent = initialBase.parentNode; - if (baseParent && base!==baseParent) { - baseParent.replaceChild(base, initialBase); + if (toUnmount && toUnmount.base) { + unmountComponent(toUnmount); + } - 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 && toUnmount.base) { - 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 (caught) { + component._caught = false; } - if (options.afterUpdate) options.afterUpdate(component); - } - while (component._renderCallbacks.length) component._renderCallbacks.pop().call(component); + } catch (e) { + catchErrorInComponent(e, component._ancestorComponent); + } if (typeof exception !== "undefined") { catchErrorInComponent(exception, component); @@ -222,32 +245,6 @@ function irenderComponent(component, renderMode, mountAll, isChild) { -/** - * Render a Component, triggering necessary lifecycle events and taking - * High-Order Components into account. - * @param {import('../component').Component} component The component to render - * @param {number} [renderMode] render mode, see constants.js for available options. - * @param {boolean} [mountAll] Whether or not to immediately mount all components - * @param {boolean} [isChild] ? - * @private - */ -export function renderComponent(component, renderMode, mountAll, isChild) { - if (component._disable) return; - - try { - irenderComponent(component, renderMode, mountAll, isChild); - if (component._caught) { - // Attempt rendering again, but let any further errors pass through to ancestor - irenderComponent(component, renderMode, false, isChild); - component._caught = false; - } - } catch (e) { - catchErrorInComponent(e, component._ancestorComponent); - } -} - - - /** * Apply the Component referenced by a VNode to the DOM. * @param {import('../dom').PreactElement} dom The DOM node to mutate From 308d8aa76bd709f0787aa5f1ccba04fd7809c27f Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Wed, 30 May 2018 16:21:33 -0400 Subject: [PATCH 15/19] Fully debounce rendering during lifecycle tests (because error handling can cause rendering to be spread amongst multiple microtasks) --- test/browser/lifecycle.js | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/browser/lifecycle.js b/test/browser/lifecycle.js index b10765dc7e..fe17b570d8 100644 --- a/test/browser/lifecycle.js +++ b/test/browser/lifecycle.js @@ -1,6 +1,16 @@ -import { h, render, rerender, Component } from '../../src/preact'; +import { h, render, options, Component } from '../../src/preact'; /** @jsx h */ +// Wrapper rerender function that fully drains the queue +const renderQueue = []; +options.debounceRendering = (callback) => renderQueue.push(callback); +function rerender() { + let renderCallback; + while (renderCallback = renderQueue.shift()) { + renderCallback(); + } +} + let spyAll = obj => Object.keys(obj).forEach( key => sinon.spy(obj,key) ); const EMPTY_CHILDREN = []; From 30cde7b9130d0b515cb356d7404c924ca97bc5cc Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Wed, 30 May 2018 17:00:50 -0400 Subject: [PATCH 16/19] Avoid infinite rerender loop when top-most error boundary attempts to handle errors of its children, but fails --- src/vdom/component.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/vdom/component.js b/src/vdom/component.js index 9232f003e4..b0b50c89dc 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -97,7 +97,7 @@ export function renderComponent(component, renderMode, mountAll, isChild) { skip = false, snapshot = previousContext, rendered, inst, cbase, - exception, caught = component._caught; + exception, clearCaught = component._caught; try { if (component.constructor.getDerivedStateFromProps) { @@ -178,6 +178,7 @@ export function renderComponent(component, renderMode, mountAll, isChild) { } } catch (e) { exception = e; + clearCaught = false; if (!base) { base = initialBase || document.createTextNode(""); } @@ -228,7 +229,7 @@ export function renderComponent(component, renderMode, mountAll, isChild) { while (component._renderCallbacks.length) component._renderCallbacks.pop().call(component); - if (caught) { + if (clearCaught) { component._caught = false; } From 286e6ea562692ccef141009807961f1f166fcdce Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Tue, 5 Jun 2018 03:15:42 -0400 Subject: [PATCH 17/19] Add test to verify that componentDidCatch errors stop bubbling once caught --- test/browser/lifecycle.js | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/test/browser/lifecycle.js b/test/browser/lifecycle.js index f658eb05c8..8c358167ce 100644 --- a/test/browser/lifecycle.js +++ b/test/browser/lifecycle.js @@ -6,7 +6,7 @@ const renderQueue = []; options.debounceRendering = (callback) => renderQueue.push(callback); function rerender() { let renderCallback; - while (renderCallback = renderQueue.shift()) { + while ((renderCallback = renderQueue.shift())) { renderCallback(); } } @@ -1353,6 +1353,35 @@ describe('Lifecycle methods', () => { expect(scratch).to.have.property('textContent', 'Error: Error!'); }); + it('should not bubble on caught errors', () => { + class TopLevelReceiverComponent extends Component { + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + class ErrorReceiverComponent extends Component { + componentDidCatch(error) { + this.setState({ error }); + } + render() { + return
{this.state.error ? String(this.state.error) : this.props.children}
; + } + } + function ErrorGeneratorComponent() { + throw new Error("Error!"); + } + sinon.spy(TopLevelReceiverComponent.prototype, 'componentDidCatch'); + sinon.spy(ErrorReceiverComponent.prototype, 'componentDidCatch'); + render(, scratch); + rerender(); + expect(TopLevelReceiverComponent.prototype.componentDidCatch).not.to.have.been.called; + expect(ErrorReceiverComponent.prototype.componentDidCatch).to.have.been.called; + expect(scratch).to.have.property('textContent', 'Error: Error!'); + }); + it('should be called through non-component parent elements', () => { class ErrorReceiverComponent extends Component { componentDidCatch(error) { From be4dee53d0d676537c7bd9cb5317a5029fe73c20 Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Tue, 5 Jun 2018 03:29:39 -0400 Subject: [PATCH 18/19] Add JSDoc comments for componentRoot/ancestorComponent parameters --- src/vdom/component-recycler.js | 2 ++ src/vdom/component.js | 2 ++ src/vdom/diff.js | 8 ++++++-- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/vdom/component-recycler.js b/src/vdom/component-recycler.js index a8af6ffdbd..dc1d2dd5d6 100644 --- a/src/vdom/component-recycler.js +++ b/src/vdom/component-recycler.js @@ -26,6 +26,8 @@ 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, ancestorComponent) { diff --git a/src/vdom/component.js b/src/vdom/component.js index b0b50c89dc..d3defedd3a 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -252,6 +252,8 @@ 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 */ diff --git a/src/vdom/diff.js b/src/vdom/diff.js index 6bb4596b2c..93d431355f 100644 --- a/src/vdom/diff.js +++ b/src/vdom/diff.js @@ -45,7 +45,8 @@ export function flushMounts() { * @param {object} context The current context * @param {boolean} mountAll Whether or not to immediately mount all components * @param {Element} parent ? - * @param {import('../component').Component} componentRoot ? + * @param {import('../component').Component} [componentRoot] The nearest ancestor component beneath + * which the diff will occur * @returns {import('../dom').PreactElement} The created/mutated element * @private */ @@ -82,7 +83,8 @@ export function diff(dom, vnode, context, mountAll, parent, componentRoot) { * @param {import('../vnode').VNode} vnode A VNode (with descendants forming a tree) representing the desired DOM structure * @param {object} context The current context * @param {boolean} mountAll Whether or not to immediately mount all components - * @param {import('../component').Component} [componentRoot] ? + * @param {import('../component').Component} [componentRoot] The nearest ancestor component beneath + * which the diff will occur * @private */ function idiff(dom, vnode, context, mountAll, componentRoot) { @@ -188,6 +190,8 @@ function idiff(dom, vnode, context, mountAll, componentRoot) { * @param {boolean} mountAll Whether or not to immediately mount all components * @param {boolean} isHydrating if `true`, consumes externally created elements * similar to hydration + * @param {import('../component').Component} [componentRoot] The nearest ancestor component beneath + * which the diff will occur */ function innerDiffNode(dom, vchildren, context, mountAll, isHydrating, componentRoot) { let originalChildren = dom.childNodes, From a019562109aacf82ce2806dabe13711b9e5fe890 Mon Sep 17 00:00:00 2001 From: Ryan Petrich Date: Tue, 5 Jun 2018 03:32:13 -0400 Subject: [PATCH 19/19] Check if component is disabled before dispatching beforeUnmount hook (so that it can't be inadvertently called on an already unmounted component) --- src/vdom/component.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vdom/component.js b/src/vdom/component.js index d3defedd3a..bce966bd16 100644 --- a/src/vdom/component.js +++ b/src/vdom/component.js @@ -304,11 +304,11 @@ export function buildComponentFromVNode(dom, vnode, context, mountAll, ancestorC * @private */ export function unmountComponent(component) { - if (options.beforeUnmount) options.beforeUnmount(component); - if (component._disable) return; component._disable = true; + if (options.beforeUnmount) options.beforeUnmount(component); + let base = component.base; if (component.componentWillUnmount) {