From afbd59a742d412157e5d4bb86ab4cf33ca167698 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 18 Jan 2019 22:20:43 +0000 Subject: [PATCH 1/2] Fix handling of sync rejection Reverts #14632 and adds a regression test. --- .../src/ReactFiberLazyComponent.js | 2 +- .../src/__tests__/ReactLazy-test.internal.js | 31 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberLazyComponent.js b/packages/react-reconciler/src/ReactFiberLazyComponent.js index 07fd7a70762..179262fa449 100644 --- a/packages/react-reconciler/src/ReactFiberLazyComponent.js +++ b/packages/react-reconciler/src/ReactFiberLazyComponent.js @@ -47,6 +47,7 @@ export function readLazyComponentType(lazyComponent: LazyComponent): T { lazyComponent._status = Pending; const ctor = lazyComponent._ctor; const thenable = ctor(); + lazyComponent._result = thenable; thenable.then( moduleObject => { if (lazyComponent._status === Pending) { @@ -77,7 +78,6 @@ export function readLazyComponentType(lazyComponent: LazyComponent): T { if (lazyComponent._status === Resolved) { return lazyComponent._result; } - lazyComponent._result = thenable; throw thenable; } } diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index 8fb4210f929..58460cc0928 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -78,6 +78,37 @@ describe('ReactLazy', () => { expect(root).toMatchRenderedOutput('Hi'); }); + it('can handle synchronous thenable rejection', async () => { + const LazyText = lazy(() => ({ + then(resolve, reject) { + reject(new Error('oh no')); + }, + })); + + class ErrorBoundary extends React.Component { + state = {}; + static getDerivedStateFromError(error) { + return {message: error.message}; + } + render() { + return this.state.message + ? `Error: ${this.state.message}` + : this.props.children; + } + } + + const root = ReactTestRenderer.create( + + }> + + + , + ); + // TODO: ideally we could make it skip this commit. + expect(ReactTestRenderer).toHaveYielded(['Loading...']); + expect(root).toMatchRenderedOutput('Error: oh no'); + }); + it('multiple lazy components', async () => { function Foo() { return ; From 842d1e2e57bc6c63ead4bf55aaa83f5b0fc13b27 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 18 Jan 2019 22:24:35 +0000 Subject: [PATCH 2/2] Handle rejection synchronously too Fewer footguns and seems like nicer behavior anyway. --- .../react-reconciler/src/ReactFiberLazyComponent.js | 11 +++++++---- .../src/__tests__/ReactLazy-test.internal.js | 5 ++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberLazyComponent.js b/packages/react-reconciler/src/ReactFiberLazyComponent.js index 179262fa449..1db5ecd35ca 100644 --- a/packages/react-reconciler/src/ReactFiberLazyComponent.js +++ b/packages/react-reconciler/src/ReactFiberLazyComponent.js @@ -47,7 +47,6 @@ export function readLazyComponentType(lazyComponent: LazyComponent): T { lazyComponent._status = Pending; const ctor = lazyComponent._ctor; const thenable = ctor(); - lazyComponent._result = thenable; thenable.then( moduleObject => { if (lazyComponent._status === Pending) { @@ -74,10 +73,14 @@ export function readLazyComponentType(lazyComponent: LazyComponent): T { } }, ); - // Check if it resolved synchronously - if (lazyComponent._status === Resolved) { - return lazyComponent._result; + // Handle synchronous thenables. + switch (lazyComponent._status) { + case Resolved: + return lazyComponent._result; + case Rejected: + throw lazyComponent._result; } + lazyComponent._result = thenable; throw thenable; } } diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index 58460cc0928..4ce78581544 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -78,7 +78,7 @@ describe('ReactLazy', () => { expect(root).toMatchRenderedOutput('Hi'); }); - it('can handle synchronous thenable rejection', async () => { + it('can reject synchronously without suspending', async () => { const LazyText = lazy(() => ({ then(resolve, reject) { reject(new Error('oh no')); @@ -104,8 +104,7 @@ describe('ReactLazy', () => { , ); - // TODO: ideally we could make it skip this commit. - expect(ReactTestRenderer).toHaveYielded(['Loading...']); + expect(ReactTestRenderer).toHaveYielded([]); expect(root).toMatchRenderedOutput('Error: oh no'); });