Warn for invalid type in renderer with the correct RSC stack (#30102)

This is all behind the `enableOwnerStacks` flag. This is a follow up to #29088. In that I moved type validation into the renderer since that's the one that knows what types are allowed. However, I only removed it from `React.createElement` and not the JSX which was an oversight. However, I also noticed that for invalid types we don't have the right stack trace for throws because we're not yet inside the JSX element that itself is invalid. We should use its stack for the stack trace. That's the reason it's enough to just use the throw now because we can get a good stack trace from the owner stack. This is fixed by creating a fake Throw Fiber that gets assigned the right stack. Additionally, I noticed that for certain invalid types like the most common one `undefined` we error in Flight so a missing import in RSC leads to a generic error. Instead of erroring on the Flight side we should just let anything that's not a Server Component through to the client and then let the Client renderer determine whether it's a valid type or not. Since we now have owner stacks through the server too, this will still be able to provide a good stack trace on the client that points to the server in that case. <img width="571" alt="Screenshot 2024-06-25 at 6 46 35 PM" src="https://github.com/facebook/react/assets/63648/6812c24f-e274-4e09-b4de-21deda9ea1d4"> To get the best stack you have to expand the little icon and the regular stack is noisy [due to this Chrome bug](https://issues.chromium.org/issues/345248263) which makes it a little harder to find but once that's fixed it might be easier.
facebook · Jun 27, 2024 · e02baf6 · e02baf6
1 parent ffec9ec
commit e02baf6
Show file tree

Hide file tree

Showing 12 changed files with 250 additions and 183 deletions.
diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js
@@ -692,14 +692,22 @@ describe('ReactFlight', () => {
 
     const transport = ReactNoopFlightServer.render(<ServerComponent />);
 
-    await act(async () => {
-      const rootModel = await ReactNoopFlightClient.read(transport);
-      ReactNoop.render(rootModel);
-    });
-    expect(ReactNoop).toMatchRenderedOutput('Loading...');
-    spyOnDevAndProd(console, 'error').mockImplementation(() => {});
     await load();
-    expect(console.error).toHaveBeenCalledTimes(1);
+
+    await expect(async () => {
+      await act(async () => {
+        const rootModel = await ReactNoopFlightClient.read(transport);
+        ReactNoop.render(rootModel);
+      });
+    }).rejects.toThrow(
+      __DEV__
+        ? 'Element type is invalid: expected a string (for built-in components) or a class/function ' +
+            '(for composite components) but got: <div />. ' +
+            'Did you accidentally export a JSX literal instead of a component?'
+        : 'Element type is invalid: expected a string (for built-in components) or a class/function ' +
+            '(for composite components) but got: object.',
+    );
+    expect(ReactNoop).toMatchRenderedOutput(null);
   });
 
   it('can render a lazy element', async () => {

diff --git a/packages/react-dom/src/__tests__/ReactComponent-test.js b/packages/react-dom/src/__tests__/ReactComponent-test.js
@@ -14,6 +14,7 @@ let ReactDOM;
 let ReactDOMClient;
 let ReactDOMServer;
 let act;
+let assertConsoleErrorDev;
 
 describe('ReactComponent', () => {
   beforeEach(() => {
@@ -24,6 +25,8 @@ describe('ReactComponent', () => {
     ReactDOMClient = require('react-dom/client');
     ReactDOMServer = require('react-dom/server');
     act = require('internal-test-utils').act;
+    assertConsoleErrorDev =
+      require('internal-test-utils').assertConsoleErrorDev;
   });
 
   // @gate !disableLegacyMode
@@ -131,8 +134,6 @@ describe('ReactComponent', () => {
 
   // @gate !disableStringRefs
   it('string refs do not detach and reattach on every render', async () => {
-    spyOnDev(console, 'error').mockImplementation(() => {});
-
     let refVal;
     class Child extends React.Component {
       componentDidUpdate() {
@@ -171,6 +172,8 @@ describe('ReactComponent', () => {
       root.render(<Parent />);
     });
 
+    assertConsoleErrorDev(['contains the string ref']);
+
     expect(refVal).toBe(undefined);
     await act(() => {
       root.render(<Parent showChild={true} />);
@@ -511,19 +514,25 @@ describe('ReactComponent', () => {
   });
 
   it('throws usefully when rendering badly-typed elements', async () => {
+    const container = document.createElement('div');
+    const root = ReactDOMClient.createRoot(container);
+
     const X = undefined;
-    let container = document.createElement('div');
-    let root = ReactDOMClient.createRoot(container);
-    await expect(
-      expect(async () => {
-        await act(() => {
-          root.render(<X />);
-        });
-      }).toErrorDev(
-        'React.jsx: type is invalid -- expected a string (for built-in components) ' +
-          'or a class/function (for composite components) but got: undefined.',
-      ),
-    ).rejects.toThrowError(
+    const XElement = <X />;
+    if (gate(flags => !flags.enableOwnerStacks)) {
+      assertConsoleErrorDev(
+        [
+          'React.jsx: type is invalid -- expected a string (for built-in components) ' +
+            'or a class/function (for composite components) but got: undefined.',
+        ],
+        {withoutStack: true},
+      );
+    }
+    await expect(async () => {
+      await act(() => {
+        root.render(XElement);
+      });
+    }).rejects.toThrowError(
       'Element type is invalid: expected a string (for built-in components) ' +
         'or a class/function (for composite components) but got: undefined.' +
         (__DEV__
@@ -533,21 +542,44 @@ describe('ReactComponent', () => {
     );
 
     const Y = null;
-    container = document.createElement('div');
-    root = ReactDOMClient.createRoot(container);
-    await expect(
-      expect(async () => {
-        await act(() => {
-          root.render(<Y />);
-        });
-      }).toErrorDev(
-        'React.jsx: type is invalid -- expected a string (for built-in components) ' +
-          'or a class/function (for composite components) but got: null.',
-      ),
-    ).rejects.toThrowError(
+    const YElement = <Y />;
+    if (gate(flags => !flags.enableOwnerStacks)) {
+      assertConsoleErrorDev(
+        [
+          'React.jsx: type is invalid -- expected a string (for built-in components) ' +
+            'or a class/function (for composite components) but got: null.',
+        ],
+        {withoutStack: true},
+      );
+    }
+    await expect(async () => {
+      await act(() => {
+        root.render(YElement);
+      });
+    }).rejects.toThrowError(
       'Element type is invalid: expected a string (for built-in components) ' +
         'or a class/function (for composite components) but got: null.',
     );
+
+    const Z = true;
+    const ZElement = <Z />;
+    if (gate(flags => !flags.enableOwnerStacks)) {
+      assertConsoleErrorDev(
+        [
+          'React.jsx: type is invalid -- expected a string (for built-in components) ' +
+            'or a class/function (for composite components) but got: boolean.',
+        ],
+        {withoutStack: true},
+      );
+    }
+    await expect(async () => {
+      await act(() => {
+        root.render(ZElement);
+      });
+    }).rejects.toThrowError(
+      'Element type is invalid: expected a string (for built-in components) ' +
+        'or a class/function (for composite components) but got: boolean.',
+    );
   });
 
   it('includes owner name in the error about badly-typed elements', async () => {

diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js
@@ -987,11 +987,13 @@ describe('ReactDOMServerIntegration', () => {
           expect(() => {
             EmptyComponent = <EmptyComponent />;
           }).toErrorDev(
-            'React.jsx: type is invalid -- expected a string ' +
-              '(for built-in components) or a class/function (for composite ' +
-              'components) but got: object. You likely forgot to export your ' +
-              "component from the file it's defined in, or you might have mixed up " +
-              'default and named imports.',
+            gate(flags => flags.enableOwnerStacks)
+              ? []
+              : 'React.jsx: type is invalid -- expected a string ' +
+                  '(for built-in components) or a class/function (for composite ' +
+                  'components) but got: object. You likely forgot to export your ' +
+                  "component from the file it's defined in, or you might have mixed up " +
+                  'default and named imports.',
             {withoutStack: true},
           );
           await render(EmptyComponent);
@@ -1011,9 +1013,11 @@ describe('ReactDOMServerIntegration', () => {
           expect(() => {
             NullComponent = <NullComponent />;
           }).toErrorDev(
-            'React.jsx: type is invalid -- expected a string ' +
-              '(for built-in components) or a class/function (for composite ' +
-              'components) but got: null.',
+            gate(flags => flags.enableOwnerStacks)
+              ? []
+              : 'React.jsx: type is invalid -- expected a string ' +
+                  '(for built-in components) or a class/function (for composite ' +
+                  'components) but got: null.',
             {withoutStack: true},
           );
           await render(NullComponent);
@@ -1029,11 +1033,13 @@ describe('ReactDOMServerIntegration', () => {
           expect(() => {
             UndefinedComponent = <UndefinedComponent />;
           }).toErrorDev(
-            'React.jsx: type is invalid -- expected a string ' +
-              '(for built-in components) or a class/function (for composite ' +
-              'components) but got: undefined. You likely forgot to export your ' +
-              "component from the file it's defined in, or you might have mixed up " +
-              'default and named imports.',
+            gate(flags => flags.enableOwnerStacks)
+              ? []
+              : 'React.jsx: type is invalid -- expected a string ' +
+                  '(for built-in components) or a class/function (for composite ' +
+                  'components) but got: undefined. You likely forgot to export your ' +
+                  "component from the file it's defined in, or you might have mixed up " +
+                  'default and named imports.',
             {withoutStack: true},
           );
 

diff --git a/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js
@@ -13,6 +13,7 @@ let PropTypes;
 let React;
 let ReactDOM;
 let act;
+let assertConsoleErrorDev;
 
 // TODO: Refactor this test once componentDidCatch setState is deprecated.
 describe('ReactLegacyErrorBoundaries', () => {
@@ -42,6 +43,8 @@ describe('ReactLegacyErrorBoundaries', () => {
     ReactDOM = require('react-dom');
     React = require('react');
     act = require('internal-test-utils').act;
+    assertConsoleErrorDev =
+      require('internal-test-utils').assertConsoleErrorDev;
 
     log = [];
 
@@ -2099,32 +2102,38 @@ describe('ReactLegacyErrorBoundaries', () => {
     const Y = undefined;
 
     await expect(async () => {
-      await expect(async () => {
-        const container = document.createElement('div');
-        await act(() => {
-          ReactDOM.render(<X />, container);
-        });
-      }).rejects.toThrow('got: null');
-    }).toErrorDev(
-      'React.jsx: type is invalid -- expected a string ' +
-        '(for built-in components) or a class/function ' +
-        '(for composite components) but got: null.',
-      {withoutStack: 1},
-    );
+      const container = document.createElement('div');
+      await act(() => {
+        ReactDOM.render(<X />, container);
+      });
+    }).rejects.toThrow('got: null');
+    if (gate(flags => !flags.enableOwnerStacks)) {
+      assertConsoleErrorDev(
+        [
+          'React.jsx: type is invalid -- expected a string ' +
+            '(for built-in components) or a class/function ' +
+            '(for composite components) but got: null.',
+        ],
+        {withoutStack: true},
+      );
+    }
 
     await expect(async () => {
-      await expect(async () => {
-        const container = document.createElement('div');
-        await act(() => {
-          ReactDOM.render(<Y />, container);
-        });
-      }).rejects.toThrow('got: undefined');
-    }).toErrorDev(
-      'React.jsx: type is invalid -- expected a string ' +
-        '(for built-in components) or a class/function ' +
-        '(for composite components) but got: undefined.',
-      {withoutStack: 1},
-    );
+      const container = document.createElement('div');
+      await act(() => {
+        ReactDOM.render(<Y />, container);
+      });
+    }).rejects.toThrow('got: undefined');
+    if (gate(flags => !flags.enableOwnerStacks)) {
+      assertConsoleErrorDev(
+        [
+          'React.jsx: type is invalid -- expected a string ' +
+            '(for built-in components) or a class/function ' +
+            '(for composite components) but got: undefined.',
+        ],
+        {withoutStack: true},
+      );
+    }
   });
 
   // @gate !disableLegacyMode

diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js
@@ -220,6 +220,9 @@ function validateFragmentProps(
           // For unkeyed root fragments there's no Fiber. We create a fake one just for
           // error stack handling.
           fiber = createFiberFromElement(element, returnFiber.mode, 0);
+          if (__DEV__) {
+            fiber._debugInfo = currentDebugInfo;
+          }
           fiber.return = returnFiber;
         }
         runWithFiberInDEV(
@@ -242,6 +245,9 @@ function validateFragmentProps(
         // For unkeyed root fragments there's no Fiber. We create a fake one just for
         // error stack handling.
         fiber = createFiberFromElement(element, returnFiber.mode, 0);
+        if (__DEV__) {
+          fiber._debugInfo = currentDebugInfo;
+        }
         fiber.return = returnFiber;
       }
       runWithFiberInDEV(fiber, () => {

diff --git a/packages/react-reconciler/src/ReactFiber.js b/packages/react-reconciler/src/ReactFiber.js
@@ -485,6 +485,7 @@ export function createHostRootFiber(
   return createFiber(HostRoot, null, null, mode);
 }
 
+// TODO: Get rid of this helper. Only createFiberFromElement should exist.
 export function createFiberFromTypeAndProps(
   type: any, // React$ElementType
   key: null | string,
@@ -650,11 +651,18 @@ export function createFiberFromTypeAndProps(
           typeString = type === null ? 'null' : typeof type;
         }
 
-        throw new Error(
+        // The type is invalid but it's conceptually a child that errored and not the
+        // current component itself so we create a virtual child that throws in its
+        // begin phase. This is the same thing we do in ReactChildFiber if we throw
+        // but we do it here so that we can assign the debug owner and stack from the
+        // element itself. That way the error stack will point to the JSX callsite.
+        fiberTag = Throw;
+        pendingProps = new Error(
           'Element type is invalid: expected a string (for built-in ' +
             'components) or a class/function (for composite components) ' +
             `but got: ${typeString}.${info}`,
         );
+        resolvedType = null;
       }
     }
   }

diff --git a/packages/react-reconciler/src/__tests__/ErrorBoundaryReconciliation-test.internal.js b/packages/react-reconciler/src/__tests__/ErrorBoundaryReconciliation-test.internal.js
@@ -6,13 +6,16 @@ describe('ErrorBoundaryReconciliation', () => {
   let ReactTestRenderer;
   let span;
   let act;
+  let assertConsoleErrorDev;
 
   beforeEach(() => {
     jest.resetModules();
 
     ReactTestRenderer = require('react-test-renderer');
     React = require('react');
     act = require('internal-test-utils').act;
+    assertConsoleErrorDev =
+      require('internal-test-utils').assertConsoleErrorDev;
     DidCatchErrorBoundary = class extends React.Component {
       state = {error: null};
       componentDidCatch(error) {
@@ -58,15 +61,17 @@ describe('ErrorBoundaryReconciliation', () => {
       );
     });
     expect(renderer).toMatchRenderedOutput(<span prop="BrokenRender" />);
-    await expect(async () => {
-      await act(() => {
-        renderer.update(
-          <ErrorBoundary fallbackTagName={fallbackTagName}>
-            <BrokenRender fail={true} />
-          </ErrorBoundary>,
-        );
-      });
-    }).toErrorDev(['invalid', 'invalid']);
+    await act(() => {
+      renderer.update(
+        <ErrorBoundary fallbackTagName={fallbackTagName}>
+          <BrokenRender fail={true} />
+        </ErrorBoundary>,
+      );
+    });
+    if (gate(flags => !flags.enableOwnerStacks)) {
+      assertConsoleErrorDev(['invalid', 'invalid']);
+    }
+
     const Fallback = fallbackTagName;
     expect(renderer).toMatchRenderedOutput(<Fallback prop="ErrorBoundary" />);
   }