fix: Throw on invalid elements in JSX route configs (#326)

Also, improve test coverage.

package.json

@@ -81,6 +81,7 @@
     "@babel/runtime-corejs2": "^7.4.0",
     "@restart/context": "^2.1.4",
     "farce": "^0.2.8",
+    "invariant": "^2.2.4",
     "is-promise": "^2.1.0",
     "lodash": "^4.17.11",
     "path-to-regexp": "^1.7.0",

src/makeRouteConfig.js

@@ -1,10 +1,13 @@
+import invariant from 'invariant';
 import React from 'react';
 
 function buildRouteConfig(node, routeConfig) {
   React.Children.forEach(node, child => {
-    if (!React.isValidElement(child)) {
-      return;
-    }
+    invariant(
+      React.isValidElement(child),
+      '`%s` is not a valid React element',
+      child,
+    );
 
     let Type = child.type;
     const { children, ...props } = child.props;

test/Router.test.js

@@ -6,24 +6,16 @@ import ReactTestUtils from 'react-dom/test-utils';
 
 import createFarceRouter from '../src/createFarceRouter';
 import HttpError from '../src/HttpError';
-import RedirectException from '../src/RedirectException';
 import useRouter from '../src/useRouter';
 import withRouter from '../src/withRouter';
 
 import { InstrumentedResolver } from './helpers';
 
 describe('Router', () => {
-  it('should support throwing HttpError in route render method', async () => {
+  it('should render 404 when no routes match', async () => {
     const Router = createFarceRouter({
       historyProtocol: new ServerProtocol('/foo'),
-      routeConfig: [
-        {
-          path: '/foo',
-          render: () => {
-            throw new HttpError(404);
-          },
-        },
-      ],
+      routeConfig: [],
 
       renderError: ({ error }) => <div className={`error-${error.status}`} />,
     });
@@ -33,27 +25,24 @@ describe('Router', () => {
       <Router resolver={resolver} />,
     );
 
-    await resolver.done;
     await delay(10);
 
     ReactTestUtils.findRenderedDOMComponentWithClass(instance, 'error-404');
   });
 
-  it('should support throwing RedirectException in route render method', async () => {
+  it('should support throwing HttpError in route render method', async () => {
     const Router = createFarceRouter({
-      historyProtocol: new MemoryProtocol('/foo'),
+      historyProtocol: new ServerProtocol('/foo'),
       routeConfig: [
         {
           path: '/foo',
           render: () => {
-            throw new RedirectException('/bar');
+            throw new HttpError(404);
           },
         },
-        {
-          path: '/bar',
-          render: () => <div className="bar" />,
-        },
       ],
+
+      renderError: ({ error }) => <div className={`error-${error.status}`} />,
     });
 
     const resolver = new InstrumentedResolver();
@@ -64,7 +53,7 @@ describe('Router', () => {
     await resolver.done;
     await delay(10);
 
-    ReactTestUtils.findRenderedDOMComponentWithClass(instance, 'bar');
+    ReactTestUtils.findRenderedDOMComponentWithClass(instance, 'error-404');
   });
 
   it('should support reloading the route configuration', async () => {

test/__snapshots__/makeRouteConfig.test.js.snap

@@ -0,0 +1,3 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`makeRouteConfig should error on non-element children 1`] = `"\`,\` is not a valid React element"`;

test/makeRouteConfig.test.js

@@ -33,6 +33,7 @@ describe('makeRouteConfig', () => {
       makeRouteConfig(
         <Route path="/" Component={AppPage}>
           <Route Component={MainPage} />
+          {/* This comment should be ignored. */}
           <Route path="foo" Component={FooPage}>
             <Route path="bar" Component={BarPage} />
           </Route>
@@ -189,6 +190,18 @@ describe('makeRouteConfig', () => {
     ]);
   });
 
+  it('should error on non-element children', () => {
+    expect(() => {
+      makeRouteConfig(
+        <Route path="/" Component={AppPage}>
+          {/* The comma below will fail the invariant. */}
+          <Route path="foo" Component={FooPage} />,
+          <Route path="bar" Component={BarPage} />
+        </Route>,
+      );
+    }).toThrowErrorMatchingSnapshot();
+  });
+
   ['react-proxy', 'react-stand-in'].forEach(packageName => {
     it(`should work with proxies from ${packageName}`, () => {
       // eslint-disable-next-line global-require, import/no-dynamic-require

test/redirect.test.js

@@ -0,0 +1,62 @@
+import delay from 'delay';
+import MemoryProtocol from 'farce/lib/MemoryProtocol';
+import React from 'react';
+import ReactTestUtils from 'react-dom/test-utils';
+
+import createFarceRouter from '../src/createFarceRouter';
+import Redirect from '../src/Redirect';
+import RedirectException from '../src/RedirectException';
+
+import { InstrumentedResolver } from './helpers';
+
+describe('redirect', () => {
+  async function assertRedirect(fooRoute) {
+    const Router = createFarceRouter({
+      historyProtocol: new MemoryProtocol('/foo'),
+      routeConfig: [
+        fooRoute,
+        {
+          path: '/bar',
+          render: () => <div className="bar" />,
+        },
+      ],
+    });
+
+    const resolver = new InstrumentedResolver();
+    const instance = ReactTestUtils.renderIntoDocument(
+      <Router resolver={resolver} />,
+    );
+
+    await resolver.done;
+    await delay(10);
+
+    ReactTestUtils.findRenderedDOMComponentWithClass(instance, 'bar');
+  }
+
+  it('should support static redirects', async () => {
+    await assertRedirect(
+      new Redirect({
+        from: '/foo',
+        to: '/bar',
+      }),
+    );
+  });
+
+  it('should support function redirects', async () => {
+    await assertRedirect(
+      new Redirect({
+        from: '/foo',
+        to: ({ location }) => location.pathname.replace('foo', 'bar'),
+      }),
+    );
+  });
+
+  it('should support throwing RedirectException in route render method', async () => {
+    await assertRedirect({
+      path: '/foo',
+      render: () => {
+        throw new RedirectException('/bar');
+      },
+    });
+  });
+});

test/render.test.js

@@ -4,7 +4,6 @@ import React from 'react';
 import ReactTestUtils from 'react-dom/test-utils';
 
 import createFarceRouter from '../src/createFarceRouter';
-import createRender from '../src/createRender';
 
 import { InstrumentedResolver } from './helpers';
 
@@ -34,9 +33,7 @@ describe('render', () => {
         },
       ],
 
-      render: createRender({
-        renderPending: () => <div className="pending" />,
-      }),
+      renderPending: () => <div className="pending" />,
     });
 
     const resolver = new InstrumentedResolver();
@@ -187,4 +184,48 @@ describe('render', () => {
       ReactTestUtils.scryRenderedDOMComponentsWithClass(instance, 'qux'),
     ).toHaveLength(0);
   });
+
+  it('should support custom renderReady', async () => {
+    const Router = createFarceRouter({
+      historyProtocol: new ServerProtocol('/foo'),
+      routeConfig: [
+        {
+          path: 'foo',
+          render: () => null,
+        },
+      ],
+
+      renderReady: () => <div className="ready" />,
+    });
+
+    const resolver = new InstrumentedResolver();
+    const instance = ReactTestUtils.renderIntoDocument(
+      <Router resolver={resolver} />,
+    );
+
+    await resolver.done;
+    ReactTestUtils.findRenderedDOMComponentWithClass(instance, 'ready');
+  });
+
+  it('should support fully custom render', async () => {
+    const Router = createFarceRouter({
+      historyProtocol: new ServerProtocol('/foo'),
+      routeConfig: [
+        {
+          path: 'foo',
+          render: () => null,
+        },
+      ],
+
+      render: () => <div className="rendered" />,
+    });
+
+    const resolver = new InstrumentedResolver();
+    const instance = ReactTestUtils.renderIntoDocument(
+      <Router resolver={resolver} />,
+    );
+
+    await resolver.done;
+    ReactTestUtils.findRenderedDOMComponentWithClass(instance, 'rendered');
+  });
 });