From 5b28ba5aa0766892a5089a755663746ece52f27d Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Thu, 2 May 2019 18:31:34 +0200 Subject: [PATCH 01/26] Introduce first draft of Suspense and lazy --- compat/src/index.js | 6 +++-- src/index.js | 1 + src/suspense.js | 65 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 2 deletions(-) create mode 100644 src/suspense.js diff --git a/compat/src/index.js b/compat/src/index.js index 252cee6c417..0e8eaf3e35d 100644 --- a/compat/src/index.js +++ b/compat/src/index.js @@ -1,4 +1,4 @@ -import { render as preactRender, cloneElement as preactCloneElement, createRef, h, Component, options, toChildArray, createContext, Fragment } from 'preact'; +import { render as preactRender, cloneElement as preactCloneElement, createRef, h, Component, options, toChildArray, createContext, Fragment, Suspense, lazy } from 'preact'; import * as hooks from 'preact/hooks'; export * from 'preact/hooks'; import { assign } from '../../src/util'; @@ -401,5 +401,7 @@ export default assign({ PureComponent, memo, forwardRef, - unstable_batchedUpdates + unstable_batchedUpdates, + Suspense, + lazy }, hooks); diff --git a/src/index.js b/src/index.js index 8b998b08cdf..be0d546638d 100644 --- a/src/index.js +++ b/src/index.js @@ -4,4 +4,5 @@ export { Component } from './component'; export { cloneElement } from './clone-element'; export { createContext } from './create-context'; export { toChildArray } from './diff/children'; +export { Suspense, lazy } from './suspense'; export { default as options } from './options'; diff --git a/src/suspense.js b/src/suspense.js new file mode 100644 index 00000000000..7fbbeebb3d3 --- /dev/null +++ b/src/suspense.js @@ -0,0 +1,65 @@ +import { Component } from './component'; +import { createElement } from './create-element'; + +// TODO: react warns in dev mode about defaultProps and propTypes not being supported on lazy +// loaded components + +export class Suspense extends Component { + constructor(props) { + // TODO: should we add propTypes in DEV mode? + super(props); + + this.state = { + l: false + }; + } + + componentDidCatch(e) { + if (e && typeof e.then === 'function') { + this.setState({ l: true }); + e.then( + () => { + this.setState({ l: false }); + }, + // TODO: what to do in error case?! + // we could store the error to the state and then throw it during render + // should have a look what react does in these cases... + () => { + this.setState({ l: false }); + } + ); + } + else { + throw e; + } + } + + render() { + return this.state.l ? this.props.fallback : this.props.children; + } +} + +export function lazy(loader) { + let prom; + let component; + let error; + return function Lazy(props) { + if (!prom) { + prom = loader(); + prom.then( + ({ default: c }) => { component = c; }, + e => error = e, + ); + } + + if (error) { + throw error; + } + + if (!component) { + throw prom; + } + + return createElement(component, props); + }; +} From 1d525f1c14af6ad34c74ea7dce74d8125521ddf8 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Thu, 2 May 2019 23:14:43 +0200 Subject: [PATCH 02/26] Add tests, handle missing suspense, handle custom error boundaries --- src/diff/index.js | 30 +++++++- src/suspense.js | 5 ++ test/browser/suspense.test.js | 133 ++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 test/browser/suspense.test.js diff --git a/src/diff/index.js b/src/diff/index.js index 4a0e148d0a2..f1b2bb76742 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -5,6 +5,7 @@ import { diffChildren } from './children'; import { diffProps } from './props'; import { assign, removeNode } from '../util'; import options from '../options'; +import { sym as suspenseSymbol } from '../suspense'; /** * Diff two virtual nodes and apply proper changes to the DOM @@ -362,10 +363,24 @@ function doRender(props, state, context) { * component check for error boundary behaviors */ function catchErrorInComponent(error, component) { + // thrown Promises are meant to suspend... + let isSuspend = typeof error.then === 'function'; + let suspendingComponent = component; + for (; component; component = component._ancestorComponent) { if (!component._processingException) { try { - if (component.constructor.getDerivedStateFromError!=null) { + if (isSuspend) { + // console.log('catchErrorInComponent component[suspenseSymbol]', component[suspenseSymbol]); + if (component[suspenseSymbol] === suspenseSymbol && component.componentDidCatch!=null) { + // console.log('hitting suspense...'); + component.componentDidCatch(error); + } + else { + continue; + } + } + else if (component.constructor.getDerivedStateFromError!=null) { component.setState(component.constructor.getDerivedStateFromError(error)); } else if (component.componentDidCatch!=null) { @@ -378,8 +393,21 @@ function catchErrorInComponent(error, component) { } catch (e) { error = e; + isSuspend = typeof error.then === 'function'; + suspendingComponent = component; } } } + + if (isSuspend && suspendingComponent) { + // TODO: what is the preact way to determine the component name? + const componentName = suspendingComponent.displayName || (suspendingComponent._vnode && suspendingComponent._vnode.type + && suspendingComponent._vnode.type.name); + + return catchErrorInComponent(new Error(`${componentName} suspended while rendering, but no fallback UI was specified. + +Add a component higher in the tree to provide a loading indicator or placeholder to display.`), suspendingComponent); + } + throw error; } diff --git a/src/suspense.js b/src/suspense.js index 7fbbeebb3d3..e854f9db2a0 100644 --- a/src/suspense.js +++ b/src/suspense.js @@ -4,11 +4,16 @@ import { createElement } from './create-element'; // TODO: react warns in dev mode about defaultProps and propTypes not being supported on lazy // loaded components +export const sym = Symbol.for('Suspense'); + export class Suspense extends Component { constructor(props) { // TODO: should we add propTypes in DEV mode? super(props); + // mark this component as a handler of suspension (thrown Promises) + this[sym] = sym; + this.state = { l: false }; diff --git a/test/browser/suspense.test.js b/test/browser/suspense.test.js new file mode 100644 index 00000000000..725a1075936 --- /dev/null +++ b/test/browser/suspense.test.js @@ -0,0 +1,133 @@ +/*eslint-env browser, mocha */ +import { setupRerender } from 'preact/test-utils'; +import { expect } from 'chai'; +import { createElement as h, render, Component, Suspense, lazy } from '../../src/index'; +import { setupScratch, teardown } from '../_util/helpers'; + +class LazyComp extends Component { + render() { + return
Hello Lazy
; + } +} + +class CustomSuspense extends Component { + constructor(props) { + super(props); + this.state = { done: false }; + } + render() { + if (!this.state.done) { + throw new Promise((res) => { + setTimeout(() => { + this.setState({ done: true }); + res(); + }, 0); + }); + } + + return ( +
+ Hello CustomSuspense +
+ ); + } +} + +class Catcher extends Component { + constructor(props) { + super(props); + this.state = { error: null }; + } + + componentDidCatch(e) { + this.setState({ error: e }); + } + + render() { + return this.state.error ? ( +
+ Catcher did catch: {this.state.error.message} +
+ ) : this.props.children; + } +} + +const Lazy = lazy(() => new Promise((res) => { + setTimeout(() => { + res({ default: LazyComp }); + }, 0); +})); + +/** @jsx h */ + +describe('suspense', () => { + let scratch, rerender; + + beforeEach(() => { + scratch = setupScratch(); + rerender = setupRerender(); + }); + + afterEach(() => { + teardown(scratch); + }); + + it('should suspend when using lazy', () => { + render(Suspended...}> + + , scratch); + rerender(); + expect(scratch.innerHTML).to.eql( + `
Suspended...
` + ); + }); + + it('should suspend when a promise is throw', () => { + render(Suspended...}> + + , scratch); + rerender(); + expect(scratch.innerHTML).to.eql( + `
Suspended...
` + ); + }); + + it('should suspend with custom error boundary', () => { + render(Suspended...}> + + + + , scratch); + rerender(); + expect(scratch.innerHTML).to.eql( + `
Suspended...
` + ); + }); + + it('should only suspend the most inner Suspend', () => { + render(Suspended... 1}> + Not suspended... + Suspended... 2}> + + + + + , scratch); + rerender(); + expect(scratch.innerHTML).to.eql( + `Not suspended...
Suspended... 2
` + ); + }); + + it('should throw when missing Suspense', () => { + render( + + , scratch); + rerender(); + expect(scratch.innerHTML).to.eql( + `
Catcher did catch: CustomSuspense suspended while rendering, but no fallback UI was specified. + +Add a <Suspense fallback=...> component higher in the tree to provide a loading indicator or placeholder to display.
` + ); + }); +}); \ No newline at end of file From eb9c7dbb4c2a0f8f242384439c515c4f949990f8 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Thu, 2 May 2019 23:45:46 +0200 Subject: [PATCH 03/26] Work on sunspense... --- src/suspense.js | 4 +- test/browser/suspense.test.js | 110 ++++++++++++++++++++++++++++++++-- 2 files changed, 107 insertions(+), 7 deletions(-) diff --git a/src/suspense.js b/src/suspense.js index e854f9db2a0..5b94385198c 100644 --- a/src/suspense.js +++ b/src/suspense.js @@ -26,9 +26,7 @@ export class Suspense extends Component { () => { this.setState({ l: false }); }, - // TODO: what to do in error case?! - // we could store the error to the state and then throw it during render - // should have a look what react does in these cases... + // Suspense ignores errors thrown in Promises as this should be handled by user land code () => { this.setState({ l: false }); } diff --git a/test/browser/suspense.test.js b/test/browser/suspense.test.js index 725a1075936..d8d42bd1993 100644 --- a/test/browser/suspense.test.js +++ b/test/browser/suspense.test.js @@ -18,10 +18,10 @@ class CustomSuspense extends Component { render() { if (!this.state.done) { throw new Promise((res) => { - setTimeout(() => { + setImmediate(() => { this.setState({ done: true }); res(); - }, 0); + }); }); } @@ -33,6 +33,29 @@ class CustomSuspense extends Component { } } +class ThrowingSuspense extends Component { + constructor(props) { + super(props); + this.state = { done: false }; + } + render() { + if (!this.state.done) { + throw new Promise((res, rej) => { + setImmediate(() => { + this.setState({ done: true }); + rej(new Error('I\'m a throwing Suspense!')); + }); + }); + } + + return ( +
+ Hello ThrowingSuspense +
+ ); + } +} + class Catcher extends Component { constructor(props) { super(props); @@ -53,13 +76,26 @@ class Catcher extends Component { } const Lazy = lazy(() => new Promise((res) => { - setTimeout(() => { + setImmediate(() => { res({ default: LazyComp }); - }, 0); + }); +})); + +const ThrowingLazy = lazy(() => new Promise((res, rej) => { + setImmediate(() => { + rej(new Error('Thrown in lazy\'s loader...')); + }); })); /** @jsx h */ +function tick(fn) { + return new Promise((res) => { + setTimeout(res, 10); + }) + .then(fn); +} + describe('suspense', () => { let scratch, rerender; @@ -80,6 +116,13 @@ describe('suspense', () => { expect(scratch.innerHTML).to.eql( `
Suspended...
` ); + + return tick(() => { + rerender(); + expect(scratch.innerHTML).to.eql( + `
Hello Lazy
` + ); + }); }); it('should suspend when a promise is throw', () => { @@ -90,6 +133,13 @@ describe('suspense', () => { expect(scratch.innerHTML).to.eql( `
Suspended...
` ); + + return tick(() => { + rerender(); + expect(scratch.innerHTML).to.eql( + `
Hello CustomSuspense...
` + ); + }); }); it('should suspend with custom error boundary', () => { @@ -102,6 +152,32 @@ describe('suspense', () => { expect(scratch.innerHTML).to.eql( `
Suspended...
` ); + + return tick(() => { + rerender(); + expect(scratch.innerHTML).to.eql( + `
Hello CustomSuspense...
` + ); + }); + }); + + it('should support throwing suspense', () => { + render(Suspended...}> + + + + , scratch); + rerender(); + expect(scratch.innerHTML).to.eql( + `
Suspended...
` + ); + + return tick(() => { + rerender(); + expect(scratch.innerHTML).to.eql( + `
Hello ThrowingSuspense...
` + ); + }); }); it('should only suspend the most inner Suspend', () => { @@ -117,6 +193,13 @@ describe('suspense', () => { expect(scratch.innerHTML).to.eql( `Not suspended...
Suspended... 2
` ); + + return tick(() => { + rerender(); + expect(scratch.innerHTML).to.eql( + `Not suspended...
Hello CustomSuspend
` + ); + }); }); it('should throw when missing Suspense', () => { @@ -130,4 +213,23 @@ describe('suspense', () => { Add a <Suspense fallback=...> component higher in the tree to provide a loading indicator or placeholder to display.` ); }); + + it('should throw when lazy\'s loader throws', () => { + render(Suspended...}> + + + + , scratch); + rerender(); + expect(scratch.innerHTML).to.eql( + `
Suspended...
` + ); + + return tick(() => { + rerender(); + expect(scratch.innerHTML).to.eql( + `
Hello ThrowingSuspense...
` + ); + }); + }); }); \ No newline at end of file From 104649cbc830bb739125942fbe2680756333852d Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Fri, 3 May 2019 18:13:01 +0200 Subject: [PATCH 04/26] Make tests work --- src/diff/index.js | 34 ++++++++++++++++++++++------- src/suspense.js | 17 ++++++--------- test/browser/suspense.test.js | 40 +++++++++++++++++++++++------------ 3 files changed, 59 insertions(+), 32 deletions(-) diff --git a/src/diff/index.js b/src/diff/index.js index f1b2bb76742..e1b2c3fb45b 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -27,7 +27,19 @@ import { sym as suspenseSymbol } from '../suspense'; export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChildren, mounts, ancestorComponent, force, oldDom) { // If the previous type doesn't match the new type we drop the whole subtree if (oldVNode==null || newVNode==null || oldVNode.type!==newVNode.type || oldVNode.key!==newVNode.key) { - if (oldVNode!=null) unmount(oldVNode, ancestorComponent); + if (oldVNode!=null) { + const ancestor = oldVNode._component && oldVNode._component._ancestorComponent; + if (ancestor && ancestor[suspenseSymbol]) { + ancestor._vnode.suspendedContent = oldVNode; + // TODO: check whether parent was already removed from DOM... + if (oldVNode._dom) { + removeNode(oldVNode._dom); + } + } + else { + unmount(oldVNode, ancestorComponent); + } + } if (newVNode==null) return null; oldVNode = EMPTY_OBJ; } @@ -141,6 +153,11 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi let prev = c._prevVNode || null; c._dirty = false; + + if (c[suspenseSymbol] === suspenseSymbol && c._vnode.suspendedContent) { + if (prev) { unmount(prev); } + prev = c._vnode.suspendedContent; + } let vnode = c._prevVNode = coerceToVNode(c.render(c.props, c.state, c.context)); if (c.getChildContext!=null) { @@ -152,6 +169,7 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi } c._depth = ancestorComponent ? (ancestorComponent._depth || 0) + 1 : 0; + // this creates a new WrapperOne / new CustomSuspense c.base = newVNode._dom = diff(parentDom, vnode, prev, context, isSvg, excessDomChildren, mounts, c, null, oldDom); if (vnode!=null) { @@ -399,14 +417,14 @@ function catchErrorInComponent(error, component) { } } - if (isSuspend && suspendingComponent) { - // TODO: what is the preact way to determine the component name? - const componentName = suspendingComponent.displayName || (suspendingComponent._vnode && suspendingComponent._vnode.type - && suspendingComponent._vnode.type.name); - - return catchErrorInComponent(new Error(`${componentName} suspended while rendering, but no fallback UI was specified. + // TODO: Add a react-like error message to preact/debug + /* + [componentName] suspended while rendering, but no fallback UI was specified. -Add a component higher in the tree to provide a loading indicator or placeholder to display.`), suspendingComponent); + Add a component higher in the tree to provide a loading indicator or placeholder to display. + */ + if (isSuspend) { + return catchErrorInComponent(new Error('Missing Suspense'), suspendingComponent); } throw error; diff --git a/src/suspense.js b/src/suspense.js index 5b94385198c..b057ee9bc45 100644 --- a/src/suspense.js +++ b/src/suspense.js @@ -4,7 +4,9 @@ import { createElement } from './create-element'; // TODO: react warns in dev mode about defaultProps and propTypes not being supported on lazy // loaded components -export const sym = Symbol.for('Suspense'); +export const sym = '_s'; + +export const Suspense2 = 'suspense'; export class Suspense extends Component { constructor(props) { @@ -22,15 +24,10 @@ export class Suspense extends Component { componentDidCatch(e) { if (e && typeof e.then === 'function') { this.setState({ l: true }); - e.then( - () => { - this.setState({ l: false }); - }, - // Suspense ignores errors thrown in Promises as this should be handled by user land code - () => { - this.setState({ l: false }); - } - ); + const cb = () => { this.setState({ l: false }); }; + + // Suspense ignores errors thrown in Promises as this should be handled by user land code + e.then(cb, cb); } else { throw e; diff --git a/test/browser/suspense.test.js b/test/browser/suspense.test.js index d8d42bd1993..8ade6d6d078 100644 --- a/test/browser/suspense.test.js +++ b/test/browser/suspense.test.js @@ -4,6 +4,11 @@ import { expect } from 'chai'; import { createElement as h, render, Component, Suspense, lazy } from '../../src/index'; import { setupScratch, teardown } from '../_util/helpers'; +function schedule(cb) { + setImmediate(cb); + // setTimeout(cb, 0); +} + class LazyComp extends Component { render() { return
Hello Lazy
; @@ -18,7 +23,7 @@ class CustomSuspense extends Component { render() { if (!this.state.done) { throw new Promise((res) => { - setImmediate(() => { + schedule(() => { this.setState({ done: true }); res(); }); @@ -41,7 +46,7 @@ class ThrowingSuspense extends Component { render() { if (!this.state.done) { throw new Promise((res, rej) => { - setImmediate(() => { + schedule(() => { this.setState({ done: true }); rej(new Error('I\'m a throwing Suspense!')); }); @@ -76,17 +81,23 @@ class Catcher extends Component { } const Lazy = lazy(() => new Promise((res) => { - setImmediate(() => { + schedule(() => { res({ default: LazyComp }); }); })); const ThrowingLazy = lazy(() => new Promise((res, rej) => { - setImmediate(() => { + schedule(() => { rej(new Error('Thrown in lazy\'s loader...')); }); })); +class WrapperOne extends Component { + render() { + return this.props.children; + } +} + /** @jsx h */ function tick(fn) { @@ -127,8 +138,11 @@ describe('suspense', () => { it('should suspend when a promise is throw', () => { render(Suspended...}> - + + + , scratch); + // TODO: why a rerender needed here. Will this even work in the browser?! rerender(); expect(scratch.innerHTML).to.eql( `
Suspended...
` @@ -137,7 +151,7 @@ describe('suspense', () => { return tick(() => { rerender(); expect(scratch.innerHTML).to.eql( - `
Hello CustomSuspense...
` + `
Hello CustomSuspense
` ); }); }); @@ -156,7 +170,7 @@ describe('suspense', () => { return tick(() => { rerender(); expect(scratch.innerHTML).to.eql( - `
Hello CustomSuspense...
` + `
Hello CustomSuspense
` ); }); }); @@ -164,7 +178,7 @@ describe('suspense', () => { it('should support throwing suspense', () => { render(Suspended...}> - + , scratch); rerender(); @@ -175,7 +189,7 @@ describe('suspense', () => { return tick(() => { rerender(); expect(scratch.innerHTML).to.eql( - `
Hello ThrowingSuspense...
` + `
Hello ThrowingSuspense
` ); }); }); @@ -197,7 +211,7 @@ describe('suspense', () => { return tick(() => { rerender(); expect(scratch.innerHTML).to.eql( - `Not suspended...
Hello CustomSuspend
` + `Not suspended...
Hello CustomSuspense
` ); }); }); @@ -208,9 +222,7 @@ describe('suspense', () => { , scratch); rerender(); expect(scratch.innerHTML).to.eql( - `
Catcher did catch: CustomSuspense suspended while rendering, but no fallback UI was specified. - -Add a <Suspense fallback=...> component higher in the tree to provide a loading indicator or placeholder to display.
` + `
Catcher did catch: Missing Suspense
` ); }); @@ -228,7 +240,7 @@ Add a <Suspense fallback=...> component higher in the tree to provide a lo return tick(() => { rerender(); expect(scratch.innerHTML).to.eql( - `
Hello ThrowingSuspense...
` + `
Catcher did catch: Thrown in lazy's loader...
` ); }); }); From b8bb86ef1aaf8c881cad0a718110c7eb320e29b0 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Fri, 3 May 2019 18:14:05 +0200 Subject: [PATCH 05/26] Replace setImmediate with setTimeout --- test/browser/suspense.test.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/browser/suspense.test.js b/test/browser/suspense.test.js index 8ade6d6d078..f20e4d5152c 100644 --- a/test/browser/suspense.test.js +++ b/test/browser/suspense.test.js @@ -5,8 +5,7 @@ import { createElement as h, render, Component, Suspense, lazy } from '../../src import { setupScratch, teardown } from '../_util/helpers'; function schedule(cb) { - setImmediate(cb); - // setTimeout(cb, 0); + setTimeout(cb, 0); } class LazyComp extends Component { From 503653612de958ed64515e931a6a4a17b2cabadb Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sat, 4 May 2019 12:24:24 +0200 Subject: [PATCH 06/26] Make suspense work like it does in React --- src/diff/index.js | 30 +--- src/suspense.js | 31 +--- test/browser/lifecycle.test.js | 11 +- test/browser/suspense.test.js | 297 ++++++++++++++++++++++----------- 4 files changed, 222 insertions(+), 147 deletions(-) diff --git a/src/diff/index.js b/src/diff/index.js index e1b2c3fb45b..7dab6ff623c 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -5,7 +5,6 @@ import { diffChildren } from './children'; import { diffProps } from './props'; import { assign, removeNode } from '../util'; import options from '../options'; -import { sym as suspenseSymbol } from '../suspense'; /** * Diff two virtual nodes and apply proper changes to the DOM @@ -27,19 +26,7 @@ import { sym as suspenseSymbol } from '../suspense'; export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChildren, mounts, ancestorComponent, force, oldDom) { // If the previous type doesn't match the new type we drop the whole subtree if (oldVNode==null || newVNode==null || oldVNode.type!==newVNode.type || oldVNode.key!==newVNode.key) { - if (oldVNode!=null) { - const ancestor = oldVNode._component && oldVNode._component._ancestorComponent; - if (ancestor && ancestor[suspenseSymbol]) { - ancestor._vnode.suspendedContent = oldVNode; - // TODO: check whether parent was already removed from DOM... - if (oldVNode._dom) { - removeNode(oldVNode._dom); - } - } - else { - unmount(oldVNode, ancestorComponent); - } - } + if (oldVNode!=null) unmount(oldVNode, ancestorComponent); if (newVNode==null) return null; oldVNode = EMPTY_OBJ; } @@ -56,6 +43,9 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi try { outer: if (oldVNode.type===Fragment || newType===Fragment) { + // Passing the ancestorComponent here is needed for catchErrorInComponent to properly traverse upwards through fragments + // to find a parent Suspense + c = { _ancestorComponent: ancestorComponent }; diffChildren(parentDom, newVNode, oldVNode, context, isSvg, excessDomChildren, mounts, c, oldDom); // Mark dom as empty in case `_children` is any empty array. If it isn't @@ -153,11 +143,6 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi let prev = c._prevVNode || null; c._dirty = false; - - if (c[suspenseSymbol] === suspenseSymbol && c._vnode.suspendedContent) { - if (prev) { unmount(prev); } - prev = c._vnode.suspendedContent; - } let vnode = c._prevVNode = coerceToVNode(c.render(c.props, c.state, c.context)); if (c.getChildContext!=null) { @@ -169,7 +154,6 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi } c._depth = ancestorComponent ? (ancestorComponent._depth || 0) + 1 : 0; - // this creates a new WrapperOne / new CustomSuspense c.base = newVNode._dom = diff(parentDom, vnode, prev, context, isSvg, excessDomChildren, mounts, c, null, oldDom); if (vnode!=null) { @@ -389,10 +373,8 @@ function catchErrorInComponent(error, component) { if (!component._processingException) { try { if (isSuspend) { - // console.log('catchErrorInComponent component[suspenseSymbol]', component[suspenseSymbol]); - if (component[suspenseSymbol] === suspenseSymbol && component.componentDidCatch!=null) { - // console.log('hitting suspense...'); - component.componentDidCatch(error); + if (component.__s) { + component.__s(error); } else { continue; diff --git a/src/suspense.js b/src/suspense.js index b057ee9bc45..c1dfb4cb6f0 100644 --- a/src/suspense.js +++ b/src/suspense.js @@ -1,30 +1,17 @@ import { Component } from './component'; import { createElement } from './create-element'; -// TODO: react warns in dev mode about defaultProps and propTypes not being supported on lazy -// loaded components - -export const sym = '_s'; - -export const Suspense2 = 'suspense'; - export class Suspense extends Component { constructor(props) { - // TODO: should we add propTypes in DEV mode? super(props); - // mark this component as a handler of suspension (thrown Promises) - this[sym] = sym; - - this.state = { - l: false - }; + this.state = {}; } - componentDidCatch(e) { - if (e && typeof e.then === 'function') { - this.setState({ l: true }); - const cb = () => { this.setState({ l: false }); }; + __s(e) { + if (typeof e.then == 'function') { + this.setState({ l: 1 }); + const cb = () => { this.setState({ l: 0 }); }; // Suspense ignores errors thrown in Promises as this should be handled by user land code e.then(cb, cb); @@ -34,8 +21,8 @@ export class Suspense extends Component { } } - render() { - return this.state.l ? this.props.fallback : this.props.children; + render(props, state) { + return state.l ? props.fallback : props.children; } } @@ -43,7 +30,7 @@ export function lazy(loader) { let prom; let component; let error; - return function Lazy(props) { + return function L(props) { if (!prom) { prom = loader(); prom.then( @@ -62,4 +49,4 @@ export function lazy(loader) { return createElement(component, props); }; -} +} \ No newline at end of file diff --git a/test/browser/lifecycle.test.js b/test/browser/lifecycle.test.js index 22082ab2e1b..54ebd188010 100644 --- a/test/browser/lifecycle.test.js +++ b/test/browser/lifecycle.test.js @@ -1,5 +1,5 @@ import { setupRerender } from 'preact/test-utils'; -import { createElement as h, render, Component } from '../../src/index'; +import { createElement as h, render, Component, Fragment } from '../../src/index'; import { setupScratch, teardown } from '../_util/helpers'; /** @jsx h */ @@ -2245,6 +2245,15 @@ describe('Lifecycle methods', () => { expect(Receiver.prototype.componentDidCatch).to.have.been.called; }); + it('should be called when child inside a Fragment fails', () => { + function ThrowErr() { + throw new Error('Error!'); + } + + render(, scratch); + expect(Receiver.prototype.componentDidCatch).to.have.been.called; + }); + it('should re-render with new content', () => { class ThrowErr extends Component { componentWillMount() { diff --git a/test/browser/suspense.test.js b/test/browser/suspense.test.js index f20e4d5152c..096516c1d12 100644 --- a/test/browser/suspense.test.js +++ b/test/browser/suspense.test.js @@ -1,94 +1,73 @@ /*eslint-env browser, mocha */ +/** @jsx h */ import { setupRerender } from 'preact/test-utils'; -import { expect } from 'chai'; -import { createElement as h, render, Component, Suspense, lazy } from '../../src/index'; +import { createElement as h, render, Component, Suspense, lazy, Fragment } from '../../src/index'; import { setupScratch, teardown } from '../_util/helpers'; -function schedule(cb) { - setTimeout(cb, 0); -} - class LazyComp extends Component { render() { - return
Hello Lazy
; + return
Hello from LazyComp
; } } -class CustomSuspense extends Component { - constructor(props) { - super(props); - this.state = { done: false }; - } - render() { - if (!this.state.done) { - throw new Promise((res) => { - schedule(() => { - this.setState({ done: true }); - res(); - }); - }); - } - - return ( -
- Hello CustomSuspense -
- ); +function CustomSuspense({ isDone, prom, name }) { + if (!isDone()) { + throw prom; } -} -class ThrowingSuspense extends Component { - constructor(props) { - super(props); - this.state = { done: false }; - } - render() { - if (!this.state.done) { - throw new Promise((res, rej) => { - schedule(() => { - this.setState({ done: true }); - rej(new Error('I\'m a throwing Suspense!')); - }); - }); - } - - return ( -
- Hello ThrowingSuspense -
- ); - } + return ( +
+ Hello from CustomSuspense {name} +
+ ); } class Catcher extends Component { constructor(props) { super(props); - this.state = { error: null }; + this.state = { error: false }; } componentDidCatch(e) { this.setState({ error: e }); } - render() { - return this.state.error ? ( -
- Catcher did catch: {this.state.error.message} -
- ) : this.props.children; + render(props, state) { + return state.error ?
Catcher did catch: {state.error.message}
: props.children; } } +function createSuspension(name, timeout, t) { + let done = false; + const prom = new Promise((res, rej) => { + setTimeout(() => { + done = true; + if (t) { + rej(t); + } + else { + res(); + } + }, timeout); + }); + + return { + name, + prom, + isDone: () => done + }; +} + const Lazy = lazy(() => new Promise((res) => { - schedule(() => { + setTimeout(() => { res({ default: LazyComp }); - }); + }, 0); })); const ThrowingLazy = lazy(() => new Promise((res, rej) => { - schedule(() => { + setTimeout(() => { rej(new Error('Thrown in lazy\'s loader...')); - }); + }, 0); })); class WrapperOne extends Component { @@ -97,8 +76,6 @@ class WrapperOne extends Component { } } -/** @jsx h */ - function tick(fn) { return new Promise((res) => { setTimeout(res, 10); @@ -119,9 +96,12 @@ describe('suspense', () => { }); it('should suspend when using lazy', () => { - render(Suspended...}> - - , scratch); + render( + Suspended...}> + + , + scratch, + ); rerender(); expect(scratch.innerHTML).to.eql( `
Suspended...
` @@ -130,17 +110,22 @@ describe('suspense', () => { return tick(() => { rerender(); expect(scratch.innerHTML).to.eql( - `
Hello Lazy
` + `
Hello from LazyComp
` ); }); }); it('should suspend when a promise is throw', () => { - render(Suspended...}> - - - - , scratch); + const s = createSuspension('regular case', 0, null); + + render( + Suspended...}> + + + + , + scratch, + ); // TODO: why a rerender needed here. Will this even work in the browser?! rerender(); expect(scratch.innerHTML).to.eql( @@ -150,17 +135,22 @@ describe('suspense', () => { return tick(() => { rerender(); expect(scratch.innerHTML).to.eql( - `
Hello CustomSuspense
` + `
Hello from CustomSuspense regular case
` ); }); }); it('should suspend with custom error boundary', () => { - render(Suspended...}> - - - - , scratch); + const s = createSuspension('within error boundary', 0, null); + + render( + Suspended...}> + + + + , + scratch, + ); rerender(); expect(scratch.innerHTML).to.eql( `
Suspended...
` @@ -169,17 +159,22 @@ describe('suspense', () => { return tick(() => { rerender(); expect(scratch.innerHTML).to.eql( - `
Hello CustomSuspense
` + `
Hello from CustomSuspense within error boundary
` ); }); }); it('should support throwing suspense', () => { - render(Suspended...}> - - - - , scratch); + const s = createSuspension('throwing', 0, new Error('Thrown in suspense')); + + render( + Suspended...}> + + + + , + scratch, + ); rerender(); expect(scratch.innerHTML).to.eql( `
Suspended...
` @@ -188,20 +183,114 @@ describe('suspense', () => { return tick(() => { rerender(); expect(scratch.innerHTML).to.eql( - `
Hello ThrowingSuspense
` + `
Hello from CustomSuspense throwing
` ); }); }); - it('should only suspend the most inner Suspend', () => { - render(Suspended... 1}> - Not suspended... - Suspended... 2}> + it('should call multiple suspending components render in one go', () => { + const s1 = createSuspension('first', 0, null); + const s2 = createSuspension('second', 0, null); + const LoggedCustomSuspense = sinon.spy(CustomSuspense); + + render( + Suspended...}> + + {/* Adding a
here will make things work... */} + + + + + , + scratch, + ); + expect(LoggedCustomSuspense).to.have.been.calledTwice; + rerender(); + expect(scratch.innerHTML).to.eql( + `
Suspended...
` + ); + + return tick(() => { + rerender(); + expect(scratch.innerHTML).to.eql( + `
Hello from CustomSuspense first
Hello from CustomSuspense second
` + ); + }); + }); + + it('should call multiple nested suspending components render in one go', () => { + const s1 = createSuspension('first', 0, null); + const s2 = createSuspension('second', 0, null); + const LoggedCustomSuspense = sinon.spy(CustomSuspense); + + render( + Suspended...
}> + + {/* Adding a
here will make things work... */} + +
+ +
+ + + , + scratch, + ); + expect(LoggedCustomSuspense).to.have.been.calledTwice; + rerender(); + expect(scratch.innerHTML).to.eql( + `
Suspended...
` + ); + + return tick(() => { + rerender(); + expect(scratch.innerHTML).to.eql( + `
Hello from CustomSuspense first
Hello from CustomSuspense second
` + ); + }); + }); + + it('should support suspension nested in a Fragment', () => { + const s = createSuspension('nested in a Fragment', 0, null); + + render( + Suspended...
}> - + + +
-
, scratch); + , + scratch, + ); + rerender(); + expect(scratch.innerHTML).to.eql( + `
Suspended...
` + ); + + return tick(() => { + rerender(); + expect(scratch.innerHTML).to.eql( + `
Hello from CustomSuspense nested in a Fragment
` + ); + }); + }); + + it('should only suspend the most inner Suspend', () => { + const s = createSuspension('1', 0, new Error('Thrown in suspense')); + + render( + Suspended... 1}> + Not suspended... + Suspended... 2}> + + + + + , + scratch, + ); rerender(); expect(scratch.innerHTML).to.eql( `Not suspended...
Suspended... 2
` @@ -210,15 +299,20 @@ describe('suspense', () => { return tick(() => { rerender(); expect(scratch.innerHTML).to.eql( - `Not suspended...
Hello CustomSuspense
` + `Not suspended...
Hello from CustomSuspense 1
` ); }); }); it('should throw when missing Suspense', () => { - render( - - , scratch); + const s = createSuspension('1', 0, null); + + render( + + + , + scratch, + ); rerender(); expect(scratch.innerHTML).to.eql( `
Catcher did catch: Missing Suspense
` @@ -226,11 +320,14 @@ describe('suspense', () => { }); it('should throw when lazy\'s loader throws', () => { - render(Suspended...}> - - - - , scratch); + render( + Suspended...}> + + + + , + scratch, + ); rerender(); expect(scratch.innerHTML).to.eql( `
Suspended...
` @@ -243,4 +340,4 @@ describe('suspense', () => { ); }); }); -}); \ No newline at end of file +}); From cdd73b7c6b897664e72cfd13e9944d9b5289fe0b Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sat, 4 May 2019 12:49:15 +0200 Subject: [PATCH 07/26] Make tests more resilient in terms of timing --- test/browser/suspense.test.js | 101 +++++++++++++++++++--------------- 1 file changed, 56 insertions(+), 45 deletions(-) diff --git a/test/browser/suspense.test.js b/test/browser/suspense.test.js index 096516c1d12..eeedadd9cd4 100644 --- a/test/browser/suspense.test.js +++ b/test/browser/suspense.test.js @@ -10,9 +10,9 @@ class LazyComp extends Component { } } -function CustomSuspense({ isDone, prom, name }) { +function CustomSuspense({ isDone, start, name }) { if (!isDone()) { - throw prom; + throw start(); } return ( @@ -39,50 +39,38 @@ class Catcher extends Component { function createSuspension(name, timeout, t) { let done = false; - const prom = new Promise((res, rej) => { - setTimeout(() => { - done = true; - if (t) { - rej(t); - } - else { - res(); - } - }, timeout); - }); + let prom; return { name, - prom, + start: () => { + if (!prom) { + prom = new Promise((res, rej) => { + setTimeout(() => { + done = true; + if (t) { + rej(t); + } + else { + res(); + } + }, timeout); + }); + } + + return prom; + }, + getPromise: () => prom, isDone: () => done }; } -const Lazy = lazy(() => new Promise((res) => { - setTimeout(() => { - res({ default: LazyComp }); - }, 0); -})); - -const ThrowingLazy = lazy(() => new Promise((res, rej) => { - setTimeout(() => { - rej(new Error('Thrown in lazy\'s loader...')); - }, 0); -})); - class WrapperOne extends Component { render() { return this.props.children; } } -function tick(fn) { - return new Promise((res) => { - setTimeout(res, 10); - }) - .then(fn); -} - describe('suspense', () => { let scratch, rerender; @@ -96,6 +84,18 @@ describe('suspense', () => { }); it('should suspend when using lazy', () => { + let prom; + + const Lazy = lazy(() => { + prom = new Promise((res) => { + setTimeout(() => { + res({ default: LazyComp }); + }, 0); + }); + + return prom; + }); + render( Suspended...}> @@ -107,7 +107,7 @@ describe('suspense', () => { `
Suspended...
` ); - return tick(() => { + return prom.then(() => { rerender(); expect(scratch.innerHTML).to.eql( `
Hello from LazyComp
` @@ -132,7 +132,7 @@ describe('suspense', () => { `
Suspended...
` ); - return tick(() => { + return s.getPromise().then(() => { rerender(); expect(scratch.innerHTML).to.eql( `
Hello from CustomSuspense regular case
` @@ -156,7 +156,7 @@ describe('suspense', () => { `
Suspended...
` ); - return tick(() => { + return s.getPromise().then(() => { rerender(); expect(scratch.innerHTML).to.eql( `
Hello from CustomSuspense within error boundary
` @@ -180,7 +180,7 @@ describe('suspense', () => { `
Suspended...
` ); - return tick(() => { + return s.getPromise().catch(() => { rerender(); expect(scratch.innerHTML).to.eql( `
Hello from CustomSuspense throwing
` @@ -210,7 +210,7 @@ describe('suspense', () => { `
Suspended...
` ); - return tick(() => { + return s1.getPromise().then(s2.getPromise).then(() => { rerender(); expect(scratch.innerHTML).to.eql( `
Hello from CustomSuspense first
Hello from CustomSuspense second
` @@ -219,8 +219,8 @@ describe('suspense', () => { }); it('should call multiple nested suspending components render in one go', () => { - const s1 = createSuspension('first', 0, null); - const s2 = createSuspension('second', 0, null); + const s1 = createSuspension('first', 5, null); + const s2 = createSuspension('second', 5, null); const LoggedCustomSuspense = sinon.spy(CustomSuspense); render( @@ -242,7 +242,7 @@ describe('suspense', () => { `
Suspended...
` ); - return tick(() => { + return s1.getPromise().then(s2.getPromise).then(() => { rerender(); expect(scratch.innerHTML).to.eql( `
Hello from CustomSuspense first
Hello from CustomSuspense second
` @@ -269,7 +269,7 @@ describe('suspense', () => { `
Suspended...
` ); - return tick(() => { + return s.getPromise().then(() => { rerender(); expect(scratch.innerHTML).to.eql( `
Hello from CustomSuspense nested in a Fragment
` @@ -278,7 +278,7 @@ describe('suspense', () => { }); it('should only suspend the most inner Suspend', () => { - const s = createSuspension('1', 0, new Error('Thrown in suspense')); + const s = createSuspension('1', 0, null); render( Suspended... 1}> @@ -296,7 +296,7 @@ describe('suspense', () => { `Not suspended...
Suspended... 2
` ); - return tick(() => { + return s.getPromise().then(() => { rerender(); expect(scratch.innerHTML).to.eql( `Not suspended...
Hello from CustomSuspense 1
` @@ -320,6 +320,17 @@ describe('suspense', () => { }); it('should throw when lazy\'s loader throws', () => { + let prom; + + const ThrowingLazy = lazy(() => { + prom = new Promise((res, rej) => { + setTimeout(() => { + rej(new Error('Thrown in lazy\'s loader...')); + }, 0); + }); + return prom; + }); + render( Suspended...}> @@ -333,7 +344,7 @@ describe('suspense', () => { `
Suspended...
` ); - return tick(() => { + return prom.catch(() => { rerender(); expect(scratch.innerHTML).to.eql( `
Catcher did catch: Thrown in lazy's loader...
` From c058372e2bbe372766a709151256c61ec40b7015 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 13:59:57 +0200 Subject: [PATCH 08/26] Golf some bytes --- src/diff/index.js | 4 ++-- src/suspense.js | 41 +++++++++++++++++------------------------ 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/src/diff/index.js b/src/diff/index.js index 7dab6ff623c..72846585f35 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -373,8 +373,8 @@ function catchErrorInComponent(error, component) { if (!component._processingException) { try { if (isSuspend) { - if (component.__s) { - component.__s(error); + if (component._childDidSuspend) { + component._childDidSuspend(error); } else { continue; diff --git a/src/suspense.js b/src/suspense.js index c1dfb4cb6f0..10029d7e268 100644 --- a/src/suspense.js +++ b/src/suspense.js @@ -1,30 +1,23 @@ import { Component } from './component'; import { createElement } from './create-element'; -export class Suspense extends Component { - constructor(props) { - super(props); - - this.state = {}; - } - - __s(e) { - if (typeof e.then == 'function') { - this.setState({ l: 1 }); - const cb = () => { this.setState({ l: 0 }); }; - - // Suspense ignores errors thrown in Promises as this should be handled by user land code - e.then(cb, cb); - } - else { - throw e; - } - } - - render(props, state) { - return state.l ? props.fallback : props.children; - } -} +// having a "custom class" here saves 50bytes gzipped +export function s(props) {} +s.prototype = new Component(); +s.prototype._childDidSuspend = function(e) { + this.setState({ _loading: true }); + const cb = () => { this.setState({ _loading: false }); }; + + // Suspense ignores errors thrown in Promises as this should be handled by user land code + e.then(cb, cb); +}; +s.prototype.render = function(props, state) { + return state._loading ? props.fallback : props.children; +}; + +// exporting s as Suspense instead of naming the class iself Suspense saves 4 bytes gzipped +// TODO: does this add the need of a displayName? +export const Suspense = s; export function lazy(loader) { let prom; From a37853c97ad9850e6bae0adf254399b3d6d29424 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 14:02:15 +0200 Subject: [PATCH 09/26] Add some jsdoc to _childDidSuspend --- src/suspense.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/suspense.js b/src/suspense.js index 10029d7e268..c8264c10516 100644 --- a/src/suspense.js +++ b/src/suspense.js @@ -4,13 +4,18 @@ import { createElement } from './create-element'; // having a "custom class" here saves 50bytes gzipped export function s(props) {} s.prototype = new Component(); -s.prototype._childDidSuspend = function(e) { + +/** + * @param {Promise} promise The thrown promise + */ +s.prototype._childDidSuspend = function(promise) { this.setState({ _loading: true }); const cb = () => { this.setState({ _loading: false }); }; // Suspense ignores errors thrown in Promises as this should be handled by user land code - e.then(cb, cb); + promise.then(cb, cb); }; + s.prototype.render = function(props, state) { return state._loading ? props.fallback : props.children; }; From 861e98d333c2c1562a906de929d27f2982c01c20 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 14:04:26 +0200 Subject: [PATCH 10/26] Golf 2 bytes in lazy() --- src/suspense.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/suspense.js b/src/suspense.js index c8264c10516..7c14f9cf5d9 100644 --- a/src/suspense.js +++ b/src/suspense.js @@ -32,8 +32,8 @@ export function lazy(loader) { if (!prom) { prom = loader(); prom.then( - ({ default: c }) => { component = c; }, - e => error = e, + (exports) => { component = exports.default; }, + (e) => { error = e; }, ); } From 54aaf570d6940d77355b7bd1d1525f38c377fc8d Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 14:08:05 +0200 Subject: [PATCH 11/26] Add displayName to Lazy (+28B) --- src/suspense.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/suspense.js b/src/suspense.js index 7c14f9cf5d9..21b3c7525e0 100644 --- a/src/suspense.js +++ b/src/suspense.js @@ -28,11 +28,15 @@ export function lazy(loader) { let prom; let component; let error; - return function L(props) { + + function Lazy(props) { if (!prom) { prom = loader(); prom.then( - (exports) => { component = exports.default; }, + (exports) => { + component = exports.default; + Lazy.displayName = 'Lazy(' + (component.displayName || component.name) + ')'; + }, (e) => { error = e; }, ); } @@ -46,5 +50,7 @@ export function lazy(loader) { } return createElement(component, props); - }; + } + + return Lazy; } \ No newline at end of file From bc80d46588892251f58ad6aca321096284e4bb2d Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 14:13:25 +0200 Subject: [PATCH 12/26] Golf 5 bytes in catchErrorInComponent --- src/diff/index.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/diff/index.js b/src/diff/index.js index 72846585f35..6201811ccec 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -393,8 +393,7 @@ function catchErrorInComponent(error, component) { } catch (e) { error = e; - isSuspend = typeof error.then === 'function'; - suspendingComponent = component; + isSuspend = false; } } } From de9588c4b60d65522698c20e256d64fc89d33dc8 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 14:21:00 +0200 Subject: [PATCH 13/26] Add some DOM to Wrapper and add a functional Wrapper too in tests --- test/browser/suspense.test.js | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/test/browser/suspense.test.js b/test/browser/suspense.test.js index eeedadd9cd4..be5fef0882f 100644 --- a/test/browser/suspense.test.js +++ b/test/browser/suspense.test.js @@ -65,12 +65,24 @@ function createSuspension(name, timeout, t) { }; } -class WrapperOne extends Component { - render() { - return this.props.children; +class ClassWrapper extends Component { + render(props) { + return ( +
+ {props.children} +
+ ); } } +function FuncWrapper(props) { + return ( +
+ {props.children} +
+ ); +} + describe('suspense', () => { let scratch, rerender; @@ -120,13 +132,14 @@ describe('suspense', () => { render( Suspended...}> - - - + + + + + , scratch, ); - // TODO: why a rerender needed here. Will this even work in the browser?! rerender(); expect(scratch.innerHTML).to.eql( `
Suspended...
` @@ -135,7 +148,7 @@ describe('suspense', () => { return s.getPromise().then(() => { rerender(); expect(scratch.innerHTML).to.eql( - `
Hello from CustomSuspense regular case
` + `
Hello from CustomSuspense regular case
` ); }); }); From 2a527eddb5889d32d08bed668d1c1ae5c0c17130 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 14:48:16 +0200 Subject: [PATCH 14/26] Rename t to error in createSuspension --- test/browser/suspense.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/browser/suspense.test.js b/test/browser/suspense.test.js index be5fef0882f..cedebddeddd 100644 --- a/test/browser/suspense.test.js +++ b/test/browser/suspense.test.js @@ -37,7 +37,7 @@ class Catcher extends Component { } } -function createSuspension(name, timeout, t) { +function createSuspension(name, timeout, error) { let done = false; let prom; @@ -48,8 +48,8 @@ function createSuspension(name, timeout, t) { prom = new Promise((res, rej) => { setTimeout(() => { done = true; - if (t) { - rej(t); + if (error) { + rej(error); } else { res(); From 3f59a37b0b50cea4e50e1baaeb7b09dc6805d421 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 14:51:27 +0200 Subject: [PATCH 15/26] Golf 5 bytes --- src/diff/index.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/diff/index.js b/src/diff/index.js index fa0199b6378..cd752558413 100644 --- a/src/diff/index.js +++ b/src/diff/index.js @@ -43,10 +43,9 @@ export function diff(parentDom, newVNode, oldVNode, context, isSvg, excessDomChi try { outer: if (oldVNode.type===Fragment || newType===Fragment) { - // Passing the ancestorComponent here is needed for catchErrorInComponent to properly traverse upwards through fragments - // to find a parent Suspense - c = { _ancestorComponent: ancestorComponent }; - diffChildren(parentDom, newVNode, oldVNode, context, isSvg, excessDomChildren, mounts, c, oldDom); + // Passing the ancestorComponent instead of c here is needed for catchErrorInComponent + // to properly traverse upwards through fragments to find a parent Suspense + diffChildren(parentDom, newVNode, oldVNode, context, isSvg, excessDomChildren, mounts, ancestorComponent, oldDom); // Mark dom as empty in case `_children` is any empty array. If it isn't // we'll set `dom` to the correct value just a few lines later. From a08684efd66eb6e7a6e02269cd7ea44c2c13db17 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 15:01:33 +0200 Subject: [PATCH 16/26] Fix exports in compat --- compat/src/index.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/compat/src/index.js b/compat/src/index.js index 0e8eaf3e35d..e289c55c99c 100644 --- a/compat/src/index.js +++ b/compat/src/index.js @@ -378,7 +378,9 @@ export { memo, forwardRef, // eslint-disable-next-line camelcase - unstable_batchedUpdates + unstable_batchedUpdates, + Suspense, + lazy }; // React copies the named exports to the default one. From 9dcf70b73b03978cba10c36b879f1fca2c2f1ca5 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 15:02:18 +0200 Subject: [PATCH 17/26] Add Suspense/lazy to devtools demo --- demo/devtools.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/demo/devtools.js b/demo/devtools.js index aa8311e1986..c521fded1ce 100644 --- a/demo/devtools.js +++ b/demo/devtools.js @@ -1,10 +1,17 @@ // eslint-disable-next-line no-unused-vars -import { createElement, Component, memo, Fragment } from "react"; +import { createElement, Component, memo, Fragment, Suspense, lazy } from "react"; function Foo() { return
I'm memoed
; } +function LazyComp() { + return
I'm (fake) lazy loaded
; +} + +console.log(lazy); +const Lazy = lazy(() => Promise.resolve({ default: LazyComp })); + const Memoed = memo(Foo); export default class DevtoolsDemo extends Component { @@ -14,6 +21,11 @@ export default class DevtoolsDemo extends Component {

memo()

functional component:

+

lazy()

+

functional component:

+ Loading (fake) lazy loaded component...}> + + ); } From 5282e85a36a085cab18510430f2a44f2b3ae5833 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 15:04:43 +0200 Subject: [PATCH 18/26] Fix display name of Suspense --- src/suspense.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/suspense.js b/src/suspense.js index 21b3c7525e0..051b3c44880 100644 --- a/src/suspense.js +++ b/src/suspense.js @@ -2,13 +2,13 @@ import { Component } from './component'; import { createElement } from './create-element'; // having a "custom class" here saves 50bytes gzipped -export function s(props) {} -s.prototype = new Component(); +export function Suspense(props) {} +Suspense.prototype = new Component(); /** * @param {Promise} promise The thrown promise */ -s.prototype._childDidSuspend = function(promise) { +Suspense.prototype._childDidSuspend = function(promise) { this.setState({ _loading: true }); const cb = () => { this.setState({ _loading: false }); }; @@ -16,14 +16,10 @@ s.prototype._childDidSuspend = function(promise) { promise.then(cb, cb); }; -s.prototype.render = function(props, state) { +Suspense.prototype.render = function(props, state) { return state._loading ? props.fallback : props.children; }; -// exporting s as Suspense instead of naming the class iself Suspense saves 4 bytes gzipped -// TODO: does this add the need of a displayName? -export const Suspense = s; - export function lazy(loader) { let prom; let component; From 96da3baa09c0b09fe3a13594831127f573d32a74 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 15:06:16 +0200 Subject: [PATCH 19/26] Drop console from devtools demo --- demo/devtools.js | 1 - 1 file changed, 1 deletion(-) diff --git a/demo/devtools.js b/demo/devtools.js index c521fded1ce..9ade69b66a8 100644 --- a/demo/devtools.js +++ b/demo/devtools.js @@ -9,7 +9,6 @@ function LazyComp() { return
I'm (fake) lazy loaded
; } -console.log(lazy); const Lazy = lazy(() => Promise.resolve({ default: LazyComp })); const Memoed = memo(Foo); From 96a9fbbb74449cb7c6217ff89c928d25d469100b Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 15:32:35 +0200 Subject: [PATCH 20/26] Add suspense to demo --- demo/index.js | 3 ++ demo/suspense.js | 87 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+) create mode 100644 demo/suspense.js diff --git a/demo/index.js b/demo/index.js index 6b3eee56598..245e1f6af5f 100644 --- a/demo/index.js +++ b/demo/index.js @@ -17,6 +17,7 @@ import StyledComp from './styled-components'; import { initDevTools } from 'preact/debug/src/devtools'; import { initDebug } from 'preact/debug/src/debug'; import DevtoolsDemo from './devtools'; +import SuspenseDemo from './suspense'; let isBenchmark = /(\/spiral|\/pythagoras|[#&]bench)/g.test(window.location.href); if (!isBenchmark) { @@ -71,6 +72,7 @@ class App extends Component { People Browser State Order Styled Components + Suspense / lazy
@@ -96,6 +98,7 @@ class App extends Component { + diff --git a/demo/suspense.js b/demo/suspense.js new file mode 100644 index 00000000000..a5ed09908bd --- /dev/null +++ b/demo/suspense.js @@ -0,0 +1,87 @@ +// eslint-disable-next-line no-unused-vars +import { createElement, Component, memo, Fragment, Suspense, lazy } from "react"; + +function LazyComp() { + return
I'm (fake) lazy loaded
; +} + +const Lazy = lazy(() => Promise.resolve({ default: LazyComp })); + +function createSuspension(name, timeout, error) { + let done = false; + let prom; + + return { + name, + timeout, + start: () => { + if (!prom) { + prom = new Promise((res, rej) => { + setTimeout(() => { + done = true; + if (error) { + rej(error); + } + else { + res(); + } + }, timeout); + }); + } + + return prom; + }, + getPromise: () => prom, + isDone: () => done + }; +} + +function CustomSuspense({ isDone, start, timeout, name }) { + if (!isDone()) { + throw start(); + } + + return ( +
+ Hello from CustomSuspense {name}, loaded after {timeout / 1000}s +
+ ); +} + +function init() { + return { + s1: createSuspension('1', 1000, null), + s2: createSuspension('2', 2000, null), + s3: createSuspension('3', 3000, null) + }; +} + +export default class DevtoolsDemo extends Component { + constructor(props) { + super(props); + this.state = init(); + } + + onRerun() { + this.setState(init()); + } + + render(props, state) { + return ( +
+

lazy()

+ Loading (fake) lazy loaded component...
}> + + +

Suspense

+ Fallback 1}> + + Fallback 2}> + + + + + + ); + } +} From b9c1d747c4cc79ae107dce98a5633df9505b57f4 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 15:33:01 +0200 Subject: [PATCH 21/26] Add test that shows ordering issue of Suspense inside Fragment --- test/browser/suspense.test.js | 64 +++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/test/browser/suspense.test.js b/test/browser/suspense.test.js index cedebddeddd..ae5aacb714b 100644 --- a/test/browser/suspense.test.js +++ b/test/browser/suspense.test.js @@ -290,6 +290,70 @@ describe('suspense', () => { }); }); + it('should support multiple non-nested Suspense', () => { + const s1 = createSuspension('1', 0, null); + const s2 = createSuspension('2', 0, null); + + render( +
+
+ Suspended 1...
}> + + +
+
+ Suspended 2...
}> + + + + + , + scratch, + ); + rerender(); + expect(scratch.innerHTML).to.eql( + `
Suspended 1...
Suspended 2...
` + ); + + return s1.getPromise().then(s2.getPromise).then(() => { + rerender(); + expect(scratch.innerHTML).to.eql( + `
Hello from CustomSuspense 1
Hello from CustomSuspense 2
` + ); + }); + }); + + it('should support multiple Suspense inside a Fragment', () => { + const s1 = createSuspension('1', 0, null); + const s2 = createSuspension('2', 0, null); + + render( +
+ + Suspended 1...
}> + + + Suspended 2...}> + + + + + , + scratch, + ); + rerender(); + expect(scratch.innerHTML).to.eql( + `
Suspended 1...
Suspended 2...
` + ); + + return s1.getPromise().then(s2.getPromise).then(() => { + rerender(); + expect(scratch.innerHTML).to.eql( + `
Hello from CustomSuspense 1
Hello from CustomSuspense 2
` + ); + }); + }); + it('should only suspend the most inner Suspend', () => { const s = createSuspension('1', 0, null); From ada75fe4ab556eafe283d1f2a4ab03c98cc2a92a Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 15:33:31 +0200 Subject: [PATCH 22/26] Keep displayName of Lazy static --- src/suspense.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/suspense.js b/src/suspense.js index 051b3c44880..8126f995796 100644 --- a/src/suspense.js +++ b/src/suspense.js @@ -29,10 +29,7 @@ export function lazy(loader) { if (!prom) { prom = loader(); prom.then( - (exports) => { - component = exports.default; - Lazy.displayName = 'Lazy(' + (component.displayName || component.name) + ')'; - }, + (exports) => { component = exports.default; }, (e) => { error = e; }, ); } @@ -48,5 +45,7 @@ export function lazy(loader) { return createElement(component, props); } + Lazy.displayName = 'Lazy'; + return Lazy; -} \ No newline at end of file +} From f39793ab5af79ab67d8c8b48e586cfbeb5eae41b Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 16:50:46 +0200 Subject: [PATCH 23/26] Add rerun button to demo --- demo/suspense.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/demo/suspense.js b/demo/suspense.js index a5ed09908bd..7850630dfea 100644 --- a/demo/suspense.js +++ b/demo/suspense.js @@ -49,6 +49,7 @@ function CustomSuspense({ isDone, start, timeout, name }) { } function init() { + console.log('init'); return { s1: createSuspension('1', 1000, null), s2: createSuspension('2', 2000, null), @@ -60,6 +61,7 @@ export default class DevtoolsDemo extends Component { constructor(props) { super(props); this.state = init(); + this.onRerun = this.onRerun.bind(this) } onRerun() { @@ -74,6 +76,9 @@ export default class DevtoolsDemo extends Component {

Suspense

+
+ +
Fallback 1}> Fallback 2}> From 8a93332e96bec8dbf12313a2df66ab3e1c8ae2c0 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 16:51:14 +0200 Subject: [PATCH 24/26] Drop console --- demo/suspense.js | 1 - 1 file changed, 1 deletion(-) diff --git a/demo/suspense.js b/demo/suspense.js index 7850630dfea..28d4ce72056 100644 --- a/demo/suspense.js +++ b/demo/suspense.js @@ -49,7 +49,6 @@ function CustomSuspense({ isDone, start, timeout, name }) { } function init() { - console.log('init'); return { s1: createSuspension('1', 1000, null), s2: createSuspension('2', 2000, null), From 96932c77e958a96e14d3bb5be02381ab1e8dbdeb Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Sun, 5 May 2019 16:51:32 +0200 Subject: [PATCH 25/26] Add lost semicolon --- demo/suspense.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/demo/suspense.js b/demo/suspense.js index 28d4ce72056..6f888d50864 100644 --- a/demo/suspense.js +++ b/demo/suspense.js @@ -60,7 +60,7 @@ export default class DevtoolsDemo extends Component { constructor(props) { super(props); this.state = init(); - this.onRerun = this.onRerun.bind(this) + this.onRerun = this.onRerun.bind(this); } onRerun() { From 8677fa7103568f71a07e509bd248103b14dd3563 Mon Sep 17 00:00:00 2001 From: Sven Tschui Date: Mon, 6 May 2019 07:50:52 +0200 Subject: [PATCH 26/26] Add debug logging for propTypes on lazy --- debug/src/debug.js | 16 ++++++++++ debug/test/browser/debug.test.js | 52 ++++++++++++++++++++++++++++++-- 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/debug/src/debug.js b/debug/src/debug.js index c09a0863fd6..c4ed11c4e4b 100644 --- a/debug/src/debug.js +++ b/debug/src/debug.js @@ -74,6 +74,22 @@ export function initDebug() { // Check prop-types if available if (typeof vnode.type==='function' && vnode.type.propTypes) { + if (vnode.type.name === 'Lazy') { + const m = 'PropTypes are not supported on lazy(). Use propTypes on the wrapped component itself. '; + try { + const lazied = vnode.type(); + console.warn(m + 'Component wrapped in lazy() is ' + lazied.then.displayName || lazied.then.name); + } + catch (promise) { + console.warn(m + 'We will log the wrapped component\'s name once it is loaded.'); + if (promise.then) { + promise.then((exports) => { + console.warn('Component wrapped in lazy() is ' + (exports.default.displayName || exports.default.name)); + }); + } + + } + } checkPropTypes(vnode.type.propTypes, vnode.props, getDisplayName(vnode), serializeVNode(vnode)); } diff --git a/debug/test/browser/debug.test.js b/debug/test/browser/debug.test.js index 1e5b6fd65f3..151b667df72 100644 --- a/debug/test/browser/debug.test.js +++ b/debug/test/browser/debug.test.js @@ -1,6 +1,6 @@ -import { createElement as h, options, render, createRef, Component, Fragment } from 'preact'; +import { createElement as h, options, render, createRef, Component, Fragment, lazy, Suspense } from 'preact'; import { useState, useEffect, useLayoutEffect, useMemo, useCallback } from 'preact/hooks'; -import { act } from 'preact/test-utils'; +import { act, setupRerender } from 'preact/test-utils'; import { setupScratch, teardown, clearOptions, serializeHtml } from '../../../test/_util/helpers'; import { serializeVNode, initDebug } from '../../src/debug'; import * as PropTypes from 'prop-types'; @@ -392,5 +392,53 @@ describe('debug', () => { render(, scratch); expect(console.error).to.not.be.called; }); + + it('should validate propTypes inside lazy()', () => { + const rerender = setupRerender(); + + function Baz(props) { + return

{props.unhappy}

; + } + + Baz.propTypes = { + unhappy: function alwaysThrows(obj, key) { if (obj[key] === 'signal') throw Error('got prop inside lazy()'); } + }; + + + const loader = Promise.resolve({ default: Baz }); + const LazyBaz = lazy(() => loader); + + render( + fallback...}> + + , + scratch + ); + + expect(console.error).to.not.be.called; + + return loader.then(() => { + rerender(); + expect(errors.length).to.equal(1); + expect(errors[0].includes('got prop')).to.equal(true); + expect(serializeHtml(scratch)).to.equal('

signal

'); + }); + }); + + it('should warn for PropTypes on lazy()', () => { + const loader = Promise.resolve({ default: function MyLazyLoadedComponent() { return
Hi there
} }); + const FakeLazy = lazy(() => loader); + FakeLazy.propTypes = {}; + render( + fallback...} > + + , + scratch + ); + return loader.then(() => { + expect(console.warn).to.be.calledTwice; + expect(warnings[1].includes('MyLazyLoadedComponent')).to.equal(true); + }); + }); }); });