Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Suspense and lazy #1593

Merged
merged 37 commits into from May 9, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
5b28ba5
Introduce first draft of Suspense and lazy
May 2, 2019
1d525f1
Add tests, handle missing suspense, handle custom error boundaries
May 2, 2019
eb9c7db
Work on sunspense...
May 2, 2019
20cb0ae
Merge branch 'master' into feature/suspense-lazy
sventschui May 2, 2019
104649c
Make tests work
May 3, 2019
b8bb86e
Replace setImmediate with setTimeout
May 3, 2019
5036536
Make suspense work like it does in React
May 4, 2019
cdd73b7
Make tests more resilient in terms of timing
May 4, 2019
c058372
Golf some bytes
May 5, 2019
a37853c
Add some jsdoc to _childDidSuspend
May 5, 2019
861e98d
Golf 2 bytes in lazy()
May 5, 2019
54aaf57
Add displayName to Lazy (+28B)
May 5, 2019
bc80d46
Golf 5 bytes in catchErrorInComponent
May 5, 2019
de9588c
Add some DOM to Wrapper and add a functional Wrapper too in tests
May 5, 2019
563fffa
Merge branch 'master' into feature/suspense-lazy
sventschui May 5, 2019
2a527ed
Rename t to error in createSuspension
May 5, 2019
3f59a37
Golf 5 bytes
May 5, 2019
a08684e
Fix exports in compat
May 5, 2019
9dcf70b
Add Suspense/lazy to devtools demo
May 5, 2019
5282e85
Fix display name of Suspense
May 5, 2019
96da3ba
Drop console from devtools demo
May 5, 2019
96a9fbb
Add suspense to demo
May 5, 2019
b9c1d74
Add test that shows ordering issue of Suspense inside Fragment
May 5, 2019
ada75fe
Keep displayName of Lazy static
May 5, 2019
f39793a
Add rerun button to demo
May 5, 2019
8a93332
Drop console
May 5, 2019
96932c7
Add lost semicolon
May 5, 2019
7b8ea43
Merge branch 'master' into feature/suspense-lazy
sventschui May 6, 2019
8677fa7
Add debug logging for propTypes on lazy
May 6, 2019
26ae430
Use Lazy.displayName instead of .name in debug
May 6, 2019
30ce608
Skip test that will be fixed later
May 8, 2019
2c63c59
Drop TODO
May 8, 2019
85075cb
Merge branch 'master' into feature/suspense-lazy
sventschui May 8, 2019
23970fb
Improve coverage
May 8, 2019
d5df946
Improve coverage
May 8, 2019
a22531b
Fix debug message
May 8, 2019
0431636
Add debug test with throwing loader in lazy
May 9, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions 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';
Expand Down Expand Up @@ -401,5 +401,7 @@ export default assign({
PureComponent,
memo,
forwardRef,
unstable_batchedUpdates
unstable_batchedUpdates,
Suspense,
lazy
}, hooks);
30 changes: 29 additions & 1 deletion src/diff/index.js
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
sventschui marked this conversation as resolved.
Show resolved Hide resolved
// console.log('hitting suspense...');
component.componentDidCatch(error);
}
else {
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move to here?

Suggested change
continue;
return catchErrorInComponent(Error('Missing Suspense'), suspendingComponent);

Copy link
Member Author

Choose a reason for hiding this comment

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

The continue results in traversing up the tree. This would break the tree traversing AFAIK

}
}
else if (component.constructor.getDerivedStateFromError!=null) {
component.setState(component.constructor.getDerivedStateFromError(error));
}
else if (component.componentDidCatch!=null) {
Expand All @@ -378,8 +393,21 @@ function catchErrorInComponent(error, component) {
}
catch (e) {
error = e;
isSuspend = typeof error.then === 'function';
sventschui marked this conversation as resolved.
Show resolved Hide resolved
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
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can move this section into preact/debug. A Suspense component without a fallback doesn't seem to make much sense, so we could just always warn on that. This would also keep core small and tiny 馃帀

EDIT: Removing that would save 158 B 馃憤

Copy link
Member

Choose a reason for hiding this comment

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

This also allows the recheck of isSuspsend and suspendingComponent, so that will save a lot

Copy link
Member Author

@sventschui sventschui May 4, 2019

Choose a reason for hiding this comment

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

I still throw the Missing Suspense error on the suspendingComponent.

Would you want to save some bytes and just throw the error at the end of the function instead of the suspendingComponent?

Copy link
Member

Choose a reason for hiding this comment

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

What he's saying is to throw it in a hook from preact/debug since it doesn't really matter in production anyway. I also think that is the best approach to maintain our lightweight profile

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you guys give me a hint on where I could add this to preact/debug. I did not find a proper place to add it

&& suspendingComponent._vnode.type.name);

return catchErrorInComponent(new Error(`${componentName} 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.`), suspendingComponent);
}

throw error;
}
1 change: 1 addition & 0 deletions src/index.js
Expand Up @@ -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';
68 changes: 68 additions & 0 deletions src/suspense.js
@@ -0,0 +1,68 @@
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 = Symbol.for('Suspense');

export class Suspense extends Component {
constructor(props) {
sventschui marked this conversation as resolved.
Show resolved Hide resolved
// TODO: should we add propTypes in DEV mode?
super(props);

// mark this component as a handler of suspension (thrown Promises)
this[sym] = sym;
sventschui marked this conversation as resolved.
Show resolved Hide resolved

this.state = {
l: false
};
}

componentDidCatch(e) {
if (e && typeof e.then === 'function') {
marvinhagemeister marked this conversation as resolved.
Show resolved Hide resolved
this.setState({ l: true });
e.then(
() => {
JoviDeCroock marked this conversation as resolved.
Show resolved Hide resolved
this.setState({ l: false });
},
// Suspense ignores errors thrown in Promises as this should be handled by user land code
() => {
this.setState({ l: false });
}
);
}
else {
throw e;
}
}

render() {
return this.state.l ? this.props.fallback : this.props.children;
marvinhagemeister marked this conversation as resolved.
Show resolved Hide resolved
}
}

export function lazy(loader) {
let prom;
let component;
let error;
return function Lazy(props) {
if (!prom) {
prom = loader();
prom.then(
({ default: c }) => { component = c; },
sventschui marked this conversation as resolved.
Show resolved Hide resolved
e => error = e,
sventschui marked this conversation as resolved.
Show resolved Hide resolved
);
}

if (error) {
throw error;
}

if (!component) {
throw prom;
}

return createElement(component, props);
};
}
235 changes: 235 additions & 0 deletions test/browser/suspense.test.js
@@ -0,0 +1,235 @@
/*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 <div>Hello Lazy</div>;
}
}

class CustomSuspense extends Component {
constructor(props) {
super(props);
this.state = { done: false };
}
render() {
if (!this.state.done) {
throw new Promise((res) => {
setImmediate(() => {
this.setState({ done: true });
res();
});
});
}

return (
<div>
Hello CustomSuspense
</div>
);
}
}

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 (
<div>
Hello ThrowingSuspense
</div>
);
}
}

class Catcher extends Component {
constructor(props) {
super(props);
this.state = { error: null };
}

componentDidCatch(e) {
this.setState({ error: e });
}

render() {
return this.state.error ? (
<div>
Catcher did catch: {this.state.error.message}
</div>
) : this.props.children;
}
}

const Lazy = lazy(() => new Promise((res) => {
setImmediate(() => {
JoviDeCroock marked this conversation as resolved.
Show resolved Hide resolved
res({ default: LazyComp });
});
}));

const ThrowingLazy = lazy(() => new Promise((res, rej) => {
setImmediate(() => {
JoviDeCroock marked this conversation as resolved.
Show resolved Hide resolved
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;

beforeEach(() => {
scratch = setupScratch();
rerender = setupRerender();
});

afterEach(() => {
teardown(scratch);
});

it('should suspend when using lazy', () => {
render(<Suspense fallback={<div>Suspended...</div>}>
<Lazy />
</Suspense>, scratch);
rerender();
expect(scratch.innerHTML).to.eql(
`<div>Suspended...</div>`
);

return tick(() => {
rerender();
expect(scratch.innerHTML).to.eql(
`<div>Hello Lazy</div>`
);
});
});

it('should suspend when a promise is throw', () => {
render(<Suspense fallback={<div>Suspended...</div>}>
<CustomSuspense />
</Suspense>, scratch);
rerender();
expect(scratch.innerHTML).to.eql(
`<div>Suspended...</div>`
);

return tick(() => {
rerender();
expect(scratch.innerHTML).to.eql(
`<div>Hello CustomSuspense...</div>`
);
});
});

it('should suspend with custom error boundary', () => {
render(<Suspense fallback={<div>Suspended...</div>}>
<Catcher>
<CustomSuspense />
</Catcher>
</Suspense>, scratch);
rerender();
expect(scratch.innerHTML).to.eql(
`<div>Suspended...</div>`
);

return tick(() => {
rerender();
expect(scratch.innerHTML).to.eql(
`<div>Hello CustomSuspense...</div>`
);
});
});

it('should support throwing suspense', () => {
render(<Suspense fallback={<div>Suspended...</div>}>
<Catcher>
<CustomSuspense />
</Catcher>
</Suspense>, scratch);
rerender();
expect(scratch.innerHTML).to.eql(
`<div>Suspended...</div>`
);

return tick(() => {
rerender();
expect(scratch.innerHTML).to.eql(
`<div>Hello ThrowingSuspense...</div>`
);
});
});

it('should only suspend the most inner Suspend', () => {
render(<Suspense fallback={<div>Suspended... 1</div>}>
Not suspended...
<Suspense fallback={<div>Suspended... 2</div>}>
<Catcher>
<CustomSuspense />
</Catcher>
</Suspense>
</Suspense>, scratch);
rerender();
expect(scratch.innerHTML).to.eql(
`Not suspended...<div>Suspended... 2</div>`
);

return tick(() => {
rerender();
expect(scratch.innerHTML).to.eql(
`Not suspended...<div>Hello CustomSuspend</div>`
);
});
});

it('should throw when missing Suspense', () => {
render(<Catcher>
<CustomSuspense />
</Catcher>, scratch);
rerender();
expect(scratch.innerHTML).to.eql(
`<div>Catcher did catch: CustomSuspense suspended while rendering, but no fallback UI was specified.

Add a &lt;Suspense fallback=...&gt; component higher in the tree to provide a loading indicator or placeholder to display.</div>`
);
});

it('should throw when lazy\'s loader throws', () => {
render(<Suspense fallback={<div>Suspended...</div>}>
<Catcher>
<ThrowingLazy />
</Catcher>
</Suspense>, scratch);
rerender();
expect(scratch.innerHTML).to.eql(
`<div>Suspended...</div>`
);

return tick(() => {
rerender();
expect(scratch.innerHTML).to.eql(
`<div>Hello ThrowingSuspense...</div>`
);
});
});
});
JoviDeCroock marked this conversation as resolved.
Show resolved Hide resolved