React.lazy constructor must return result of a dynamic import (#13886)

We may want to change the protocol later, so until then we'll be restrictive. Heuristic is to check for existence of `default`.
facebook · Oct 19, 2018 · d9a3cc0 · d9a3cc0
1 parent d9659e4
commit d9a3cc0
Show file tree

Hide file tree

Showing 8 changed files with 85 additions and 55 deletions.
diff --git 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(<React.unstable_Suspense />);
     }).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 <div />;
             }),
           ),
+        ),
       );
       ReactDOMServer.renderToString(<LazyFoo />);
     }).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
@@ -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
@@ -10,6 +10,7 @@
 import type {LazyComponent} from 'shared/ReactLazyComponent';
 
 import {Resolved, Rejected, Pending} from 'shared/ReactLazyComponent';
+import warning from 'shared/warning';
 
 export function readLazyComponentType<T>(lazyComponent: LazyComponent<T>): T {
   const status = lazyComponent._status;
@@ -26,22 +27,22 @@ export function readLazyComponentType<T>(lazyComponent: LazyComponent<T>): T {
       const ctor = lazyComponent._ctor;
       const thenable = ctor();
       thenable.then(
-        resolvedValue => {
+        moduleObject => {
           if (lazyComponent._status === Pending) {
-            lazyComponent._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,
+                );
+              }
             }
-            lazyComponent._result = resolvedValue;
+            lazyComponent._status = Resolved;
+            lazyComponent._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
@@ -573,6 +573,10 @@ describe('ReactDebugFiberPerf', () => {
       return <span />;
     }
 
+    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 <div />;
-    });
+    resolve(
+      fakeImport(function Foo() {
+        return <div />;
+      }),
+    );
     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
@@ -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(
       <Suspense fallback={<Text text="Loading..." />}>
@@ -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(
       <Suspense fallback={<Text text="Loading..." />}>
@@ -67,17 +73,13 @@ describe('ReactLazy', () => {
 
     await LazyText;
 
-    expect(root).toFlushAndYield(['Hi']);
-    expect(root).toMatchRenderedOutput('Hi');
-
-    // Should not suspend on update
-    root.update(
-      <Suspense fallback={<Text text="Loading..." />}>
-        <LazyText text="Hi again" />
-      </Suspense>,
-    );
-    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 <Text {...props} />;
     }
     T.defaultProps = {text: 'Hi'};
-    const LazyText = lazy(async () => T);
+    const LazyText = lazy(() => fakeImport(T));
 
     const root = ReactTestRenderer.create(
       <Suspense fallback={<Text text="Loading..." />}>
@@ -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 => <div>{[<Text text="A" />, <Text text="B" />]}</div>;
-      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 <Text text="Foo" />;
         }
       }
-      return Foo;
+      return fakeImport(Foo);
     });
 
     const LazyForwardRef = lazy(async () => {
@@ -278,10 +280,12 @@ describe('ReactLazy', () => {
           return <Text text="Bar" />;
         }
       }
-      return React.forwardRef((props, ref) => {
-        ReactTestRenderer.unstable_yield('forwardRef');
-        return <Bar ref={ref} />;
-      });
+      return fakeImport(
+        React.forwardRef((props, ref) => {
+          ReactTestRenderer.unstable_yield('forwardRef');
+          return <Bar ref={ref} />;
+        }),
+      );
     });
 
     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
@@ -34,6 +34,10 @@ describe('pure', () => {
     return <span prop={props.text} />;
   }
 
+  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) {
       return <Pure {...props} />;
     }
-    return React.lazy(async () => Indirection);
+    return React.lazy(() => fakeImport(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
@@ -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(
       <Suspense fallback={<Text text="Loading..." />}>

diff --git a/packages/shared/ReactLazyComponent.js b/packages/shared/ReactLazyComponent.js
@@ -13,14 +13,14 @@ export type Thenable<T, R> = {
 
 export type LazyComponent<T> = {
   $$typeof: Symbol | number,
-  _ctor: () => Thenable<T, mixed>,
+  _ctor: () => Thenable<{default: T}, mixed>,
   _status: 0 | 1 | 2,
   _result: any,
 };
 
-type ResolvedLazyComponentThenable<T> = {
+type ResolvedLazyComponent<T> = {
   $$typeof: Symbol | number,
-  _ctor: () => Thenable<T, mixed>,
+  _ctor: () => Thenable<{default: T}, mixed>,
   _status: 1,
   _result: any,
 };
@@ -30,13 +30,13 @@ export const Resolved = 1;
 export const Rejected = 2;
 
 export function getResultFromResolvedLazyComponent<T>(
-  lazyComponent: ResolvedLazyComponentThenable<T>,
+  lazyComponent: ResolvedLazyComponent<T>,
 ): T {
   return lazyComponent._result;
 }
 
 export function refineResolvedLazyComponent<T>(
   lazyComponent: LazyComponent<T>,
-): ResolvedLazyComponentThenable<T> | null {
+): ResolvedLazyComponent<T> | null {
   return lazyComponent._status === Resolved ? lazyComponent._result : null;
 }