From b570815a13cd47b0aac50866bb7e731f8e777eea Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 18 Oct 2018 18:04:55 -0700 Subject: [PATCH] React.lazy constructor must return result of a dynamic import We may want to change the protocol later, so until then we'll be restrictive. Heuristic is to check for existence of `default`. --- .../__tests__/ReactServerRendering-test.js | 9 +++- .../ReactServerRenderingHydration-test.js | 12 +++-- .../src/ReactFiberLazyComponent.js | 30 ++++++----- .../ReactIncrementalPerf-test.internal.js | 12 +++-- .../src/__tests__/ReactLazy-test.internal.js | 54 ++++++++++--------- .../src/__tests__/ReactPure-test.internal.js | 8 ++- .../__tests__/ReactSuspense-test.internal.js | 6 ++- 7 files changed, 81 insertions(+), 50 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactServerRendering-test.js b/packages/react-dom/src/__tests__/ReactServerRendering-test.js index f15cb5557e5c8..88b47fde827ee 100644 --- a/packages/react-dom/src/__tests__/ReactServerRendering-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRendering-test.js @@ -571,14 +571,19 @@ describe('ReactDOMServer', () => { ReactDOMServer.renderToString(); }).toThrow('ReactDOMServer does not yet support Suspense.'); + async function fakeImport(result) { + return {default: result}; + } + expect(() => { - const LazyFoo = React.lazy( - () => + const LazyFoo = React.lazy(() => + fakeImport( new Promise(resolve => resolve(function Foo() { return
; }), ), + ), ); ReactDOMServer.renderToString(); }).toThrow('ReactDOMServer does not yet support lazy-loaded components.'); diff --git a/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js b/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js index 17b8932e6973c..3bd57a8e7eec1 100644 --- a/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js @@ -444,14 +444,20 @@ describe('ReactDOMServerHydration', () => { }); it('should be able to use lazy components after hydrating', async () => { + async function fakeImport(result) { + return {default: result}; + } + const Lazy = React.lazy( () => new Promise(resolve => { setTimeout( () => - resolve(function World() { - return 'world'; - }), + resolve( + fakeImport(function World() { + return 'world'; + }), + ), 1000, ); }), diff --git a/packages/react-reconciler/src/ReactFiberLazyComponent.js b/packages/react-reconciler/src/ReactFiberLazyComponent.js index 8595e0e2a3cfd..ec76f8ca9d248 100644 --- a/packages/react-reconciler/src/ReactFiberLazyComponent.js +++ b/packages/react-reconciler/src/ReactFiberLazyComponent.js @@ -11,6 +11,8 @@ import type {LazyComponentThenable} from 'shared/ReactLazyComponent'; import {Resolved, Rejected, Pending} from 'shared/ReactLazyComponent'; +import warning from 'shared/warning'; + export function readLazyComponentType( thenable: LazyComponentThenable, ): T { @@ -26,22 +28,22 @@ export function readLazyComponentType( default: { thenable._status = Pending; thenable.then( - resolvedValue => { + moduleObject => { if (thenable._status === Pending) { - thenable._status = Resolved; - if (typeof resolvedValue === 'object' && resolvedValue !== null) { - // If the `default` property is not empty, assume it's the result - // of an async import() and use that. Otherwise, use the - // resolved value itself. - const defaultExport = (resolvedValue: any).default; - resolvedValue = - defaultExport !== undefined && defaultExport !== null - ? defaultExport - : resolvedValue; - } else { - resolvedValue = resolvedValue; + const defaultExport = moduleObject.default; + if (__DEV__) { + if (defaultExport === undefined) { + warning( + false, + 'lazy: Expected the result of a dynamic import() call. ' + + 'Instead received: %s\n\nYour code should look like: \n ' + + "const MyComponent = lazy(() => import('./MyComponent'))", + moduleObject, + ); + } } - thenable._result = resolvedValue; + thenable._status = Resolved; + thenable._result = defaultExport; } }, error => { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js index a6bb1a7663b6e..7faa68ce85ffd 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js @@ -573,6 +573,10 @@ describe('ReactDebugFiberPerf', () => { return ; } + async function fakeImport(result) { + return {default: result}; + } + let resolve; const LazyFoo = React.lazy( () => @@ -591,9 +595,11 @@ describe('ReactDebugFiberPerf', () => { ReactNoop.flush(); expect(getFlameChart()).toMatchSnapshot(); - resolve(function Foo() { - return
; - }); + resolve( + fakeImport(function Foo() { + return
; + }), + ); await LazyFoo; ReactNoop.render( diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index e39fc79c83884..4df941acd42a1 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -21,8 +21,12 @@ describe('ReactLazy', () => { return props.text; } + async function fakeImport(result) { + return {default: result}; + } + it('suspends until module has loaded', async () => { - const LazyText = lazy(async () => Text); + const LazyText = lazy(() => fakeImport(Text)); const root = ReactTestRenderer.create( }> @@ -51,8 +55,10 @@ describe('ReactLazy', () => { expect(root).toMatchRenderedOutput('Hi again'); }); - it('uses `default` property, if it exists', async () => { - const LazyText = lazy(async () => ({default: Text})); + it('does not support arbitrary promises, only module objects', async () => { + spyOnDev(console, 'error'); + + const LazyText = lazy(async () => Text); const root = ReactTestRenderer.create( }> @@ -67,17 +73,13 @@ describe('ReactLazy', () => { await LazyText; - expect(root).toFlushAndYield(['Hi']); - expect(root).toMatchRenderedOutput('Hi'); - - // Should not suspend on update - root.update( - }> - - , - ); - expect(root).toFlushAndYield(['Hi again']); - expect(root).toMatchRenderedOutput('Hi again'); + if (__DEV__) { + expect(console.error).toHaveBeenCalledTimes(1); + expect(console.error.calls.argsFor(0)[0]).toContain( + 'Expected the result of a dynamic import() call', + ); + } + expect(root).toFlushAndThrow('Element type is invalid'); }); it('throws if promise rejects', async () => { @@ -117,8 +119,8 @@ describe('ReactLazy', () => { } } - const LazyChildA = lazy(async () => Child); - const LazyChildB = lazy(async () => Child); + const LazyChildA = lazy(() => fakeImport(Child)); + const LazyChildB = lazy(() => fakeImport(Child)); function Parent({swap}) { return ( @@ -160,7 +162,7 @@ describe('ReactLazy', () => { return ; } T.defaultProps = {text: 'Hi'}; - const LazyText = lazy(async () => T); + const LazyText = lazy(() => fakeImport(T)); const root = ReactTestRenderer.create( }> @@ -200,7 +202,7 @@ describe('ReactLazy', () => { ); } LazyImpl.defaultProps = {siblingText: 'Sibling'}; - const Lazy = lazy(async () => LazyImpl); + const Lazy = lazy(() => fakeImport(LazyImpl)); class Stateful extends React.Component { state = {text: 'A'}; @@ -239,7 +241,7 @@ describe('ReactLazy', () => { const LazyFoo = lazy(() => { ReactTestRenderer.unstable_yield('Started loading'); const Foo = props =>
{[, ]}
; - return Promise.resolve(Foo); + return fakeImport(Foo); }); const root = ReactTestRenderer.create( @@ -263,13 +265,13 @@ describe('ReactLazy', () => { }); it('supports class and forwardRef components', async () => { - const LazyClass = lazy(async () => { + const LazyClass = lazy(() => { class Foo extends React.Component { render() { return ; } } - return Foo; + return fakeImport(Foo); }); const LazyForwardRef = lazy(async () => { @@ -278,10 +280,12 @@ describe('ReactLazy', () => { return ; } } - return React.forwardRef((props, ref) => { - ReactTestRenderer.unstable_yield('forwardRef'); - return ; - }); + return fakeImport( + React.forwardRef((props, ref) => { + ReactTestRenderer.unstable_yield('forwardRef'); + return ; + }), + ); }); const ref = React.createRef(); diff --git a/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js b/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js index d1a9f5542b92c..3478cabb1c076 100644 --- a/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactPure-test.internal.js @@ -34,6 +34,10 @@ describe('pure', () => { return ; } + async function fakeImport(result) { + return {default: result}; + } + // Tests should run against both the lazy and non-lazy versions of `pure`. // To make the tests work for both versions, we wrap the non-lazy version in // a lazy function component. @@ -42,11 +46,11 @@ describe('pure', () => { function Indirection(props, ref) { return ; } - return React.lazy(async () => React.forwardRef(Indirection)); + return React.lazy(() => fakeImport(React.forwardRef(Indirection))); }); sharedTests('lazy', (...args) => { const Pure = React.pure(...args); - return React.lazy(async () => Pure); + return React.lazy(() => fakeImport(Pure)); }); function sharedTests(label, pure) { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 5ce531841266f..0a6f6a445a461 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -256,7 +256,11 @@ describe('ReactSuspense', () => { } } - const LazyClass = React.lazy(() => Promise.resolve(Class)); + async function fakeImport(result) { + return {default: result}; + } + + const LazyClass = React.lazy(() => fakeImport(Class)); const root = ReactTestRenderer.create( }>